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

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

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.

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