[systemd-devel] [PATCH 6/6] udevd: event - port spawn_wait() to sd-event

[Zbigniew Jędrzejewski-Szmek]

On Tue, May 26, 2015 at 12:38:48AM +0200, Tom Gundersen wrote:
> This allows us to drop the special sigterm handling in spawn_wait()
> as this will now be passed directly to the worker event loop.
> 
> We now log failing processe at 'warning' leve, otherwise the
> behavior is unchanged.

Changes look nice. I think you can push them... if anything is wrong,
we'll see soon enough.

> ---
>  src/test/test-udev.c    |   7 --
>  src/udev/udev-event.c   | 177 +++++++++++++++++++++++++-----------------------
>  src/udev/udev.h         |   2 -
>  src/udev/udevadm-test.c |   8 ---
>  src/udev/udevd.c        |   8 ---
>  5 files changed, 94 insertions(+), 108 deletions(-)
> 
> diff --git a/src/test/test-udev.c b/src/test/test-udev.c
> index 23b7faa..f3953fe 100644
> --- a/src/test/test-udev.c
> +++ b/src/test/test-udev.c
> @@ -120,11 +120,6 @@ int main(int argc, char *argv[]) {
>  
>          sigfillset(&mask);
>          sigprocmask(SIG_SETMASK, &mask, &sigmask_orig);
> -        event->fd_signal = signalfd(-1, &mask, SFD_NONBLOCK|SFD_CLOEXEC);
> -        if (event->fd_signal < 0) {
> -                fprintf(stderr, "error creating signalfd\n");
> -                goto out;
> -        }
>  
>          /* do what devtmpfs usually provides us */
>          if (udev_device_get_devnode(dev) != NULL) {
> @@ -153,8 +148,6 @@ int main(int argc, char *argv[]) {
>                                 3 * USEC_PER_SEC, USEC_PER_SEC,
>                                 NULL);
>  out:
> -        if (event != NULL && event->fd_signal >= 0)
> -                close(event->fd_signal);
>          mac_selinux_finish();
>  
>          return err ? EXIT_FAILURE : EXIT_SUCCESS;
> diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c
> index 2fa26a4..b8c79b1 100644
> --- a/src/udev/udev-event.c
> +++ b/src/udev/udev-event.c
> @@ -32,7 +32,14 @@
>  
>  #include "udev.h"
>  #include "rtnl-util.h"
> +#include "event-util.h"
>  #include "formats-util.h"
> +#include "process-util.h"
> +
> +typedef struct Spawn {
> +        const char *cmd;
> +        pid_t pid;
> +} Spawn;
>  
>  struct udev_event *udev_event_new(struct udev_device *dev) {
>          struct udev *udev = udev_device_get_udev(dev);
> @@ -45,7 +52,6 @@ struct udev_event *udev_event_new(struct udev_device *dev) {
>          event->udev = udev;
>          udev_list_init(udev, &event->run_list, false);
>          udev_list_init(udev, &event->seclabel_list, false);
> -        event->fd_signal = -1;
>          event->birth_usec = now(CLOCK_MONOTONIC);
>          return event;
>  }
> @@ -540,102 +546,107 @@ static void spawn_read(struct udev_event *event,
>                  result[respos] = '\0';
>  }
>  
> -static int spawn_wait(struct udev_event *event,
> -                      usec_t timeout_usec,
> -                      usec_t timeout_warn_usec,
> -                      const char *cmd, pid_t pid) {
> -        struct pollfd pfd[1];
> -        int err = 0;
> +static int on_spawn_timeout(sd_event_source *s, uint64_t usec, void *userdata) {
> +        Spawn *spawn = userdata;
>  
> -        pfd[0].events = POLLIN;
> -        pfd[0].fd = event->fd_signal;
> +        assert(spawn);
>  
> -        while (pid > 0) {
> -                int timeout;
> -                int timeout_warn = 0;
> -                int fdcount;
> +        kill_and_sigcont(spawn->pid, SIGKILL);
>  
> -                if (timeout_usec > 0) {
> -                        usec_t age_usec;
> +        log_error("timeout: '%s' ["PID_FMT"], killing", spawn->cmd, spawn->pid);
>  
> -                        age_usec = now(CLOCK_MONOTONIC) - event->birth_usec;
> -                        if (age_usec >= timeout_usec)
> -                                timeout = 1000;
> -                        else {
> -                                if (timeout_warn_usec > 0)
> -                                        timeout_warn = ((timeout_warn_usec - age_usec) / USEC_PER_MSEC) + MSEC_PER_SEC;
> +        return 1;
> +}
>  
> -                                timeout = ((timeout_usec - timeout_warn_usec - age_usec) / USEC_PER_MSEC) + MSEC_PER_SEC;
> -                        }
> -                } else {
> -                        timeout = -1;
> -                }
> +static int on_spawn_timeout_warning(sd_event_source *s, uint64_t usec, void *userdata) {
> +        Spawn *spawn = userdata;
>  
> -                fdcount = poll(pfd, 1, timeout_warn);
> -                if (fdcount < 0) {
> -                        if (errno == EINTR)
> -                                continue;
> -                        err = -errno;
> -                        log_error_errno(errno, "failed to poll: %m");
> -                        goto out;
> -                }
> -                if (fdcount == 0) {
> -                        log_warning("slow: '%s' ["PID_FMT"]", cmd, pid);
> +        assert(spawn);
>  
> -                        fdcount = poll(pfd, 1, timeout);
> -                        if (fdcount < 0) {
> -                                if (errno == EINTR)
> -                                        continue;
> -                                err = -errno;
> -                                log_error_errno(errno, "failed to poll: %m");
> -                                goto out;
> -                        }
> -                        if (fdcount == 0) {
> -                                log_error("timeout: killing '%s' ["PID_FMT"]", cmd, pid);
> -                                kill(pid, SIGKILL);
> -                        }
> +        log_warning("slow: '%s' ["PID_FMT"]", spawn->cmd, spawn->pid);
I think it would be nice to include the timeout length in the
message for convenience.

Zbyszek