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 ("Re: [PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn op"):
>> Ian Jackson wrote:
>>> 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.
>
> Yes.
>
>> Well the order is the following:
>>
>> libxl__device_{disk/nic}_add (device type dependant function) ->
>> libxl__initiate_device_add (device type independent).
>
> Yes.
>
>> 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".
>
> Perhaps so.
>
>> 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.
>
> But  libxl__initiate_device_remove  _is_ the function which actually
> initiates a device removal.  It isn't an internal helper for a
> family of  libxl__device_FOO_remove  functions.
>
> If it were symmetrical,  libxl__initate_device_add  would be the
> single entrypoint and it would call something kind-specific as a helper.
> That's what the comments and names seem to suggest to me, apart from
> the explicit statement that it's the other way around.
>
> Hence my feeling that it needs to be clarified and perhaps this
> function renamed.

What about libxl__initiate_device_connection?

and thanks for reviewing the code :).