[PATCH] libxl: introduce asynchronous execution API

[PATCH] libxl: introduce asynchronous execution API

[Yang Hongyang]

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>
---
 tools/libxl/libxl_aoutils.c  | 89 ++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_device.c   | 78 +++++++++++---------------------------
 tools/libxl/libxl_internal.h | 32 ++++++++++++++--
 3 files changed, 139 insertions(+), 60 deletions(-)

diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
index 1c9eb9e..6215a3d 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(DEBUG, "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 errout;
+    }
+
+    LOG(DEBUG, "asynchronously 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 errout;
+    }
+
+    if (!pid) {
+        /* child */
+        libxl__exec(gc, aes->stdfds[0], aes->stdfds[1],
+                    aes->stdfds[2], args[0], args, aes->env);
+        /* notreached */
+        abort();
+    }
+
+    return 0;
+
+errout:
+    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..3b9acc5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2030,6 +2030,31 @@ _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);
 
+/*----- asynchronous function -----*/
+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 execute */
+    char **env; /* execution environment */
+    char **args; /* execution arguments */
+    libxl__async_exec_callback *callback;
+    int timeout_ms;
+    int stdfds[3];
+
+    /* 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 +2091,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;
 };
 
 /*
-- 
1.9.1

Re: [PATCH] libxl: introduce asynchronous execution API

[Roger Pau Monné]

On 25/04/14 07:26, 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

Thanks, the patch looks fine to me, just a couple of comments on style
nits (but it can be applied as-is).

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> 
> 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>
> ---
>  tools/libxl/libxl_aoutils.c  | 89 ++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_device.c   | 78 +++++++++++---------------------------
>  tools/libxl/libxl_internal.h | 32 ++++++++++++++--
>  3 files changed, 139 insertions(+), 60 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 1c9eb9e..6215a3d 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(DEBUG, "killing execution of %s because of timeout", aes->what);

I would make the message above an ERROR, rather than DEBUG (I know it's
a DEBUG message right now, but I think the message is relevant enough to
be printed as an error).

> +
> +    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 errout;
> +    }
> +
> +    LOG(DEBUG, "asynchronously 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 errout;
> +    }
> +
> +    if (!pid) {
> +        /* child */
> +        libxl__exec(gc, aes->stdfds[0], aes->stdfds[1],
> +                    aes->stdfds[2], args[0], args, aes->env);
> +        /* notreached */
> +        abort();
> +    }
> +
> +    return 0;
> +
> +errout:

Those kind of labels are usually just called "error" inside of libxl.

Re: [PATCH] libxl: introduce asynchronous execution API [and 1 more messages]

[Ian Jackson]

Yang Hongyang writes ("[PATCH] libxl: introduce asynchronous execution API"):
> 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

Thanks, this is very good.  Exactly what I meant.

I have only some very small nits to comment on:

> +int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
> +{
...
> +    LOG(DEBUG, "asynchronously execute: %s ", aes->what);

I would write "forking to execute" in the message.


>  static void devices_remove_callback(libxl__egc *egc,
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c2b73c4..3b9acc5 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2030,6 +2030,31 @@ _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);
>  
> +/*----- asynchronous function -----*/
> +typedef struct libxl__async_exec_state libxl__async_exec_state;

I think this comment needs a blank line after it, and a better name.
I suggest "subprocess execution with timeout".

> +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 execute */

"what we're executing"

> +    char **env; /* execution environment */
> +    char **args; /* execution arguments */
> +    libxl__async_exec_callback *callback;
> +    int timeout_ms;
> +    int stdfds[3];

I would reorder these so that all the ones which are just the same as
arguments to libxl__exec are together.  And then add a comment
       /* caller must fill in; as for libxl__exec */
for that section.

Roger Pau Monné writes ("Re: [PATCH] libxl: introduce asynchronous execution API"):
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
....
> > diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> > index 1c9eb9e..6215a3d 100644
> > --- a/tools/libxl/libxl_aoutils.c
> > +++ b/tools/libxl/libxl_aoutils.c
...
> > +static void async_exec_timeout(libxl__egc *egc,
> > +                               libxl__ev_time *ev,
> > +                               const struct timeval *requested_abs)
> > +{
...
> > +    assert(libxl__ev_child_inuse(&aes->child));
> > +    LOG(DEBUG, "killing execution of %s because of timeout", aes->what);
> 
> I would make the message above an ERROR, rather than DEBUG (I know it's
> a DEBUG message right now, but I think the message is relevant enough to
> be printed as an error).

I think I agree.

> > +int libxl__async_exec_start(libxl__gc *gc, libxl__async_exec_state *aes)
> > +{
...
> > +    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 errout;
...
> > +errout:
> 
> Those kind of labels are usually just called "error" inside of libxl.

Actually, they are normally called "out".  See eg
libxl__datacopier_start, libxl__openptys, in that same file.

It should be called "out" here too.

Thanks,
Ian.