Re: [PATCH] libxl: introduce asynchronous execution API

["Roger Pau Monné" <roger.pau@citrix.com>]

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.