Re: [Xen-devel] [PATCH v6 05/13] libxl: convert libxl__device_disk_local_attach to an async op

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v6 05/13] libxl: convert 
libxl__device_disk_local_attach to an async op"):
> This will be needed in future patches, when libxl__device_disk_add
> becomes async also. Create a new status structure that defines the
> local attach of a disk device and use it in
> libxl__device_disk_local_attach.

This is looking good.  I have a couple of comments:

> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
> index 7ebc0df..182a975 100644
> --- a/tools/libxl/libxl_bootloader.c
> +++ b/tools/libxl/libxl_bootloader.c
...
> +static void bootloader_fisnihed_cb(libxl__egc *egc,
                          ^^^^^^^^
`finished'.  You have apparently managed to misspell this everywhere
you mention it!

> +                                   libxl__disk_local_state *dls,
> +                                   int rc);
> +
>  static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
>                                  int rc)
>  {
>      bootloader_cleanup(egc, bl);
> +
> +    if (bl->diskpath) {
> +        bl->dls.callback = bootloader_fisnihed_cb;
> +        libxl__device_disk_local_detach(egc, &bl->dls);
> +        return;
> +    }

Can we make libxl__device_disk_local_detach idempotent (and perhaps
call it `libxl__device_disk_local_attached_initiate_cleanup' or
something, if you can think of a name that's less than a paragraph) ?

If so then this would be simpler and wouldn't need to test
bl->diskpath.

> +static void bootloader_disk_attached_cb(libxl__egc *egc,
> +                                        libxl__disk_local_state *dls,
> +                                        int rc)
...
> +    bl->diskpath = bl->dls.diskpath;

Now that we have a bl->dls.diskpath, surely we can do away with
bl->diskpath ?  I'm not a fan of having multiple variables with the
same information in them, unless it's essential.  It just leads to
confusion and error.

> +/*
> + * Make a disk available in this (the control) domain. Calls dls->callback
> + * when finished.
> + */
> +_hidden void libxl__device_disk_local_attach(libxl__egc *egc,
> +                                             libxl__disk_local_state *dls);
> +_hidden void libxl__device_disk_local_detach(libxl__egc *egc,
> +                                             libxl__disk_local_state *dls);

You really need to explain the rules for one of these dls's.

Is it something like this:

     * A libxl__disk_local_state may be in the following states:
     *    Undefined, Idle, Attaching, Attached, Detaching

    typedef void libxl__disk_local_state_callback(libxl__egc*,
                                                  libxl__disk_local_state*,
                                                  int rc);

   _hidden void libxl__device_disk_local_attach(libxl__egc *egc,
                                                libxl__disk_local_state *dls);
       /* Undefined/Idle -> Attaching.  Will call callback.
        * On entry to callback, if rc!=0 dls is Idle;
        * if rc==0 it is Attached and dls->diskpath is usable. */

   _hidden void libxl__device_disk_local_detach(libxl__egc *egc,
                                                libxl__disk_local_state *dls);
       /* Attached -> Detaching.  Will call callback.
        * On entry to callback, dls is Idle, regardless of
        * success or failure. */

And my suggestion about idempotency would change this latter comment
to:
       /* Idle/Attached -> Detaching.  Will call callback.
        * On entry to callback, dls is Idle, regardless of
        * success or failure. */

And the bootloader code might want this:

   _hidden void libxl__device_disk_local_init(libxl__disk_local_state *dls);
       /* Undefined/Idle -> Idle */

Or you can explain it in prose if you like.

Ian.

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