Re: [Xen-devel] [PATCH V9 01/12] introduce an API to async exec scripts

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yang Hongyang writes ("[PATCH V9 01/12] introduce an API to async exec 
scripts"):
> introduce an API to async exec scripts.it will be used
> for both device and netbuffer.

Thanks.  This mostly looks plausible.

I think it would be better to combine it with the next patch.  That
way we just move/reorganise the code, rather than making a copy and
deleting the old approach.

I have some comments about details.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c2b73c4..eddafaf 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2030,6 +2030,27 @@ _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 {

We would normally call structs of this kind "libxl__something_state".
So this should be called "libxl__async_exec_state".

Your struct needs to have comments explaining which parts are to be
filled in by the caller, and which parts are private.  See
libxl__xswait_state for an example.

...
> +    void *opaque;

You shouldn't need this field.  Callers should be able to use
CONTAINER_OF to find their own state structure.

> +    void (*finish_cb)(void *opaque, int status);

Most of these kinds of setups simply name the field "callback".

> +    /* unit: second */
> +    int timeout;

The comment would be better on the same line.  However, in fact it is
better to express the units in the variable name.

And the timeout should be in milliseconds - all the timeouts in our
internal APIs are in ms.

So, "timeout_ms".

> +    bool allow_fail;

None of your callers set this to "true".  And in fact its function is
only to inhibit a log message - it makes no difference to the control
flow.  Can it be abolished ?

> +    int stdinfd;
> +    int stdoutfd;
> +    int stderrfd;

It would probably be better to make this an array, rather than 3 named
fields.  That will make it easier in the future to deal with them all
at once.

> +    libxl__ao *ao;

This field should be at the top, the way that libxl__xswait_state has
it.

> +} libxl_async_exec;
> +
> +_hidden extern int libxl_async_exec_script(libxl__gc *gc,
> +                                           libxl_async_exec *async_exec);

This function needs to be called "libxl__async_exec_start" or maybe
just "libxl__async_exec".  It doesn't seem to be limited to scripts.

It should be in libxl_exec.c, probably, or libxl_aoutils.c.

> +static void libxl_async_exec_timeout(libxl__egc *egc,
> +                                     libxl__ev_time *ev,
> +                                     const struct timeval *requested_abs)
> +{
> +    libxl_async_exec *async_exec = CONTAINER_OF(ev, *async_exec, time);

We normally give these local state variables very short names,
because we need to refer to them a lot.  If the type is changed to
libxl__async_exec_state, the local variable should be "aes".

> +    STATE_AO_GC(async_exec->ao);
> +
> +    libxl__ev_time_deregister(gc, &async_exec->time);
> +    assert(libxl__ev_child_inuse(&async_exec->child));
> +
> +    LOG(DEBUG, "killing hotplug script %s because of timeout",

You have changed the formatting here, as you move the code.  I think
the previous location of the blank line is better.

> +        async_exec->args[0]);

This still says "hotplug script" (in several places).  You probably
need to add a new field to the libxl_async_exec.  We generally seem to
use the name "what" for this - cf libxl__xswait_state.

> +static void libxl_async_exec_done(libxl__egc *egc,
> +                                  libxl__ev_child *child,
> +                                  pid_t pid, int status)

This function can be called "async_exec_done" (because it has only
internal linkage).  If you prefix it with libxl, it needs to be
libxl__.

> +    if (status && !async_exec->allow_fail) {
> +        libxl_report_child_exitstatus(CTX, LIBXL__LOG_ERROR,
> +                                      async_exec->args[0],

It's probably better to use the new "what" value rather args[0].

> +int libxl_async_exec_script(libxl__gc *gc, libxl_async_exec *async_exec)
> +{
> +    pid_t pid;
> +
> +    /* Convenience aliases */
> +    libxl__ev_child *const child = &async_exec->child;
> +    char * const *args = async_exec->args;
> +    char * const *env = async_exec->env;

Your constness doesn't seem correct here.  In the struct these
parameters are char**.  And your use of whitespace is anomalouse.
I think you want
       char **const args = aes->args;

> +    const int stdinfd = async_exec->stdinfd;
> +    const int stdoutfd = async_exec->stdoutfd;
> +    const int stderrfd = async_exec->stderrfd;

These don't buy you anything - you only use each of these once.

> +    /* Set hotplug timeout */

The word "hotplug" again.

> +    if (libxl__ev_time_register_rel(gc, &async_exec->time,
> +                                    libxl_async_exec_timeout,
> +                                    async_exec->timeout * 1000)) {
> +        LOG(ERROR, "unable to register timeout for "
> +            "script %s", args[0]);
> +        return ERROR_FAIL;
> +    }
> +
> +    LOG(DEBUG, "Calling script: %s ", args[0]);
> +    /* Fork and exec netbuf script */

The assumption of "script" again.  And a specialised comment.  Can you
make sure all of these messages use "what" as appropriate and that
messages and comments are fully general ?

Thanks,
Ian.

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