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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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 ?

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.


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


Thanks,
Ian.

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