[PATCH 0/3] libxl: add framework for device types

[PATCH 0/3] libxl: add framework for device types

[Juergen Gross]

Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
especially on domain creation introduce a framework for that purpose.

I especially found it annoying that e.g. the vtpm callback issued the
error message for a failed attach of nic devices.

Juergen Gross (3):
  libxl: add framework for device types
  libxl: refactor domcreate_attach_pci() to use device type framework
  libxl: refactor domcreate_attach_dtdev() to use device type framework

 tools/libxl/libxl.c          |  12 ++
 tools/libxl/libxl_create.c   | 275 +++++++++++++------------------------------
 tools/libxl/libxl_internal.h |  14 +++
 tools/libxl/libxl_pci.c      |  36 ++++++
 tools/libxl/libxl_pvusb.c    |  13 ++
 5 files changed, 159 insertions(+), 191 deletions(-)

-- 
2.6.6


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

[PATCH 1/3] libxl: add framework for device types

[Juergen Gross]

Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
especially on domain creation introduce a framework for that purpose.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl.c          |  12 ++++
 tools/libxl/libxl_create.c   | 163 +++++++++++++------------------------------
 tools/libxl/libxl_internal.h |  13 ++++
 tools/libxl/libxl_pvusb.c    |  13 ++++
 4 files changed, 87 insertions(+), 114 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 1c81239..df94f2e 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7434,6 +7434,18 @@ out:
     return rc;
 }
 
+struct libxl_device_type libxl__nic_devtype = {
+    .type       = "nic",
+    .num_offset = offsetof(libxl_domain_config, num_nics),
+    .add        = libxl__add_nics,
+};
+
+struct libxl_device_type libxl__vtpm_devtype = {
+    .type       = "vtpm",
+    .num_offset = offsetof(libxl_domain_config, num_vtpms),
+    .add        = libxl__add_vtpms,
+};
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1b99472..bb0f535 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -742,12 +742,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
 static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
                                 int ret);
 
-static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *multidev,
-                                   int ret);
-static void domcreate_attach_usbctrls(libxl__egc *egc,
-                                      libxl__multidev *multidev, int ret);
-static void domcreate_attach_usbdevs(libxl__egc *egc, libxl__multidev *multidev,
-                                     int ret);
 static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
                                  int ret);
 static void domcreate_attach_dtdev(libxl__egc *egc,
@@ -1407,6 +1401,53 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     domcreate_complete(egc, dcs, ret);
 }
 
+static struct libxl_device_type *device_type_tbl[] = {
+    &libxl__nic_devtype,
+    &libxl__vtpm_devtype,
+    &libxl__usbctrl_devtype,
+    &libxl__usbdev_devtype,
+};
+
+static void domcreate_attach_devices(libxl__egc *egc,
+                                     libxl__multidev *multidev,
+                                     int ret)
+{
+    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
+    STATE_AO_GC(dcs->ao);
+    int domid = dcs->guest_domid;
+    libxl_domain_config *const d_config = dcs->guest_config;
+    struct libxl_device_type *dt;
+
+    if (ret) {
+        LOG(ERROR, "unable to add %s devices",
+            device_type_tbl[dcs->device_type_idx]->type);
+        goto error_out;
+    }
+
+    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) {
+            /* Attach devices */
+            libxl__multidev_begin(ao, &dcs->multidev);
+            dcs->multidev.callback = domcreate_attach_devices;
+            dt->add(egc, ao, domid, d_config, &dcs->multidev);
+            libxl__multidev_prepared(egc, &dcs->multidev, 0);
+            return;
+        }
+
+        domcreate_attach_devices(egc, &dcs->multidev, 0);
+        return;
+    }
+
+    domcreate_attach_pci(egc, multidev, 0);
+    return;
+
+error_out:
+    assert(ret);
+    domcreate_complete(egc, dcs, ret);
+}
+
 static void domcreate_devmodel_started(libxl__egc *egc,
                                        libxl__dm_spawn_state *dmss,
                                        int ret)
@@ -1430,113 +1471,8 @@ static void domcreate_devmodel_started(libxl__egc *egc,
         }
     }
 
