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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
....
> > 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.

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