Re: [PATCH v10 07/19] 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 v10 07/19] 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 done in this patch to split the changes introduced when
>> libxl__device_disk_add becomes async.
> 
> Thanks for this.  See my other comment earlier today about the error
> handling.  The rest of my comments are below.  Very nearly there I
> think.
> 
> Thanks,
> Ian.
> 
>> @@ -2234,39 +2237,102 @@ char * libxl__device_disk_local_attach(libxl__gc *gc,
>>              goto out;
>>      }
>>      if (dev != NULL)
>> -        ret = strdup(dev);
>> -    return ret;
>> +        dls->diskpath = strdup(dev);
> 
> libxl__strdup, surely.  And I'm not sure I understand why this string
> can't be from the gc.  In any case the memory allocation strategy
> should be documented in the struct.  Eg:
> 
>> +struct libxl__disk_local_state {
> ...
>> +    /* filled by libxl__device_disk_local_initiate_attach */
>> +    char *diskpath;
> 
>   +    char *diskpath; /* from the gc */
> or
>   +    char *diskpath; /* non-0 whenever Attached, disposed by detach */
> or something.

I've changed diskpath to be allocated from the gc.

>> +void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>> +                                     libxl__disk_local_state *dls)
>>  {
> ...
>> +out:
>> +    if (dls->diskpath) {
>> +        free(dls->diskpath);
>> +        dls->diskpath = 0;
>> +    }
>> +    /*
>> +     * If there was an error in dls->rc, it means we have been called from
>> +     * a failed execution of libxl__device_disk_local_initiate_attach,
>> +     * so return the original error.
>> +     */
>> +    rc = dls->rc ? dls->rc : rc;
>> +    dls->callback(egc, dls, rc);
>> +    return;
> 
> This seems to occur twice.  Once here and once in
> local_device_detach_cb.  Clearly they should be unified, probably by
> dismembering local_device_detach_cb:

I'm calling local_device_detach_cb instead of the callback directly.

> ...
>> +static void local_device_detach_cb(libxl__egc *egc, libxl__ao_device *aodev)
>> +{
>> +    STATE_AO_GC(aodev->ao);
>> +    libxl__disk_local_state *dls = CONTAINER_OF(aodev, *dls, aodev);
>> +    int rc;
>> +
>> +    if (dls->diskpath) {
>> +        free(dls->diskpath);
>> +        dls->diskpath = 0;
>> +    }
>> +
>> +    if (aodev->rc) {
> ...
>> +    }
>> +
>> +out:
>> +    /*
>> +     * If there was an error in dls->rc, it means we have been called from
>> +     * a failed execution of libxl__device_disk_local_initiate_attach,
>> +     * so return the original error.
>> +     */
>> +    rc = dls->rc ? dls->rc : aodev->rc;
>> +    dls->callback(egc, dls, rc);
>> +    return;
> 
> 
>> diff --git a/tools/libxl/libxl_bootloader.c b/tools/libxl/libxl_bootloader.c
>> index 7ebc0df..4bcca3d 100644
>> --- a/tools/libxl/libxl_bootloader.c
>> +++ b/tools/libxl/libxl_bootloader.c
>> @@ -249,10 +245,32 @@ static void bootloader_setpaths(libxl__gc *gc, libxl__bootloader_state *bl)
> ...
>> +/* Callbacks */
>> +
>> +static void bootloader_finished_cb(libxl__egc *egc,
>> +                                   libxl__disk_local_state *dls,
>> +                                   int rc);
> 
> Wouldn't this be better named
>    bootloader_local_detached_cb
> or something ?  Because the function called when the bootloader
> finishes is bootloader_callback.

Yes, changed to bootloader_local_detached_cb.

>>  static void bootloader_callback(libxl__egc *egc, libxl__bootloader_state *bl,
>>                                  int rc)
>>  {
>>      bootloader_cleanup(egc, bl);
>> +
>> +    bl->dls.callback = bootloader_finished_cb;
>> +    libxl__device_disk_local_initiate_detach(egc, &bl->dls);
>> +}
> 
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 4f3a232..ab590be 100644
> ...
>> +/*
>> + * Clears the internal error code, can be called multiple times, and
>> + * must be called before libxl__device_disk_local_initiate_attach.
>> + */
>> +static inline void libxl__device_disk_local_init(libxl__disk_local_state *dls)
> 
> "Clears the internal error code" should not be in interface
> documentation, since it refers to an implementation detail.
> 
> Perhaps you mean "Prepares a dls for use".  You should explicitly
> state which of the documented states it transitions between.  I guess
> "Undefined -> Idle" ?

Changed the comment, and added the state transition.

>> +/* Make a disk available in this (the control) domain. Always calls
>> + * dls->callback when finished.
>> + * State Idle -> Attaching
>> + *
>> + * The state on which the device is when in the callback depends
>> + * on the passed value of rc:
>> + * Attached if rc == 0
>> + * Idle if rc != 0
> 
> A nit: this would be slightly easier to read if there the two
> subordinate options were indented.
> 
> Also correct English would be "the state in which the device is" but
> really you mean the state of the dls, not of the device.  I would
> write:
>   + * The state of dls on entry to the callback depends on the value
>   + * of rc passed to the callback:
>   + *     rc == 0: Attached if rc == 0
>   + *     rc != 0: Idle
> 
>> +_hidden void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
>> +                                                libxl__disk_local_state *dls);
>> +
>> +/* Disconnects a disk device form the control domain. If the passed
>> + * dls is not attached (or has already been detached),
>> + * libxl__device_disk_local_initiate_detach will just call the callback
>> + * directly.
>> + * State Idle/Attached -> Detaching
>> + *
>> + * The state on which the device is when in the callback is Idle.
> 
> Again, "in which", "dls" or reword as above.

I've addressed both comments.

>> @@ -2097,10 +2142,10 @@ struct libxl__bootloader_state {
>>      /* Should be zeroed by caller on entry.  Will be filled in by
>>       * bootloader machinery; represents the local attachment of the
>>       * disk for the benefit of the bootloader.  Must be detached by
>> -     * the caller using libxl__device_disk_local_detach, but only
>> +     * the caller using libxl__device_disk_local_initiate_detach, but only
>>       * after the domain's kernel and initramfs have been loaded into
>>       * memory and the file references disposed of. */
> 
> Is this last comment, which I wrote, wrong ?  That is, as previously
> discussed, is it legal to detach the disk before the kernel and
> initramfs have been loaded into the domain's memory ?
> 
> I think it probably is.  If so perhaps I should write a separate
> patch to fix it.

I didn't change this comment, but if you want I can do that in this
patch. The bootloader makes a copy of the kernel/initramfs to somewhere,
and that is what we load to the memory.