-    /* Plug nic interfaces */
-    if (d_config->num_nics > 0) {
-        /* Attach nics */
-        libxl__multidev_begin(ao, &dcs->multidev);
-        dcs->multidev.callback = domcreate_attach_vtpms;
-        libxl__add_nics(egc, ao, domid, d_config, &dcs->multidev);
-        libxl__multidev_prepared(egc, &dcs->multidev, 0);
-        return;
-    }
-
-    domcreate_attach_vtpms(egc, &dcs->multidev, 0);
-    return;
-
-error_out:
-    assert(ret);
-    domcreate_complete(egc, dcs, ret);
-}
-
-static void domcreate_attach_vtpms(libxl__egc *egc,
-                                   libxl__multidev *multidev,
-                                   int ret)
-{
-   libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
-   STATE_AO_GC(dcs->ao);
-   int domid = dcs->guest_domid;
-
-   libxl_domain_config* const d_config = dcs->guest_config;
-
-   if(ret) {
-       LOG(ERROR, "unable to add nic devices");
-       goto error_out;
-   }
-
-    /* Plug vtpm devices */
-   if (d_config->num_vtpms > 0) {
-       /* Attach vtpms */
-       libxl__multidev_begin(ao, &dcs->multidev);
-       dcs->multidev.callback = domcreate_attach_usbctrls;
-       libxl__add_vtpms(egc, ao, domid, d_config, &dcs->multidev);
-       libxl__multidev_prepared(egc, &dcs->multidev, 0);
-       return;
-   }
-
-   domcreate_attach_usbctrls(egc, multidev, 0);
-   return;
-
-error_out:
-   assert(ret);
-   domcreate_complete(egc, dcs, ret);
-}
-
-static void domcreate_attach_usbctrls(libxl__egc *egc,
-                                      libxl__multidev *multidev, int ret)
-{
-    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
-    STATE_AO_GC(dcs->ao);
-    int domid = dcs->guest_domid;
-
-    libxl_domain_config *const d_config = dcs->guest_config;
-
-    if (ret) {
-        LOG(ERROR, "unable to add vtpm devices");
-        goto error_out;
-    }
-
-    if (d_config->num_usbctrls > 0) {
-        /* Attach usbctrls */
-        libxl__multidev_begin(ao, &dcs->multidev);
-        dcs->multidev.callback = domcreate_attach_usbdevs;
-        libxl__add_usbctrls(egc, ao, domid, d_config, &dcs->multidev);
-        libxl__multidev_prepared(egc, &dcs->multidev, 0);
-        return;
-    }
-
-    domcreate_attach_usbdevs(egc, multidev, 0);
-    return;
-
-error_out:
-    assert(ret);
-    domcreate_complete(egc, dcs, ret);
-}
-
-
-static void domcreate_attach_usbdevs(libxl__egc *egc, libxl__multidev *multidev,
-                                int ret)
-{
-    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
-    STATE_AO_GC(dcs->ao);
-    int domid = dcs->guest_domid;
-
-    libxl_domain_config *const d_config = dcs->guest_config;
-
-    if (ret) {
-        LOG(ERROR, "unable to add usbctrl devices");
-        goto error_out;
-    }
-
-    if (d_config->num_usbdevs > 0) {
-        /* Attach usbctrls */
-        libxl__multidev_begin(ao, &dcs->multidev);
-        dcs->multidev.callback = domcreate_attach_pci;
-        libxl__add_usbdevs(egc, ao, domid, d_config, &dcs->multidev);
-        libxl__multidev_prepared(egc, &dcs->multidev, 0);
-        return;
-    }
-
-    domcreate_attach_pci(egc, multidev, 0);
+    dcs->device_type_idx = -1;
+    domcreate_attach_devices(egc, &dcs->multidev, 0);
     return;
 
 error_out:
@@ -1556,7 +1492,6 @@ static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
     libxl_domain_config *const d_config = dcs->guest_config;
 
     if (ret) {
-        LOG(ERROR, "unable to add usb devices");
         goto error_out;
     }
 
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e7ab85d..2603b33 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3389,6 +3389,18 @@ _hidden void libxl__bootloader_init(libxl__bootloader_state *bl);
  * If callback is passed rc==0, will have updated st->info appropriately */
 _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st);
 
+/*----- Generic Device Handling -----*/
+struct libxl_device_type {
+    char *type;
+    int num_offset;   /* Offset of # of devices in libxl_domain_config */
+    void (*add)(libxl__egc *, libxl__ao *, uint32_t, libxl_domain_config *,
+                libxl__multidev *);
+};
+
+extern struct libxl_device_type libxl__nic_devtype;
+extern struct libxl_device_type libxl__vtpm_devtype;
+extern struct libxl_device_type libxl__usbctrl_devtype;
+extern struct libxl_device_type libxl__usbdev_devtype;
 /*----- Domain destruction -----*/
 
 /* Domain destruction has been split into two functions:
@@ -3565,6 +3577,7 @@ struct libxl__domain_create_state {
     libxl_asyncprogress_how aop_console_how;
     /* private to domain_create */
     int guest_domid;
+    int device_type_idx;
     const char *colo_proxy_script;
     libxl__domain_build_state build_state;
     libxl__colo_restore_state crs;
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 885f0d4..31c38b9 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -1667,6 +1667,19 @@ out:
     GC_FREE;
     return rc;
 }
+
+struct libxl_device_type libxl__usbctrl_devtype = {
+    .type       = "usbctrl",
+    .num_offset = offsetof(libxl_domain_config, num_usbctrls),
+    .add        = libxl__add_usbctrls,
+};
+
+struct libxl_device_type libxl__usbdev_devtype = {
+    .type       = "usbdev",
+    .num_offset = offsetof(libxl_domain_config, num_usbdevs),
+    .add        = libxl__add_usbdevs,
+};
+
 /*
  * Local variables:
  * mode: C
-- 
2.6.6


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

[PATCH 2/3] libxl: refactor domcreate_attach_pci() to use device type framework

[Juergen Gross]

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_create.c   | 54 ++++++--------------------------------------
 tools/libxl/libxl_internal.h |  1 +
 tools/libxl/libxl_pci.c      | 36 +++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 47 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index bb0f535..c4e85f0 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -742,10 +742,8 @@ static void domcreate_bootloader_done(libxl__egc *egc,
 static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
                                 int ret);
 
-static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
-                                 int ret);
-static void domcreate_attach_dtdev(libxl__egc *egc,
-                                   libxl__domain_create_state *dcs);
+static void domcreate_attach_dtdev(libxl__egc *egc, libxl__multidev *multidev,
+                                   int ret);
 
 static void domcreate_console_available(libxl__egc *egc,
                                         libxl__domain_create_state *dcs);
@@ -1406,6 +1404,7 @@ static struct libxl_device_type *device_type_tbl[] = {
     &libxl__vtpm_devtype,
     &libxl__usbctrl_devtype,
     &libxl__usbdev_devtype,
+    &libxl__pci_devtype,
 };
 
 static void domcreate_attach_devices(libxl__egc *egc,
@@ -1440,7 +1439,7 @@ static void domcreate_attach_devices(libxl__egc *egc,
         return;
     }
 
-    domcreate_attach_pci(egc, multidev, 0);
+    domcreate_attach_dtdev(egc, multidev, 0);
     return;
 
 error_out:
@@ -1480,52 +1479,13 @@ error_out:
     domcreate_complete(egc, dcs, ret);
 }
 
-static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
-                                 int ret)
-{
-    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
-    STATE_AO_GC(dcs->ao);
-    int i;
-    int domid = dcs->guest_domid;
-
-    /* convenience aliases */
-    libxl_domain_config *const d_config = dcs->guest_config;
-
-    if (ret) {
-        goto error_out;
-    }
-
-    for (i = 0; i < d_config->num_pcidevs; i++) {
-        ret = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
-        if (ret < 0) {
-            LOG(ERROR, "libxl_device_pci_add failed: %d", ret);
-            goto error_out;
-        }
-    }
-
-    if (d_config->num_pcidevs > 0) {
-        ret = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
-            d_config->num_pcidevs);
-        if (ret < 0) {
-            LOG(ERROR, "libxl_create_pci_backend failed: %d", ret);
-            goto error_out;
-        }
-    }
-
-    domcreate_attach_dtdev(egc, dcs);
-    return;
-
-error_out:
-    assert(ret);
-    domcreate_complete(egc, dcs, ret);
-}
-
 static void domcreate_attach_dtdev(libxl__egc *egc,
-                                   libxl__domain_create_state *dcs)
+                                   libxl__multidev *multidev,
+                                   int ret)
 {
+    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
     STATE_AO_GC(dcs->ao);
     int i;
-    int ret;
     int domid = dcs->guest_domid;
 
     /* convenience aliases */
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 2603b33..547a78b 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3401,6 +3401,7 @@ extern struct libxl_device_type libxl__nic_devtype;
 extern struct libxl_device_type libxl__vtpm_devtype;
 extern struct libxl_device_type libxl__usbctrl_devtype;
 extern struct libxl_device_type libxl__usbdev_devtype;
+extern struct libxl_device_type libxl__pci_devtype;
 /*----- 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 236bdd0..fd245ea 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1291,6 +1291,36 @@ out:
     return rc;
 }
 
+static void libxl__add_pcis(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+                            libxl_domain_config *d_config,
+                            libxl__multidev *multidev)
+{
+    AO_GC;
+    libxl__ao_device *aodev = libxl__multidev_prepare(multidev);
+    int i, rc = 0;
+
+    for (i = 0; i < d_config->num_pcidevs; i++) {
+        rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
+        if (rc < 0) {
+            LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
+            goto out;
+        }
+    }
+
+    if (d_config->num_pcidevs > 0) {
+        rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
+            d_config->num_pcidevs);
+        if (rc < 0) {
+            LOG(ERROR, "libxl_create_pci_backend failed: %d", rc);
+            goto out;
+        }
+    }
+
+out:
+    aodev->rc = rc;
+    aodev->callback(egc, aodev);
+}
+
 static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
                                     libxl_device_pci *pcidev, int force)
 {
@@ -1668,6 +1698,12 @@ int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
     return 0;
 }
 
+struct libxl_device_type libxl__pci_devtype = {
+    .type       = "pci",
+    .num_offset = offsetof(libxl_domain_config, num_pcidevs),
+    .add        = libxl__add_pcis,
+};
+
 /*
  * Local variables:
  * mode: C
-- 
2.6.6


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

[PATCH 3/3] libxl: refactor domcreate_attach_dtdev() to use device type framework

[Juergen Gross]

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 tools/libxl/libxl_create.c | 72 ++++++++++++++++++++++------------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c4e85f0..e93b880 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -742,9 +742,6 @@ static void domcreate_bootloader_done(libxl__egc *egc,
 static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *aodevs,
                                 int ret);
 
-static void domcreate_attach_dtdev(libxl__egc *egc, libxl__multidev *multidev,
-                                   int ret);
-
 static void domcreate_console_available(libxl__egc *egc,
                                         libxl__domain_create_state *dcs);
 
@@ -1399,12 +1396,43 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     domcreate_complete(egc, dcs, ret);
 }
 
+static void libxl__add_dtdevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
+                              libxl_domain_config *d_config,
+                              libxl__multidev *multidev)
+{
+    AO_GC;
+    libxl__ao_device *aodev = libxl__multidev_prepare(multidev);
+    int i, rc = 0;
+
+    for (i = 0; i < d_config->num_dtdevs; i++) {
+        const libxl_device_dtdev *dtdev = &d_config->dtdevs[i];
+
+        LOG(DEBUG, "Assign device \"%s\" to dom%u", dtdev->path, domid);
+        rc = xc_assign_dt_device(CTX->xch, domid, dtdev->path);
+        if (rc < 0) {
+            LOG(ERROR, "xc_assign_dtdevice failed: %d", rc);
+            goto out;
+        }
+    }
+
+out:
+    aodev->rc = rc;
+    aodev->callback(egc, aodev);
+}
+
+static struct libxl_device_type libxl__dtdev_devtype = {
+    .type       = "dtdev",
+    .num_offset = offsetof(libxl_domain_config, num_dtdevs),
+    .add        = libxl__add_dtdevs,
+};
+
 static struct libxl_device_type *device_type_tbl[] = {
     &libxl__nic_devtype,
     &libxl__vtpm_devtype,
     &libxl__usbctrl_devtype,
     &libxl__usbdev_devtype,
     &libxl__pci_devtype,
+    &libxl__dtdev_devtype,
 };
 
 static void domcreate_attach_devices(libxl__egc *egc,
@@ -1439,7 +1467,10 @@ static void domcreate_attach_devices(libxl__egc *egc,
         return;
     }
 
-    domcreate_attach_dtdev(egc, multidev, 0);
+    domcreate_console_available(egc, dcs);
+
+    domcreate_complete(egc, dcs, 0);
+
     return;
 
 error_out:
@@ -1479,39 +1510,6 @@ error_out:
     domcreate_complete(egc, dcs, ret);
 }
 
-static void domcreate_attach_dtdev(libxl__egc *egc,
-                                   libxl__multidev *multidev,
-                                   int ret)
-{
-    libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
-    STATE_AO_GC(dcs->ao);
-    int i;
-    int domid = dcs->guest_domid;
-
-    /* convenience aliases */
-    libxl_domain_config *const d_config = dcs->guest_config;
-
-    for (i = 0; i < d_config->num_dtdevs; i++) {
-        const libxl_device_dtdev *dtdev = &d_config->dtdevs[i];
-
-        LOG(DEBUG, "Assign device \"%s\" to dom%u", dtdev->path, domid);
-        ret = xc_assign_dt_device(CTX->xch, domid, dtdev->path);
-        if (ret < 0) {
-            LOG(ERROR, "xc_assign_dtdevice failed: %d", ret);
-            goto error_out;
-        }
-    }
-
-    domcreate_console_available(egc, dcs);
-
-    domcreate_complete(egc, dcs, 0);
-    return;
-
-error_out:
-    assert(ret);
-    domcreate_complete(egc, dcs, ret);
-}
-
 static void domcreate_complete(libxl__egc *egc,
                                libxl__domain_create_state *dcs,
                                int rc)
