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

Re: [Xen-devel] [PATCH v5 04/10] libxl: convert libxl_device_disk_add to an asyn op

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,
                          devices, num_devices, domcreate_launch_dm)

+static void domcreate_attach_pci(libxl__egc *egc,
+                                 libxl__domain_create_state *dcs, int rc);
+                        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.)


Xen-devel mailing list



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