Re: [PATCH v9 08/17] libxl: convert libxl_device_disk_add to an async op

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

Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v9 08/17] libxl: convert libxl_device_disk_add to an async op"):
>> This patch converts libxl_device_disk_add to an ao operation that
>> waits for device backend to reach state XenbusStateInitWait and then
>> marks the operation as completed. This is not really useful now, but
>> will be used by later patches that will launch hotplug scripts after
>> we reached the desired xenbus state.
> 
> This looks pretty good.  I have just a couple of questions:
> 
>> +/* Waits for the passed device to reach state XenbusStateInitWait.
>> + * This is not really useful by itself, but is important when executing
>> + * hotplug scripts, since we need to be sure the device is in the correct
>> + * state before executing them.
>> + *
>> + * Once finished, aodev->callback will be executed.
>> + */
>> +_hidden void libxl__wait_device_connection(libxl__egc*,
>> +                                           libxl__ao_device *aodev);
> 
> This comment seems to be wrong ?  We set the callback
> to device_backend_callback but perhaps that calls aodev->callback ?

Yes, device_backend_callback is the callback for the devstate event, but
not the callback for the global aodev operation, which is stored in
aodev->callback. I think the caller doesn't need to know that internally
we set another callback, for the devstate event, since what matters is
that aodev->callback will be called when the operation is finished.

> It's hard for me to see the context without applying all of these
> patches; could you perhaps provide a public git ref too next time ?
> 
>> @@ -1808,6 +1829,8 @@ struct libxl__ao_devices {
>>   * The libxl__ao_device passed to this function should be
>>   * prepared using libxl__prepare_ao_device prior to calling
>>   * this function.
>> + *
>> + * Once finished, aodev->callback will be executed.
>>   */
>>  _hidden void libxl__initiate_device_remove(libxl__egc *egc,
>>                                             libxl__ao_device *aodev);
>> @@ -2162,6 +2185,19 @@ _hidden void libxl__destroy_domid(libxl__egc *egc,
> 
> Is this hunk in the wrong patch ?

Yes, it is.

> Ian.