-- 
2.6.6


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

Re: [PATCH 0/3] libxl: add framework for device types

[Juergen Gross]

On 21/06/16 16:24, Juergen Gross wrote:
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.
> 
> I especially found it annoying that e.g. the vtpm callback issued the
> error message for a failed attach of nic devices.
> 
> Juergen Gross (3):
>   libxl: add framework for device types
>   libxl: refactor domcreate_attach_pci() to use device type framework
>   libxl: refactor domcreate_attach_dtdev() to use device type framework
> 
>  tools/libxl/libxl.c          |  12 ++
>  tools/libxl/libxl_create.c   | 275 +++++++++++++------------------------------
>  tools/libxl/libxl_internal.h |  14 +++
>  tools/libxl/libxl_pci.c      |  36 ++++++
>  tools/libxl/libxl_pvusb.c    |  13 ++
>  5 files changed, 159 insertions(+), 191 deletions(-)
> 

Ping?


Juergen

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

Re: [PATCH 0/3] libxl: add framework for device types

[Ian Jackson]

Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"):
> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
> especially on domain creation introduce a framework for that purpose.

I think something like this is a jolly good idea.  Thanks a lot!

The rough shape - a set of structs with what amount to method calls -
seems a good direction to be going in.

I have some comments/questions.

I saw this in 2/3:

