[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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],
+    }

The same comment applies as before about libxl__add_disks and the two

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,

        if (rc>  0) return;
        if (rc) {
            LOGE(ERROR, "error connecting devices");
            goto error_out;


        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 ?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.