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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

Ian Jackson wrote:
> 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:
Thanks for the review!

> > 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!
Probably copy-pasted it wrong everywhere, fixed.

> > +                                   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) ?
I'm not sure the "attached" in the name is correct, because the device 
might have failed to attach, and still we are going to call this 
function. How about "libxl__device_disk_local_initiate_detach"? (still 
quite long). Then the attach function should be renamed to 
libxl__device_disk_local_initiate_attach.
> If so then this would be simpler and wouldn't need to test
> bl->diskpath.
Ok, I've moved the test for bl->dls.diskpath to 
libxl__device_disk_local_detach, so we have a more linear flow of callbacks.
> > +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.
Done.

> > +/*
> > + * 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
Ok, I've added the init function and the description of what this 
functions do, together with the rename it should be a little bit more clear.
>      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