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

In the subject line `asyn' should be `async'.


> @@ -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 */
> +    libxl_device_disk_add(ctx, domid, disk, 0);

Is there going to be a later patch in a future version of the series
that fixes these ?


> +/* Macro to define callbacks of libxl__add_*, checks if device is last
> + * and calls the function passed as a parameter to this macro with the
> + * following syntax; func(libxl__egc *, parent_type *, int rc) */
> +#define DEFINE_DEVICES_CALLBACK(callback_name, parent_type, array_name, \
> +                                array_size_name, func_to_call)          \
> +    static void callback_name(libxl__egc *egc, libxl__ao_device *aodev) \
> +    {                                                                   \
> +        STATE_AO_GC(aodev->ao);                                         \
> +        parent_type *p = CONTAINER_OF(aodev->base, *p, array_name);     \
> +        int rc;                                                         \
> +                                                                        \
> +        rc = libxl__ao_device_check_last(gc, aodev, p->array_name,      \
> +                                         p->array_size_name);           \
> +        if (rc > 0) return;                                             \
> +        func_to_call(egc, p, rc);                                       \
> +    }

This is acceptable IMO and better than the repetition.  However the
use of a macro for this, so far from the invocation sites, is indeed
less than perfect.

Looking at the content of this macro, the only thing it needs from the
parent is array_name and array_size_name.  If you invented

    typedef struct libxl__ao_devices libxl__ao_devices;
    struct libxl__ao_devices {
        libxl__ao_device *array;
        int size;
        void (*callback)(libxl__egc*, libxl__ao_devices*, int rc);
    };

Then aodev->base could be of type libxl__ao_devices* and the macro
above could be a function.  And indeed it would have type
suitable for simply filling into aodev->callback.  Would that work ?

Doing this would also perhaps mean DEFINE_DEVICES_ADD's generated
libxl__add_FOOs functions could become one single function.  AFAICT
the only macroish things it does are: (1) accessing
d_config->num_FOOs, which could be passed in as a parameter;
(2) passing d_config->FOOs[i] to libxl__device_FOO_add, which could be
dealt with by making libxl__device_FOO_add take a void* for the config
instead, and passing the start of the d_config->FOOs array, plus the
element size, and the add function, as arguments.

Sorry to mess you about.


Whether you retain this as a macro or make it a function,

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 2dc1cee..1dc8247 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -585,6 +585,13 @@ DEFINE_DEVICES_CALLBACK(domcreate_disk_connected,
>                          libxl__domain_create_state,
>                          devices, num_devices, domcreate_launch_dm)
> 
> +static void domcreate_attach_pci(libxl__egc *egc,
> +                                 libxl__domain_create_state *dcs, int rc);
> +
> +DEFINE_DEVICES_CALLBACK(domcreate_nic_connected,
> +                        libxl__domain_create_state,
> +                        devices, num_devices, domcreate_attach_pci)
> +
>  static void domcreate_console_available(libxl__egc *egc,
>                                          libxl__domain_create_state *dcs);
> 

These seem not to be in linear execution order ?  These kind of
arrangements with a chain of callbacks should really have both the
declarations and the definitions in linear order.  Sadly that means
writing out the declarations of domcreate_nic_connected etc. here in
the declarations section but I think that's a price worth paying.
(This is something you'd have to do anyway if you replace the macro
with a function as I suggest.)


Ian.

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