> +    for (i = 0; i < d_config->num_pcidevs; i++) {
> +        rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> +        if (rc < 0) {
> +            LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> +            goto out;
> +        }
> +    }
> +

And there is similar code in 3/3 for dtdevs.  Could that be lifted
away somehow ?  (You'd have to take some care about the types, sadly;
ie, I think libxl__device_pci_add might have to start to take a
void*; maybe some macros could make things typesafe?)


In 1/3:

> +struct libxl_device_type libxl__usbctrl_devtype = {
> +    .type       = "usbctrl",
> +    .num_offset = offsetof(libxl_domain_config, num_usbctrls),
> +    .add        = libxl__add_usbctrls,
> +};

And then num_offset is used like this:

> +        if (*(int *)((void *)d_config + dt->num_offset) > 0) {

This is a fine approach but I would prefer it if the there were a bit
more type safety, particularly in the parts that have to occur once
for each device type.

For example, there is nothing stopping one using this pattern with a
num_* field which is not an int.

Perhaps there should be a macro for generating the libxl_device_type
contents ?  It could perhaps take `usbctrls' and make `num_usbctrls'
out of it using token pasting.


Also these structs should be static const, I think.

Thanks again!

Ian.

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

Re: [PATCH 0/3] libxl: add framework for device types

[Juergen Gross]

On 06/07/16 13:04, Ian Jackson wrote:
> Juergen Gross writes ("[PATCH 0/3] libxl: add framework for device types"):
>> Instead of duplicate coding for each device type (vtpms, usbctrls, ...)
>> especially on domain creation introduce a framework for that purpose.
> 
> I think something like this is a jolly good idea.  Thanks a lot!
> 
> The rough shape - a set of structs with what amount to method calls -
> seems a good direction to be going in.
> 
> I have some comments/questions.
> 
> I saw this in 2/3:
> 
>> +    for (i = 0; i < d_config->num_pcidevs; i++) {
>> +        rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
>> +        if (rc < 0) {
>> +            LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
>> +            goto out;
>> +        }
>> +    }
>> +
> 
> And there is similar code in 3/3 for dtdevs.  Could that be lifted
> away somehow ?  (You'd have to take some care about the types, sadly;
> ie, I think libxl__device_pci_add might have to start to take a
> void*; maybe some macros could make things typesafe?)

I thought about this idea already. I think we would end up with more
code which would be rather unpleasant to read. Main reason is the
need for a dtdev wrapper function and the pci backend creation.

> In 1/3:
> 
>> +struct libxl_device_type libxl__usbctrl_devtype = {
>> +    .type       = "usbctrl",
>> +    .num_offset = offsetof(libxl_domain_config, num_usbctrls),
>> +    .add        = libxl__add_usbctrls,
>> +};
> 
> And then num_offset is used like this:
> 
>> +        if (*(int *)((void *)d_config + dt->num_offset) > 0) {
> 
> This is a fine approach but I would prefer it if the there were a bit
> more type safety, particularly in the parts that have to occur once
> for each device type.
> 
> For example, there is nothing stopping one using this pattern with a
> num_* field which is not an int.
> 
> Perhaps there should be a macro for generating the libxl_device_type
> contents ?  It could perhaps take `usbctrls' and make `num_usbctrls'
> out of it using token pasting.

I think 'usbctrl' as the macro parameter would be just fine: it would
allow to generate the complete struct:

#define DEFINE_DEVICE_STRUCT(name) \
  const libxl_device_type libxl__ ## name ## _devtype = { \
    .type       = # name , \
    .num_offset = offsetof(libxl_domain_config, num_ ## name ## s), \
    .add        = libxl__add_ ## name ## s, \
}

DEFINE_DEVICE_STRUCT(usbctrl);

> Also these structs should be static const, I think.

static: sometimes, const: yes.


Juergen

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

Re: [PATCH 0/3] libxl: add framework for device types

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
> On 06/07/16 13:04, Ian Jackson wrote:
> >> +    for (i = 0; i < d_config->num_pcidevs; i++) {
> >> +        rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> >> +        if (rc < 0) {
> >> +            LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> >> +            goto out;
> >> +        }
> >> +    }
> >> +
> > 
> > And there is similar code in 3/3 for dtdevs.  Could that be lifted
> > away somehow ?  (You'd have to take some care about the types, sadly;
> > ie, I think libxl__device_pci_add might have to start to take a
> > void*; maybe some macros could make things typesafe?)
> 
> I thought about this idea already. I think we would end up with more
> code which would be rather unpleasant to read. Main reason is the
> need for a dtdev wrapper function and the pci backend creation.

I'm not sure what you mean by dtdev wrapper function.

As for pci backend, there could be a separate hook for "after adding
all devices of this type".

But if you don't think this is feasible I won't insist on it.  The
approach you have is already a big improvement.

Ian.

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

Re: [PATCH 0/3] libxl: add framework for device types

[Juergen Gross]

On 06/07/16 14:47, Ian Jackson wrote:
> Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
>> On 06/07/16 13:04, Ian Jackson wrote:
>>>> +    for (i = 0; i < d_config->num_pcidevs; i++) {
>>>> +        rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
>>>> +        if (rc < 0) {
>>>> +            LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
>>>> +            goto out;
>>>> +        }
>>>> +    }
>>>> +
>>>
>>> And there is similar code in 3/3 for dtdevs.  Could that be lifted
>>> away somehow ?  (You'd have to take some care about the types, sadly;
>>> ie, I think libxl__device_pci_add might have to start to take a
>>> void*; maybe some macros could make things typesafe?)
>>
>> I thought about this idea already. I think we would end up with more
>> code which would be rather unpleasant to read. Main reason is the
>> need for a dtdev wrapper function and the pci backend creation.
> 
> I'm not sure what you mean by dtdev wrapper function.

The loop for dtdev calls a xc_ function with different parameters than
the one for pci. We'd need a libxl__device_dtdev_add() wrapper function
to do the xc_ call.

> As for pci backend, there could be a separate hook for "after adding
> all devices of this type".

Right. And summing up the additional hooks and queries whether they
are specified sums up to more overhead than the current version.

> But if you don't think this is feasible I won't insist on it.  The
> approach you have is already a big improvement.

Thanks.

I'm planning to add more to it (e.g. I'd like to get rid of the
MERGE() macro in libxl_retrieve_domain_configuration() in favor of a
device type hook in order to be able to have _all_ stuff for one type
in one source file.


Juergen

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

Re: [PATCH 0/3] libxl: add framework for device types

[Ian Jackson]

Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
> On 06/07/16 14:47, Ian Jackson wrote:
> > Juergen Gross writes ("Re: [PATCH 0/3] libxl: add framework for device types"):
> >> On 06/07/16 13:04, Ian Jackson wrote:
> >>>> +    for (i = 0; i < d_config->num_pcidevs; i++) {
> >>>> +        rc = libxl__device_pci_add(gc, domid, &d_config->pcidevs[i], 1);
> >>>> +        if (rc < 0) {
> >>>> +            LOG(ERROR, "libxl_device_pci_add failed: %d", rc);
> >>>> +            goto out;
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>
> >>> And there is similar code in 3/3 for dtdevs.  Could that be lifted
> >>> away somehow ?  (You'd have to take some care about the types, sadly;
> >>> ie, I think libxl__device_pci_add might have to start to take a
> >>> void*; maybe some macros could make things typesafe?)
> >>
> >> I thought about this idea already. I think we would end up with more
> >> code which would be rather unpleasant to read. Main reason is the
> >> need for a dtdev wrapper function and the pci backend creation.
> > 
> > I'm not sure what you mean by dtdev wrapper function.
> 
> The loop for dtdev calls a xc_ function with different parameters than
> the one for pci. We'd need a libxl__device_dtdev_add() wrapper function
> to do the xc_ call.

Oh, I see.

Sorry.  I was just suggesting that the iteration over the devices, and
the error handling, could be lifted out.  Not the contents of
libxl__device_FOO_add.

And yes, we'd need libxl__device_dtdev_add.

> 
> > But if you don't think this is feasible I won't insist on it.  The
> > approach you have is already a big improvement.
> 
> Thanks.
> 
> I'm planning to add more to it (e.g. I'd like to get rid of the
> MERGE() macro in libxl_retrieve_domain_configuration() in favor of a
> device type hook in order to be able to have _all_ stuff for one type
> in one source file.

Right.  That would be brilliant.

Ian.

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