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 ("[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.