Hi,
Please find a little review of the code.
Le jeudi 19 août 2010 à 18:46 +0300, Alexander Shishkin a écrit :
> This is a new utility that makes it possible and easy to make use of
> eventfd(2) IPC mechanism in shell scripts. It can be used for signalling
> between userspace processes and userspace and kernel (currently, cgroup
> notifications use this).
> diff --git a/configure.ac b/configure.ac
> index eaa5945..533c05c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -673,6 +673,8 @@ dnl unshare could be available as libc function or as syscall only
> UTIL_CHECK_SYSCALL([unshare])
> AC_CHECK_FUNCS([unshare])
>
> +AC_CHECK_FUNCS([eventfd])
> +
You should add a check for <sys/eventfd.h> too.
> diff --git a/sys-utils/eventfd.c b/sys-utils/eventfd.c
> new file mode 100644
> index 0000000..42f1522
> --- /dev/null
> +++ b/sys-utils/eventfd.c
> +
> +#define strtoeventfd(x) \
What is "x" parameter ?
> + ({ sizeof(long) == 4 \
> + ? strtoull(optarg, NULL, 10) \
> + : strtoul(optarg, NULL, 10); })
> +
Why not using strtoull() in all cases, since this macro is used to read
a uint64_t ?
> +static void watcher(int fd, const char *cmd)
> +{
> + struct pollfd fds = { .fd = fd, .events = POLLIN };
> +
> + while (poll(&fds, 1, -1) > 0) {
> + uint64_t efdval;
> + size_t len;
len should be ssize_t
> + char s[BUFSIZ];
> +
> + len = read(fd, &efdval, sizeof efdval);
> + if (len != sizeof(uint64_t)) {
Be consistent: use either sizeof(uint64_t) or sizeof efdval
> + perror(_("short read from eventfd"));
> + exit(EXIT_FAILURE);
> + }
> +
> + snprintf(s, BUFSIZ, "%lu", efdval);
%lu could not be used to format an uint64_t (without a cast at least).
> +
> + setenv("__EVENTFDVAL", s, 1);
> + system(cmd);
> + }
> +
> + fprintf(stderr, _("eventfd poll failed\n"));
Use perror() here too.
> +}
> +
> +static pid_t watcher_pid = -1;
> +
> +static void watcher_start(int fd, char *watchcmd)
> +{
> + watcher_pid = fork();
> +
> + if (watcher_pid < 0) {
> + perror(_("fork failed"));
> + exit(EXIT_FAILURE);
> + } else if (!watcher_pid)
> + watcher(fd, watchcmd);
> +}
> +
> +/* Quis custodiet ipsos custodes? */
In english
> +static void watcher_reap(void)
> +{
> + if (watcher_pid <= 0)
> + return;
> +
> + kill(watcher_pid, SIGTERM);
> + while (waitpid(watcher_pid, NULL, 0) != watcher_pid)
> + ;
> +}
> +
> +static int writer(char *varname, uint64_t echoval)
> +{
> + char *fdstr = getenv(varname);
> + int fd;
> + ssize_t len;
> +
> + if (!fdstr) {
> + fprintf(stderr, _("$%s is empty\n"), varname);
> + return EXIT_FAILURE;
> + }
> +
> + fd = atoi(fdstr);
Check fdstr content. You could also use strtol here.
> + len = write(fd, &echoval, 8);
8 ... use sizeof(uint64_t)
> + if (len == -1) {
> + perror(_("write failed"));
> + return EXIT_FAILURE;
> + }
> +
> + return EXIT_SUCCESS;
> +}
> +
> +static const struct option options[] = {
> + { "nofork", 0, 0, 'F' },
> + { "echo", 1, 0, 'e' },
> + { "varname", 1, 0, 'n' },
> + { "watchcmd", 1, 0, 'w' },
> + { "init", 1, 0, 'i' },
> + { "help", 0, 0, 'h' },
> + { NULL, 0, 0, 0 },
> +};
> +
> +static const char *optdescs[] = {
> + "exec the main process without forking",
> + "echo a number into eventfd",
> + "eventfd variable name",
> + "command to launch upon signal arrival",
> + "initial value of the eventfd counter",
> + "print help message"
> +};
> +
> +static void usage(int exitcode)
> +{
> + int i;
> +
> + printf(_("usage: %s [OPTIONS] <program> ...\n\n"
> + "Create, write and poll eventfds.\nOPTIONS:\n"),
> + program_invocation_short_name);
> + for (i = 0; options[i].name; i++)
> + printf(" -%c, --%-10s %s\n", options[i].val, options[i].name,
> + _(optdescs[i]));
> +
> + exit(exitcode);
> +}
> +
> +static const char optstr[] = "Fe:n:w:i:h";
> +
> +int main(int argc, char *argv[])
> +{
> + uint64_t echoval = 0;
> + unsigned int initval = 0;
> + int fd, c, loptidx, nofork = 0;
> + char *varname = "EVENTFD";
> + char val[BUFSIZ];
> + char **nargv;
> + char *watchcmd = NULL;
> +
> + for (;;) {
> + c = getopt_long(argc, argv, optstr, options, NULL);
> + if (c == -1)
> + break;
> +
> + switch (c) {
> + case 'e':
> + echoval = strtoeventfd(optarg);
> + break;
> + case 'i':
> + initval = strtoul(optarg, NULL, 10);
> + break;
> + case 'n':
> + varname = strdup(optarg);
> + break;
> + case 'F':
> + nofork++;
> + break;
> + case 'w':
> + watchcmd = strdup(optarg);
> + break;
> + case 'h':
> + usage(EXIT_SUCCESS);
> + default:
> + usage(EXIT_FAILURE);
> + }
> + }
Could you check strtoul{l}() return in order to catch error in
argument ?
> +
> + if (echoval)
> + return writer(varname, echoval);
> +
So this is impossible to echo the value 0 ?
Regards
--
Yann Droneaud
--
To unsubscribe from this list: send the line "unsubscribe util-linux-ng" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
