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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

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
> 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 ?
Not really, but I might add one.

> > +/* 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 ?
Yes, I've applied this change.

> 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.
I think this is really too complex for the benefits it introduces, 
libxl__add_{disks/nics} already takes a lot of arguments. Also take into 
account that libxl__device_disk_add is different from the rest now, 
because it takes the extra xs_transaction parameter.
> 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.)
Done.


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