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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel