diff mbox

[1/6] libxl: add "merge" function to generic device type support

Message ID 1468337444-24108-2-git-send-email-jgross@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jürgen Groß July 12, 2016, 3:30 p.m. UTC
Instead of using a macro generating the code to merge xenstore and
json configuration data, use the generic device type support for
this purpose.

This requires to add some accessor functions to the framework and
a structure for disks (as disks are added separately they didn't need
such a structure up to now).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl.c          | 181 ++++++++++++++++++++++++-------------------
 tools/libxl/libxl_create.c   |  18 +++--
 tools/libxl/libxl_internal.h |  52 +++++++++++--
 tools/libxl/libxl_pci.c      |   8 +-
 tools/libxl/libxl_pvusb.c    |  12 +++
 5 files changed, 181 insertions(+), 90 deletions(-)

Comments

Wei Liu July 25, 2016, 10:45 a.m. UTC | #1
On Tue, Jul 12, 2016 at 05:30:39PM +0200, Juergen Gross wrote:
> Instead of using a macro generating the code to merge xenstore and
> json configuration data, use the generic device type support for
> this purpose.
> 
> This requires to add some accessor functions to the framework and
> a structure for disks (as disks are added separately they didn't need
> such a structure up to now).
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

I definitely think this is a good idea.

The code looks good to me:

Acked-by: Wei Liu <wei.liu2@citrix.com>

I didn't do a line-by-line review though because most code looks very
mechanical. I'm confident that we can sort out issues should they arise.

Wei.
Olaf Hering Jan. 19, 2017, 4:14 p.m. UTC | #2
On Tue, Jul 12, Juergen Gross wrote:

> Instead of using a macro generating the code to merge xenstore and
> json configuration data, use the generic device type support for
> this purpose.
> This requires to add some accessor functions to the framework and
> a structure for disks (as disks are added separately they didn't need
> such a structure up to now).

> +++ b/tools/libxl/libxl.c
> @@ -7371,93 +7371,68 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,

> +            if (!dt->list || !dt->compare)
> +                continue;


This makes libxl_device_<type>_compare optional ...

