Re: [PATCH v11 03/17] 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 v11 03/17] 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.
> 
> Thanks.  Close...
> 
>> +void libxl__device_disk_local_initiate_detach(libxl__egc *egc,
>> +                                     libxl__disk_local_state *dls)
>>  {
>> +    STATE_AO_GC(dls->ao);
>>      int rc = 0;
>> +    libxl_device_disk *disk = &dls->disk;
>> +    libxl__device *device;
>> +    libxl__ao_device *aodev = &dls->aodev;
>> +    libxl__prepare_ao_device(ao, aodev);
>> +
>> +    if (!dls->diskpath) goto out;
>>
>>      switch (disk->backend) {
>>          case LIBXL_DISK_BACKEND_QDISK:
> ...
>> +                rc = libxl__device_from_disk(gc, LIBXL_TOOLSTACK_DOMID,
>> +                                             disk, device);
>> +                if (rc != 0) goto out;
> ...
>>          default:
>>              /*
>>               * Nothing to do for PHYSTYPE_PHY.
>>               * For other device types assume that the blktap2 process is
>>               * needed by the soon to be started domain and do nothing.
>>               */
>> -            break;
>> +            goto out;
>>      }
>>
>> +out:
>> +    local_device_detach_cb(egc, aodev);
> 
> Doesn't this lose the rc ?  Since...
> 
> ...
>> +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 (aodev->rc) {
> 
> ... this picks the rc out of the aodev.  And you don't set aodev->rc.
> The general code in libxl__device_disk_local_initiate_detach expects
> (as is conventional) to set the local variable rc and `goto out'.  So
> I think you need
>   +    aodev->rc = rc;

Yes, that's right.

> 
>> @@ -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. */
> 
> I queried this and you replied:
>> 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.
> 
> Right.  The comment is wrong, but you're not making it wronger, so
> this is something for me to fix up.

Thanks for the review, and for taking care of this last comment.