Re: [PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation

[Roger Pau Monne <roger.pau@citrix.com>]

Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v4 05/10] libxl: convert libxl_device_nic_add to an async operation"):
>> This patch converts libxl_device_nic_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.
>
> Thanks.  I'm afraid this is still tripping my repetition detector.
> See below.
>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 6c9bbc2..9933cc2 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -444,6 +444,24 @@ void libxl__add_disks(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
>>       }
>>   }
>>
>> +void libxl__add_nics(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_vifs);
>> +    libxl__ao_device *aodev = *devices;
>> +    int i;
>> +
>> +    for (i = 0; i<  d_config->num_vifs; i++) {
>> +        libxl__prepare_ao_device(&aodev[i], ao, devices);
>> +        aodev[i].callback = callback;
>> +        libxl__device_nic_add(egc, domid,&d_config->vifs[i],
>> +&aodev[i]);
>> +    }
>> +}
>
> The same comment applies as before about libxl__add_disks and the two
> loops.
>
>
> But, this is another copy of libxl__add_disks.  Perhaps you want a
> macro to generate these functions ?

Yes, a macro would be ok I think.

> In general this patch seems to contain an awful lot of very similar
> code to the previous one.  Eg domcreate_nic_connected is practically
> identical to domcreate_disk_connected.  And domcreate_attach_nick is
> practically identical to domcreate_attach_disk.
>
>
> Another option would be to make the types of libxl__device_FOO_add
> compatible (by replacing the actual config parameter with const void*)
> and then pass them as function pointers.
>
> Or you could even have a table
>     { offsetof(disks, libxl_domain_config), sizeof(libxl_device_disk),
>       libxl__device_disk_add },
>     { offsetof(vifs, libxl_domain_config), sizeof(libxl_device_vif),
>       libxl__device_nic_add },
>     ...
> which your domain creation logic could iterate over.  That way you
> would do away with domcreate_FOO_connected and you'd end up with
>
>     static void domcreate_device_connected(libxl__egc *egc,
>                                       libxl__ao_device *aodev)
>     {
>
>         tableentry = devicekindstable[dcs->currentdevicekind];
>
>         rc = libxl__ao_device_check_last(gc, aodev, dcs->devices,
>                                               dcs->num_devices);
>
>         if (rc>  0) return;
>         if (rc) {
>             LOGE(ERROR, "error connecting devices");
>             goto error_out;
>         }
>
>         dcs->currentdevicekind++;
>
>         if (!devicekindstable[dcs->currentdevicekind].add_function) {
>             domcreate_complete(egc, dcs, rc);
>         } else {
>             domcreate_attach_devices(egc, dcs);
>         }
>
> or something like it.

We can really iterate over this to add the devices, since we might need 
to launch the device model between connecting the disks and connecting 
the nics, I will try to find a nicer way to do this, I will try to come 
up with some macros to define this in a nicer way.

> Or something.  Anyway, can you try to find a way to avoid me having to
> review lots of nearly-identical code ?
>
>
> Thanks,
> Ian.