Re: [PATCH V2] libxl: introduce asynchronous execution API

[Hongyang Yang <yanghy@cn.fujitsu.com>]

Ping!

On 05/05/2014 12:14 PM, Yang Hongyang wrote:
> 1.introduce asynchronous execution API:
>    libxl__async_exec_init
>    libxl__async_exec_start
>    libxl__async_exec_inuse
> 2.use the async exec API to execute device hotplug scripts
>
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   tools/libxl/libxl_aoutils.c  | 89 ++++++++++++++++++++++++++++++++++++++++++++
>   tools/libxl/libxl_device.c   | 78 +++++++++++---------------------------
>   tools/libxl/libxl_internal.h | 34 +++++++++++++++--
>   3 files changed, 141 insertions(+), 60 deletions(-)
>
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 1c9eb9e..b10d2e1 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -451,3 +451,92 @@ int libxl__openptys(libxl__openpty_state *op,
>       return rc;
>   }
>
> +static void async_exec_timeout(libxl__egc *egc,
> +                               libxl__ev_time *ev,
> +                               const struct timeval *requested_abs)
> +{
> +    libxl__async_exec_state *aes = CONTAINER_OF(ev, *aes, time);
> +    STATE_AO_GC(aes->ao);
> +
> +    libxl__ev_time_deregister(gc, &aes->time);
> +
> +    assert(libxl__ev_child_inuse(&aes->child));
> +    LOG(ERROR, "killing execution of %s because of timeout", aes->what);
> +
> +    if (kill(aes->child.pid, SIGKILL)) {
> +        LOGEV(ERROR, errno, "unable to kill %s [%ld]",
> +              aes->what, (unsigned long)aes->child.pid);
> +    }
> +
> +    return;
> +}
> +
> +static void async_exec_done(libxl__egc *egc,
> +                            libxl__ev_child *child,
> +                            pid_t pid, int status)
> +{
> +    libxl__async_exec_state *aes = CONTAINER_OF(child, *aes, child);
> +    STATE_AO_GC(aes->ao);
> +
> +    libxl__ev_time_deregister(gc, &aes->time);
> +
> +    if (status) {
> +        libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
> +                                      aes->what, pid, status);
> +    }
> +
> +    aes->callback(egc, aes, status);
> +}
> +
> +void libxl__async_exec_init(libxl__async_exec_state *aes)
> +{
> +    libxl__ev_time_init(&aes->time);
> +    libxl__ev_child_init(&aes->child);
> +}
> +
> +int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
> +{
> +    pid_t pid;
> +
> +    /* Convenience aliases */
> +    libxl__ev_child *const child = &aes->child;
> +    char ** const args = aes->args;
> +
> +    /* Set execution timeout */
> +    if (libxl__ev_time_register_rel(gc, &aes->time,
> +                                    async_exec_timeout,
> +                                    aes->timeout_ms)) {
> +        LOG(ERROR, "unable to register timeout for executing: %s", aes->what);
> +        goto out;
> +    }
> +
> +    LOG(DEBUG, "forking to execute: %s ", aes->what);
> +
> +    /* Fork and exec */
> +    pid = libxl__ev_child_fork(gc, child, async_exec_done);
> +    if (pid == -1) {
> +        LOG(ERROR, "unable to fork");
> +        goto out;
> +    }
> +
> +    if (!pid) {
> +        /* child */
> +        libxl__exec(gc, aes->stdfds[0], aes->stdfds[1],
> +                    aes->stdfds[2], args[0], args, aes->env);
> +        /* notreached */
> +        abort();
> +    }
> +
> +    return 0;
> +
> +out:
> +    return ERROR_FAIL;
> +}
> +
> +bool libxl__async_exec_inuse(const libxl__async_exec_state *aes)
> +{
> +    bool time_inuse = libxl__ev_time_isregistered(&aes->time);
> +    bool child_inuse = libxl__ev_child_inuse(&aes->child);
> +    assert(time_inuse == child_inuse);
> +    return child_inuse;
> +}
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index fa99f77..90ae564 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -430,7 +430,7 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
>       aodev->rc = 0;
>       aodev->dev = NULL;
>       aodev->num_exec = 0;
> -    /* Initialize timer for QEMU Bodge and hotplug execution */
> +    /* Initialize timer for QEMU Bodge */
>       libxl__ev_time_init(&aodev->timeout);
>       /*
>        * Initialize xs_watch, because it's not used on all possible
> @@ -440,7 +440,7 @@ void libxl__prepare_ao_device(libxl__ao *ao, libxl__ao_device *aodev)
>       aodev->active = 1;
>       /* We init this here because we might call device_hotplug_done
>        * without actually calling any hotplug script */
> -    libxl__ev_child_init(&aodev->child);
> +    libxl__async_exec_init(&aodev->aes);
>   }
>
>   /* multidev */
> @@ -707,12 +707,9 @@ static void device_backend_cleanup(libxl__gc *gc,
>
>   static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev);
>
> -static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> -                                      const struct timeval *requested_abs);
> -
>   static void device_hotplug_child_death_cb(libxl__egc *egc,
> -                                          libxl__ev_child *child,
> -                                          pid_t pid, int status);
> +                                          libxl__async_exec_state *aes,
> +                                          int status);
>
>   static void device_destroy_be_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
>                                            const struct timeval *requested_abs);
> @@ -953,11 +950,11 @@ static void device_backend_cleanup(libxl__gc *gc, libxl__ao_device *aodev)
>   static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
>   {
>       STATE_AO_GC(aodev->ao);
> +    libxl__async_exec_state *aes = &aodev->aes;
>       char *be_path = libxl__device_backend_path(gc, aodev->dev);
>       char **args = NULL, **env = NULL;
>       int rc = 0;
>       int hotplug, nullfd = -1;
> -    pid_t pid;
>       uint32_t domid;
>
>       /*
> @@ -1009,16 +1006,6 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
>           goto out;
>       }
>
> -    /* Set hotplug timeout */
> -    rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
> -                                     device_hotplug_timeout_cb,
> -                                     LIBXL_HOTPLUG_TIMEOUT * 1000);
> -    if (rc) {
> -        LOG(ERROR, "unable to register timeout for hotplug device %s", be_path);
> -        goto out;
> -    }
> -
> -    aodev->what = GCSPRINTF("%s %s", args[0], args[1]);
>       LOG(DEBUG, "calling hotplug script: %s %s", args[0], args[1]);
>
>       nullfd = open("/dev/null", O_RDONLY);
> @@ -1028,23 +1015,22 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
>           goto out;
>       }
>
> -    /* fork and execute hotplug script */
> -    pid = libxl__ev_child_fork(gc, &aodev->child, device_hotplug_child_death_cb);
> -    if (pid == -1) {
> -        LOG(ERROR, "unable to fork");
> -        rc = ERROR_FAIL;
> +    aes->ao = ao;
> +    aes->what = GCSPRINTF("%s %s", args[0], args[1]);
> +    aes->env = env;
> +    aes->args = args;
> +    aes->callback = device_hotplug_child_death_cb;
> +    aes->timeout_ms = LIBXL_HOTPLUG_TIMEOUT * 1000;
> +    aes->stdfds[0] = nullfd;
> +    aes->stdfds[1] = 2;
> +    aes->stdfds[2] = -1;
> +
> +    rc = libxl__async_exec_start(gc, aes);
> +    if (rc)
>           goto out;
> -    }
> -
> -    if (!pid) {
> -        /* child */
> -        libxl__exec(gc, nullfd, 2, -1, args[0], args, env);
> -        /* notreached */
> -        abort();
> -    }
>
>       close(nullfd);
> -    assert(libxl__ev_child_inuse(&aodev->child));
> +    assert(libxl__async_exec_inuse(&aodev->aes));
>
>       return;
>
> @@ -1055,29 +1041,11 @@ out:
>       return;
>   }
>
> -static void device_hotplug_timeout_cb(libxl__egc *egc, libxl__ev_time *ev,
> -                                      const struct timeval *requested_abs)
> -{
> -    libxl__ao_device *aodev = CONTAINER_OF(ev, *aodev, timeout);
> -    STATE_AO_GC(aodev->ao);
> -
> -    libxl__ev_time_deregister(gc, &aodev->timeout);
> -
> -    assert(libxl__ev_child_inuse(&aodev->child));
> -    LOG(DEBUG, "killing hotplug script %s because of timeout", aodev->what);
> -    if (kill(aodev->child.pid, SIGKILL)) {
> -        LOGEV(ERROR, errno, "unable to kill hotplug script %s [%ld]",
> -                            aodev->what, (unsigned long)aodev->child.pid);
> -    }
> -
> -    return;
> -}
> -
>   static void device_hotplug_child_death_cb(libxl__egc *egc,
> -                                          libxl__ev_child *child,
> -                                          pid_t pid, int status)
> +                                          libxl__async_exec_state *aes,
> +                                          int status)
>   {
> -    libxl__ao_device *aodev = CONTAINER_OF(child, *aodev, child);
> +    libxl__ao_device *aodev = CONTAINER_OF(aes, *aodev, aes);
>       STATE_AO_GC(aodev->ao);
>       char *be_path = libxl__device_backend_path(gc, aodev->dev);
>       char *hotplug_error;
> @@ -1085,8 +1053,6 @@ static void device_hotplug_child_death_cb(libxl__egc *egc,
>       device_hotplug_clean(gc, aodev);
>
>       if (status) {
> -        libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
> -                                      aodev->what, pid, status);
>           hotplug_error = libxl__xs_read(gc, XBT_NULL,
>                                          GCSPRINTF("%s/hotplug-error", be_path));
>           if (hotplug_error)
> @@ -1178,7 +1144,7 @@ static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev)
>       /* Clean events and check reentrancy */
>       libxl__ev_time_deregister(gc, &aodev->timeout);
>       libxl__ev_xswatch_deregister(gc, &aodev->xs_watch);
> -    assert(!libxl__ev_child_inuse(&aodev->child));
> +    assert(!libxl__async_exec_inuse(&aodev->aes));
>   }
>
>   static void devices_remove_callback(libxl__egc *egc,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c2b73c4..fe4a3d1 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2030,6 +2030,33 @@ _hidden const char *libxl__xen_script_dir_path(void);
>   _hidden const char *libxl__lock_dir_path(void);
>   _hidden const char *libxl__run_dir_path(void);
>
> +/*----- subprocess execution with timeout -----*/
> +
> +typedef struct libxl__async_exec_state libxl__async_exec_state;
> +
> +typedef void libxl__async_exec_callback(libxl__egc *egc,
> +                        libxl__async_exec_state *aes, int status);
> +
> +struct libxl__async_exec_state {
> +    /* caller must fill these in */
> +    libxl__ao *ao;
> +    const char *what; /* for error msgs, what we're executing */
> +    int timeout_ms;
> +    libxl__async_exec_callback *callback;
> +    /* caller must fill in; as for libxl__exec */
> +    int stdfds[3];
> +    char **args; /* execution arguments */
> +    char **env; /* execution environment */
> +
> +    /* private */
> +    libxl__ev_time time;
> +    libxl__ev_child child;
> +};
> +
> +void libxl__async_exec_init(libxl__async_exec_state *aes);
> +int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes);
> +bool libxl__async_exec_inuse(const libxl__async_exec_state *aes);
> +
>   /*----- device addition/removal -----*/
>
>   typedef struct libxl__ao_device libxl__ao_device;
> @@ -2066,14 +2093,13 @@ struct libxl__ao_device {
>       libxl__multidev *multidev; /* reference to the containing multidev */
>       /* private for add/remove implementation */
>       libxl__ev_devstate backend_ds;
> -    /* Bodge for Qemu devices, also used for timeout of hotplug execution */
> +    /* Bodge for Qemu devices */
>       libxl__ev_time timeout;
>       /* xenstore watch for backend path of driver domains */
>       libxl__ev_xswatch xs_watch;
> -    /* device hotplug execution */
> -    const char *what;
>       int num_exec;
> -    libxl__ev_child child;
> +    /* for calling hotplug scripts */
> +    libxl__async_exec_state aes;
>   };
>
>   /*
>

-- 
Thanks,
Yang.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel