Close issue 38, added an ability to specify statusbar/ directory.

[lisp3r]

1. Added changes from [SUGGESTION] execute commands directly rather than through a shell #38.
2. Added an ability to specify scripts directory through SCRIPTS_DIR value.

I used to add my scripts directory into $PATH which isn't a good practice. So I made this little patch.

[ListeriaM]

suggest opening SCRIPTS_DIR once at startup with open(SCRIPTS_DIR, O_DIRECTORY | O_PATH | O_CLOEXEC) which would enable using execveat(2): execveat(scripts_dir_fd, blocks[i].command, (char **){ blocks[i].command, NULL }, environ). That way you can do without concat_paths().

[lisp3r]

Could you provide the complete code? I tried to use execveat with a directory file descriptor but only got "No such file or directory".

Here is my snippet which is getting "No such file or directory":

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <linux/fcntl.h>      /* Definition of AT_* constants */
#include <unistd.h>
#include <errno.h>

int errno;
extern char **environ;


int main(const int argc, const char* argv[]) {

    char* SCRIPTS_DIR = "/opt/dwmblocks-async/statusbar/";
    int scripts_dir_fd = open(SCRIPTS_DIR, O_DIRECTORY | O_PATH | O_CLOEXEC);

    if (scripts_dir_fd < 0) {
        printf("Error while openning %s: %d\n", SCRIPTS_DIR, scripts_dir_fd);
        exit(1);
    } else {
        printf("File descriptor: %d\n", scripts_dir_fd);
    }

    // int i = 0;
    // while(environ[i]) {
    //     printf("%s\n", environ[i++]);
    // }

    const char * pathname = "sb-battery";
    const char *const argvs[] = {pathname, NULL};

    int res = execveat(scripts_dir_fd, pathname, argvs, environ);
    if (res == -1) {
        printf("An error occured: %s (errno = %d)\n", strerror(errno), errno);
    }
}

[ListeriaM]

I missed it in my earlier comment, but it's missing the flags argument to execveat(2):

#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

extern char **environ;

int
main(void)
{
    const char *SCRIPTS_DIR = "/usr/bin";
    int scripts_dir_fd = open(SCRIPTS_DIR, O_DIRECTORY | O_PATH | O_CLOEXEC);

    if (scripts_dir_fd < 0) {
        printf("Error while openning %s: %d\n", SCRIPTS_DIR, scripts_dir_fd);
        exit(1);
    } else {
        printf("File descriptor: %d\n", scripts_dir_fd);
    }

    const char *pathname = "echo";
    char *const argv[] = { "echo", "Hello", NULL };

    execveat(scripts_dir_fd, pathname, argv, environ, 0);
    printf("execveat %s/%s: %s\n", SCRIPTS_DIR, pathname, strerror(errno));
}

But I honestly think the other alternative I provided is simpler.

[lisp3r]

Both of our snippets only work for binaries in the pathname, not for scripts. I have studied execveat documentation again and found this:

ERRORS

ENOENT The program identified by dirfd and pathname requires the
       use of an interpreter program (such as a script starting
       with "#!"), but the file descriptor dirfd was opened with
       the O_CLOEXEC flag, with the result that the program file
       is inaccessible to the launched interpreter.  See BUGS.

BUGS

The ENOENT error described above means that it is not possible to
set the close-on-exec flag on the file descriptor given to a call
of the form:

    execveat(fd, "", argv, envp, AT_EMPTY_PATH);

However, the inability to set the close-on-exec flag means that a
file descriptor referring to the script leaks through to the
script itself.  As well as wasting a file descriptor, this
leakage can lead to file-descriptor exhaustion in scenarios where
scripts recursively employ execveat().

Due this I do not think it is a good idea to use execveat.

The other alternative you provided doesn't work too. At first you can't put #ifdef inside #define. Also it isn't possible to use SCRIPTS_DIR before config.h inclusion.

[ListeriaM]

Good points, I hadn't tested any of these. The #ifdef inside #define thing can be solved trivially, though. The use of SCRIPTS_DIR before inclusion of config.h would require defining BLOCK in config.h, which seems appropiate given that it's only used there, similar to the SHCMD(cmd) macro in dwm.

This could be an issue because it adds complexity to config.h.

[ListeriaM]

Also possible is not using O_CLOEXEC on scripts_dir_fd and leak the file descriptor, it shouldn't be an issue

[ListeriaM]

also possible is:

#ifdef SCRIPTS_DIR
#define BLOCK(cmd, interval, signal) \
    { SCRIPTS_DIR "/" cmd, interval, signal }
#else
#define BLOCK(cmd, interval, signal) \
    { cmd, interval, signal }
#endif

which seems simpler IMO. It may add an extra '/' which doesn't affect the behavior, but it is more robust.

[ListeriaM]

see #24 (comment).

[UtkarshVerma]

Hi guys, thanks for taking out the time on working towards this issue. However, I'll need to close this PR since I have found a fix for #38 and it is implemented in the new-org branch. I'm facing a few hiccups from a code organization point of view, but technically there is no problem, as far as I've tested it.