Re: [PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn op

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

Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn 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 latter patches that will launch hotplug scripts after
>> we reached the desired xenbus state.
>
> And:
>
>> +/* Internal AO operation to connect a disk device, called by
>> + * libxl_device_disk_add and libxl__add_disks. This function calls
>> + * libxl__initiate_device_add */
>> +_hidden void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
>> +                                    libxl_device_disk *disk,
>> +                                    libxl__ao_device *aodev);
>> +
>> +/* Arranges that dev will be added to the guest, and the
>> + * hotplug scripts will be executed (if necessary). When
>> + * this is done (or an error happens), the callback in
>> + * aodev->callback will be called.
>> + */
>> +_hidden void libxl__initiate_device_add(libxl__egc*, libxl__ao_device *aodev);
>
> I don't think these comments are coherent.
>
> I think a function which "Arranges that dev will be added to the
> guest" is one which does everything necessary - ie, the outermost
> entrypoint.
>
> And you're using `internal' here to mean `internal to libxl'.  I think
> that's clear from the name (which has `__').  Whereas on first reading
> it sounds like you mean `internal to the device adding machinery' -
> but `libxl__device_disk_add' is the main entrypoint to that
> machinery.

I've used 'internal' here to reflex the difference between 
libxl_device_disk_add and libxl__device_disk_add, but probably leads to 
confusion.

> And `libxl__initiate_device_add' is the internal helper function.
>
> I think these comments need to be clarified and perhaps
> libxl__initiate_device_add needs to be renamed to be clear about what
> exactly it is for.  Eg
>     /* Initiates the device-kind-independent operations blah blah
>     hidden void libxl__initiate_device_add_core(libxl__egc*,
>                                     libxl__ao_device *aodev);
> or something.
>
> Please do reply suggesting alternative wording
>
> Ian.

Well the order is the following:

libxl__device_{disk/nic}_add (device type dependant function) -> 
libxl__initiate_device_add (device type independent).

I'm really bad about this naming thing. But the fact is that 
libxl__initiate_device_add is not a good name, since when we call this 
function the device is already added (to xenstore), this merely waits 
for it to reach the desired state and performs the necessary actions to 
"add" it to the guest (call hotplug scripts). Maybe "connect" is a more 
appropriate name than "add".

I liked the nomenclature because it follows the same style as 
libxl__initiate_device_remove, and I think (and I might be wrong) it 
makes things easier to understand.