> +#define DEFINE_DEVICE_TYPE_STRUCT_X(name, sname, ...)                          \
> +    const struct libxl_device_type libxl__ ## name ## _devtype = {             \
> +        .type          = #sname,                                               \
> +        .ptr_offset    = offsetof(libxl_domain_config, name ## s),             \
> +        .num_offset    = offsetof(libxl_domain_config, num_ ## name ## s),     \
> +        .dev_elem_size = sizeof(libxl_device_ ## sname),                       \
> +        .add           = libxl__add_ ## name ## s,                             \
> +        .list          = (void *(*)(libxl_ctx *, uint32_t, int *))             \
> +                         libxl_device_ ## sname ## _list,                      \
> +        .dispose       = (void (*)(void *))libxl_device_ ## sname ## _dispose, \
> +        .compare       = (int (*)(void *, void *))                             \
> +                         libxl_device_ ## sname ## _compare,                   \

... and this makes libxl_device_<type>_compare mandatory.

Which one is correct?

Olaf
Wei Liu Jan. 19, 2017, 4:19 p.m. UTC | #3
On Thu, Jan 19, 2017 at 05:14:35PM +0100, Olaf Hering wrote:
> On Tue, Jul 12, Juergen Gross wrote:
> 
> > Instead of using a macro generating the code to merge xenstore and
> > json configuration data, use the generic device type support for
> > this purpose.
> > This requires to add some accessor functions to the framework and
> > a structure for disks (as disks are added separately they didn't need
> > such a structure up to now).
> 
> > +++ b/tools/libxl/libxl.c
> > @@ -7371,93 +7371,68 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> 
> > +            if (!dt->list || !dt->compare)
> > +                continue;
> 
> 
> This makes libxl_device_<type>_compare optional ...

Actually this makes both list and compare optional.

I would say both should be mandatory -- why would you have a device type
that can't be listed or compared?

Juergen?

Wei.
Jürgen Groß Jan. 19, 2017, 4:51 p.m. UTC | #4
On 19/01/17 17:19, Wei Liu wrote:
> On Thu, Jan 19, 2017 at 05:14:35PM +0100, Olaf Hering wrote:
>> On Tue, Jul 12, Juergen Gross wrote:
>>
>>> Instead of using a macro generating the code to merge xenstore and
>>> json configuration data, use the generic device type support for
>>> this purpose.
>>> This requires to add some accessor functions to the framework and
>>> a structure for disks (as disks are added separately they didn't need
>>> such a structure up to now).
>>
>>> +++ b/tools/libxl/libxl.c
>>> @@ -7371,93 +7371,68 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>>
>>> +            if (!dt->list || !dt->compare)
>>> +                continue;
>>
>>
>> This makes libxl_device_<type>_compare optional ...
> 
> Actually this makes both list and compare optional.
> 
> I would say both should be mandatory -- why would you have a device type
> that can't be listed or compared?
> 
> Juergen?

I think above lines predate the introduction of
DEFINE_DEVICE_TYPE_STRUCT_X().

They can probably be removed.


Juergen
diff mbox

Patch

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2cf7451..07b96c7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7371,93 +7371,68 @@  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
      *    entry retrieved from xenstore while "dst" points to the entry
      *    retrieve from JSON.
      */
-#define MERGE(type, ptr, compare, merge)                                \
-    do {                                                                \
-        libxl_device_##type *p = NULL;                                  \
-        int i, j, num;                                                  \
-                                                                        \
-        p = libxl_device_##type##_list(CTX, domid, &num);               \
-        if (p == NULL) {                                                \
-            LOG(DEBUG,                                                  \
-                "no %s from xenstore for domain %d",                    \
-                #type, domid);                                          \
-        }                                                               \
-                                                                        \
-        for (i = 0; i < d_config->num_##ptr; i++) {                     \
-            libxl_device_##type *q = &d_config->ptr[i];                 \
-            for (j = 0; j < num; j++) {                                 \
-                if (compare(&p[j], q))                                  \
-                    break;                                              \
-            }                                                           \
-                                                                        \
-            if (j < num) {         /* found in xenstore */              \
-                libxl_device_##type *dst, *src;                         \
-                dst = q;                                                \
-                src = &p[j];                                            \
-                merge;                                                  \
-            } else {                /* not found in xenstore */         \
-                LOG(WARN,                                               \
-                "Device present in JSON but not in xenstore, ignored"); \
-                                                                        \
-                libxl_device_##type##_dispose(q);                       \
-                                                                        \
-                for (j = i; j < d_config->num_##ptr - 1; j++)           \
-                    memcpy(&d_config->ptr[j], &d_config->ptr[j+1],      \
-                           sizeof(libxl_device_##type));                \
-                                                                        \
-                d_config->ptr =                                         \
-                    libxl__realloc(NOGC, d_config->ptr,                 \
-                                   sizeof(libxl_device_##type) *        \
-                                   (d_config->num_##ptr - 1));          \
-                                                                        \
-                /* rewind counters */                                   \
-                d_config->num_##ptr--;                                  \
-                i--;                                                    \
-            }                                                           \
-        }                                                               \
-                                                                        \
-        for (i = 0; i < num; i++)                                       \
-            libxl_device_##type##_dispose(&p[i]);                       \
-        free(p);                                                        \
-    } while (0);
+    {
+        const struct libxl_device_type *dt;
+        int idx;
 
-    MERGE(nic, nics, COMPARE_DEVID, {});
+        for (idx = 0;; idx++) {
+            void *p = NULL;
+            void **devs;
+            int i, j, num;
+            int *num_dev;
 
-    MERGE(vtpm, vtpms, COMPARE_DEVID, {});
+            dt = device_type_tbl[idx];
+            if (!dt)
+                break;
 
-    MERGE(pci, pcidevs, COMPARE_PCI, {});
+            if (!dt->list || !dt->compare)
+                continue;
 
-    MERGE(usbctrl, usbctrls, COMPARE_USBCTRL, {});
+            num_dev = libxl__device_type_get_num(dt, d_config);
+            p = dt->list(CTX, domid, &num);
+            if (p == NULL) {
+                LOG(DEBUG, "no %s from xenstore for domain %d",
+                    dt->type, domid);
+            }
+            devs = libxl__device_type_get_ptr(dt, d_config);
+
+            for (i = 0; i < *num_dev; i++) {
+                void *q;
+
+                q = libxl__device_type_get_elem(dt, d_config, i);
+                for (j = 0; j < num; j++) {
+                    if (dt->compare(p + dt->dev_elem_size * j, q))
+                        break;
+                }
+
+                if (j < num) {         /* found in xenstore */
+                    if (dt->merge)
+                        dt->merge(ctx, p + dt->dev_elem_size * j, q);
+                } else {                /* not found in xenstore */
+                    LOG(WARN,
+                        "Device present in JSON but not in xenstore, ignored");
+
+                    dt->dispose(q);
+
+                    for (j = i; j < *num_dev - 1; j++)
+                        memcpy(libxl__device_type_get_elem(dt, d_config, j),
+                               libxl__device_type_get_elem(dt, d_config, j+1),
+                               dt->dev_elem_size);
 
-    MERGE(usbdev, usbdevs, COMPARE_USB, {});
+                    /* rewind counters */
+                    (*num_dev)--;
+                    i--;
 
-    /* Take care of removable device. We maintain invariant in the
-     * insert / remove operation so that:
-     * 1. if xenstore is "empty" while JSON is not, the result
-     *    is "empty"
-     * 2. if xenstore has a different media than JSON, use the
-     *    one in JSON
-     * 3. if xenstore and JSON have the same media, well, you
-     *    know the answer :-)
-     *
-     * Currently there is only one removable device -- CDROM.
-     * Look for libxl_cdrom_insert for reference.
-     */
-    MERGE(disk, disks, COMPARE_DISK, {
-            if (src->removable) {
-                if (!src->pdev_path || *src->pdev_path == '\0') {
-                    /* 1, no media in drive */
-                    free(dst->pdev_path);
-                    dst->pdev_path = libxl__strdup(NOGC, "");
-                    dst->format = LIBXL_DISK_FORMAT_EMPTY;
-                } else {
-                    /* 2 and 3, use JSON, no need to touch anything */
-                    ;
+                    *devs = libxl__realloc(NOGC, *devs,
+                                           dt->dev_elem_size * *num_dev);
                 }
             }
-        });
 
-#undef MERGE
+            for (i = 0; i < num; i++)
+                dt->dispose(p + dt->dev_elem_size * i);
+            free(p);
+        }
+    }
 
 out:
     if (lock) libxl__unlock_domain_userdata(lock);
@@ -7466,6 +7441,56 @@  out:
     return rc;
 }
 
+static int libxl_device_disk_compare(libxl_device_disk *d1,
+                                     libxl_device_disk *d2)
+{
+    return COMPARE_DISK(d1, d2);
+}
+
+/* Take care of removable device. We maintain invariant in the
+ * insert / remove operation so that:
+ * 1. if xenstore is "empty" while JSON is not, the result
+ *    is "empty"
+ * 2. if xenstore has a different media than JSON, use the
+ *    one in JSON
+ * 3. if xenstore and JSON have the same media, well, you
+ *    know the answer :-)
+ *
+ * Currently there is only one removable device -- CDROM.
+ * Look for libxl_cdrom_insert for reference.
+ */
+static void libxl_device_disk_merge(libxl_ctx *ctx, void *d1, void *d2)
+{
+    GC_INIT(ctx);
+    libxl_device_disk *src = d1;
+    libxl_device_disk *dst = d2;
+
+    if (src->removable) {
+        if (!src->pdev_path || *src->pdev_path == '\0') {
+            /* 1, no media in drive */
+            free(dst->pdev_path);
+            dst->pdev_path = libxl__strdup(NOGC, "");
+            dst->format = LIBXL_DISK_FORMAT_EMPTY;
+        } else {
+            /* 2 and 3, use JSON, no need to touch anything */
+            ;
+        }
+    }
+}
+
+static int libxl_device_nic_compare(libxl_device_nic *d1,
+                                    libxl_device_nic *d2)
+{
+    return COMPARE_DEVID(d1, d2);
+}
+
+static int libxl_device_vtpm_compare(libxl_device_vtpm *d1,
+                                     libxl_device_vtpm *d2)
+{
+    return COMPARE_DEVID(d1, d2);
+}
+
+DEFINE_DEVICE_TYPE_STRUCT(disk, .merge = libxl_device_disk_merge);
 DEFINE_DEVICE_TYPE_STRUCT(nic);
 DEFINE_DEVICE_TYPE_STRUCT(vtpm);
 
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 828f254..40dac1a 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1420,15 +1420,19 @@  out:
     aodev->callback(egc, aodev);
 }
 
+#define libxl_device_dtdev_list NULL
+#define libxl_device_dtdev_compare NULL
 static DEFINE_DEVICE_TYPE_STRUCT(dtdev);
 
-static const struct libxl_device_type *device_type_tbl[] = {
+const struct libxl_device_type *device_type_tbl[] = {
+    &libxl__disk_devtype,
     &libxl__nic_devtype,
     &libxl__vtpm_devtype,
     &libxl__usbctrl_devtype,
     &libxl__usbdev_devtype,
     &libxl__pcidev_devtype,
     &libxl__dtdev_devtype,
+    NULL
 };
 
 static void domcreate_attach_devices(libxl__egc *egc,
@@ -1448,9 +1452,9 @@  static void domcreate_attach_devices(libxl__egc *egc,
     }
 
     dcs->device_type_idx++;
-    if (dcs->device_type_idx < ARRAY_SIZE(device_type_tbl)) {
-        dt = device_type_tbl[dcs->device_type_idx];
-        if (*(int *)((void *)d_config + dt->num_offset) > 0) {
+    dt = device_type_tbl[dcs->device_type_idx];
+    if (dt) {
+        if (*libxl__device_type_get_num(dt, d_config) > 0) {
             /* Attach devices */
             libxl__multidev_begin(ao, &dcs->multidev);
             dcs->multidev.callback = domcreate_attach_devices;
@@ -1497,7 +1501,11 @@  static void domcreate_devmodel_started(libxl__egc *egc,
         }
     }
 
-    dcs->device_type_idx = -1;
+    /*
+     * Setting dcs->device_type_idx to 0 will skip disks, those have been
+     * already added.
+     */
+    dcs->device_type_idx = 0;
     domcreate_attach_devices(egc, &dcs->multidev, 0);
     return;
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 5347b69..1a62d6f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3441,23 +3441,63 @@  _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
 
 struct libxl_device_type {
     char *type;
-    int num_offset;   /* Offset of # of devices in libxl_domain_config */
+    int ptr_offset;    /* Offset of device array ptr in libxl_domain_config */
+    int num_offset;    /* Offset of # of devices in libxl_domain_config */
+    int dev_elem_size; /* Size of one device element in array */
     void (*add)(libxl__egc *, libxl__ao *, uint32_t, libxl_domain_config *,
                 libxl__multidev *);
+    void *(*list)(libxl_ctx *, uint32_t, int *);
+    void (*dispose)(void *);
+    int (*compare)(void *, void *);
+    void (*merge)(libxl_ctx *, void *, void *);
 };
 
-#define DEFINE_DEVICE_TYPE_STRUCT(name)                                 \
-    const struct libxl_device_type libxl__ ## name ## _devtype = {      \
-        .type       = #name,                                            \
-        .num_offset = offsetof(libxl_domain_config, num_ ## name ## s), \
-        .add        = libxl__add_ ## name ## s,                         \
+#define DEFINE_DEVICE_TYPE_STRUCT_X(name, sname, ...)                          \
+    const struct libxl_device_type libxl__ ## name ## _devtype = {             \
+        .type          = #sname,                                               \
+        .ptr_offset    = offsetof(libxl_domain_config, name ## s),             \
+        .num_offset    = offsetof(libxl_domain_config, num_ ## name ## s),     \
+        .dev_elem_size = sizeof(libxl_device_ ## sname),                       \
+        .add           = libxl__add_ ## name ## s,                             \
+        .list          = (void *(*)(libxl_ctx *, uint32_t, int *))             \
+                         libxl_device_ ## sname ## _list,                      \
+        .dispose       = (void (*)(void *))libxl_device_ ## sname ## _dispose, \
+        .compare       = (int (*)(void *, void *))                             \
+                         libxl_device_ ## sname ## _compare,                   \
+        __VA_ARGS__                                                            \
     }
 
+#define DEFINE_DEVICE_TYPE_STRUCT(name, ...)                                   \
+    DEFINE_DEVICE_TYPE_STRUCT_X(name, name, __VA_ARGS__)
+
+static inline void **libxl__device_type_get_ptr(
+    const struct libxl_device_type *dt, const libxl_domain_config *d_config)
+{
+    return (void **)((void *)d_config + dt->ptr_offset);
+}
+
+static inline void *libxl__device_type_get_elem(
+    const struct libxl_device_type *dt, const libxl_domain_config *d_config,
+    int e)
+{
+    return *libxl__device_type_get_ptr(dt, d_config) + dt->dev_elem_size * e;
+}
+
+static inline int *libxl__device_type_get_num(
+    const struct libxl_device_type *dt, const libxl_domain_config *d_config)
+{
+    return (int *)((void *)d_config + dt->num_offset);
+}
+
+extern const struct libxl_device_type libxl__disk_devtype;
 extern const struct libxl_device_type libxl__nic_devtype;
 extern const struct libxl_device_type libxl__vtpm_devtype;
 extern const struct libxl_device_type libxl__usbctrl_devtype;
 extern const struct libxl_device_type libxl__usbdev_devtype;
 extern const struct libxl_device_type libxl__pcidev_devtype;
+
+extern const struct libxl_device_type *device_type_tbl[];
+
 /*----- Domain destruction -----*/
 
 /* Domain destruction has been split into two functions:
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 9676687..22398a4 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1698,7 +1698,13 @@  int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
     return 0;
 }
 
-DEFINE_DEVICE_TYPE_STRUCT(pcidev);
+static int libxl_device_pci_compare(libxl_device_pci *d1,
+                                    libxl_device_pci *d2)
+{
+    return COMPARE_PCI(d1, d2);
+}
+
+DEFINE_DEVICE_TYPE_STRUCT_X(pcidev, pci);
 
 /*
  * Local variables:
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 41ea6bc..48a4cec 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -1675,6 +1675,18 @@  out:
     return rc;
 }
 
+static int libxl_device_usbctrl_compare(libxl_device_usbctrl *d1,
+                                        libxl_device_usbctrl *d2)
+{
+    return COMPARE_USBCTRL(d1, d2);
+}
+
+static int libxl_device_usbdev_compare(libxl_device_usbdev *d1,
+                                       libxl_device_usbdev *d2)
+{
+    return COMPARE_USB(d1, d2);
+}
+
 DEFINE_DEVICE_TYPE_STRUCT(usbctrl);
 DEFINE_DEVICE_TYPE_STRUCT(usbdev);