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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v4 04/10] libxl: convert libxl_device_disk_add 
to an asyn op"):

Subject line needs to say `async' not `asyn'.

> 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.
> 
> As usual, libxl_device_disk_add callers have been modified, and the
> internal function libxl__device_disk_add has been used if the call was
> inside an already running ao.

This looks good to me.  I have some very minor comments.

...
> +/* Internal AO operation to connect a disk device */
> +_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);

>From the comments I'm a bit confused by the relationship between
these.  I guess libxl__device_disk_add is called by
libxl__initiate_device_add and not vice versa, and that only
libxl__initiate_device_add is allowed to call libxl__device_disk_add.
Is that true ?  If so it would be nice to say so.

> +/* Helper function to add a bunch of disks */
> +void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> +                      libxl_domain_config *d_config,
> +                      libxl__ao_device **devices,
> +                      libxl__device_callback callback);
> +

This comment should make it clear that the callback will be called
_once for each device_, and that that callback is therefore
responsible for using libxl__ao_device_check_last.

Doesn't this define `callback' as a parameter of function type?
Is that legal in C89/C99 ?

> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 5d9227e..81b467e 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -13,7 +13,7 @@ XLUMINOR = 0
> 
>  CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \
>         -Wno-declaration-after-statement -Wformat-nonliteral
> -CFLAGS += -I. -fPIC
> +CFLAGS += -I. -fPIC -O0

Stray hunk I think.

> @@ -1859,11 +1883,13 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t 
> domid, libxl_device_disk *disk)
>      ret = 0;
> 
>      libxl_device_disk_remove(ctx, domid, disks + i, 0);
> -    libxl_device_disk_add(ctx, domid, disk);
> +    /* fixme-ao */

We need to fix this in the API, at least by making libxl_cdrom_insert
take an ao_how at some point.  But not in this patch, indeed.

> +void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
> +                      libxl_domain_config *d_config,
> +                      libxl__ao_device **devices,
> +                      libxl__device_callback callback)
> +{
> +    AO_GC;
> +    GCNEW_ARRAY(*devices, d_config->num_disks);
> +    libxl__ao_device *aodev = *devices;
> +    int i;
> +
> +    for (i = 0; i < d_config->num_disks; i++) {
> +        libxl__prepare_ao_device(&aodev[i], ao, devices);
> +        aodev[i].callback = callback;
> +        libxl__device_disk_add(egc, domid, &d_config->disks[i],
> +                               &aodev[i]);
> +    }
> +}

If libxl__device_disk_add ever becomes capable of reentrantly calling
the completion callback, this will go wrong because the others haven't
been prepared yet.  Perhaps this should be in two loops ?

Ian.

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