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

[Roger Pau Monne <roger.pau@citrix.com>]

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.