[v1] nodedev state and update

[PATCH 04/11] nodedev: add an active config to mdev

Posted by Boris Fiuczynski 9 months ago

The configuration of a defined mdev can be modified after the mdev is
started. The defined configuration and the active configuration can
therefore run out of sync. Handle this by storing the modifiable data
which is the mdev type and attributes in two separate active and
defined configurations.  mdevctl supports with callout scripts to do an
attribute retrieval of started mdevs which is already implemented in
libvirt.

Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
 include/libvirt/libvirt-nodedev.h             | 11 ++++
 src/conf/node_device_conf.c                   | 53 ++++++++++------
 src/conf/node_device_conf.h                   |  5 +-
 src/libvirt-nodedev.c                         |  2 +-
 src/node_device/node_device_driver.c          | 61 +++++++++++++------
 src/node_device/node_device_driver.h          |  6 +-
 src/node_device/node_device_udev.c            |  4 +-
 src/test/test_driver.c                        |  6 +-
 tests/nodedevmdevctltest.c                    |  4 +-
 ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 14 +++++
 ...d_b7f0_4fea_b468_f1da537d301b_inactive.xml |  1 +
 ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 10 +++
 ...c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml |  9 +++
 ...9_36ea_4111_8f0a_8c9a70e21366_inactive.xml |  1 +
 ...9_495e_4243_ad9f_beb3f14c23d9_inactive.xml |  1 +
 ...4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml |  8 +++
 ...6_1ca8_49ac_b176_871d16c13076_inactive.xml |  1 +
 tests/nodedevxml2xmltest.c                    | 59 +++++++++++++++---
 18 files changed, 197 insertions(+), 59 deletions(-)
 create mode 100644 tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
 create mode 120000 tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
 create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
 create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
 create mode 120000 tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
 create mode 120000 tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
 create mode 100644 tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
 create mode 120000 tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml

diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h
index 428b0d722f..d18246e2f6 100644
--- a/include/libvirt/libvirt-nodedev.h
+++ b/include/libvirt/libvirt-nodedev.h
@@ -117,6 +117,17 @@ int                     virNodeDeviceListCaps    (virNodeDevicePtr dev,
                                                   char **const names,
                                                   int maxnames);
 
+/**
+ * virNodeDeviceGetXMLDescFlags:
+ *
+ * Flags used to provide the state of the returned node device configuration.
+ *
+ * Since: 10.1.0
+ */
+typedef enum {
+    VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE            = 1 << 0, /* dump inactive device configuration (Since: 10.1.0) */
+} virNodeDeviceGetXMLDescFlags;
+
 char *                  virNodeDeviceGetXMLDesc (virNodeDevicePtr dev,
                                                  unsigned int flags);
 
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 9a0fc68e67..40e15ef4da 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -599,16 +599,22 @@ virNodeDeviceCapMdevAttrFormat(virBuffer *buf,
 
 static void
 virNodeDeviceCapMdevDefFormat(virBuffer *buf,
-                              const virNodeDevCapData *data)
+                              const virNodeDevCapData *data,
+                              bool defined)
 {
-    virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.dev_config.type);
+    if (defined)
+        virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.defined_config.type);
+    else
+        virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.active_config.type);
     virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid);
     virBufferEscapeString(buf, "<parent_addr>%s</parent_addr>\n",
                           data->mdev.parent_addr);
     virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",
                       data->mdev.iommuGroupNumber);
-
-    virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.dev_config);
+    if (defined)
+        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.defined_config);
+    else
+        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.active_config);
 }
 
 static void
@@ -659,25 +665,28 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,
 
 
 char *
-virNodeDeviceDefFormat(const virNodeDeviceDef *def)
+virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags)
 {
     g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
     virNodeDevCapsDef *caps;
     size_t i = 0;
+    bool inactive_state = flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
 
     virBufferAddLit(&buf, "<device>\n");
     virBufferAdjustIndent(&buf, 2);
     virBufferEscapeString(&buf, "<name>%s</name>\n", def->name);
-    virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path);
-    virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n",
-                          def->devnode);
-    if (def->devlinks) {
-        for (i = 0; def->devlinks[i]; i++)
-            virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n",
-                                  def->devlinks[i]);
+    if (!inactive_state) {
+        virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path);
+        virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n",
+                              def->devnode);
+        if (def->devlinks) {
+            for (i = 0; def->devlinks[i]; i++)
+                virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n",
+                                      def->devlinks[i]);
+        }
     }
     virBufferEscapeString(&buf, "<parent>%s</parent>\n", def->parent);
-    if (def->driver) {
+    if (def->driver && !inactive_state) {
         virBufferAddLit(&buf, "<driver>\n");
         virBufferAdjustIndent(&buf, 2);
         virBufferEscapeString(&buf, "<name>%s</name>\n", def->driver);
@@ -738,7 +747,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
             virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type));
             break;
         case VIR_NODE_DEV_CAP_MDEV:
-            virNodeDeviceCapMdevDefFormat(&buf, data);
+            virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state);
             break;
         case VIR_NODE_DEV_CAP_CCW_DEV:
             virNodeDeviceCapCCWDefFormat(&buf, data);
@@ -2226,7 +2235,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
 
     ctxt->node = node;
 
-    if (!(mdev->dev_config.type = virXPathString("string(./type[1]/@id)", ctxt))) {
+    if (!(mdev->defined_config.type = virXPathString("string(./type[1]/@id)", ctxt))) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("missing type id attribute for '%1$s'"), def->name);
         return -1;
@@ -2258,7 +2267,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
         return -1;
 
     for (i = 0; i < nattrs; i++)
-        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->dev_config);
+        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->defined_config);
 
     return 0;
 }
@@ -2601,11 +2610,15 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
         g_free(data->sg.path);
         break;
     case VIR_NODE_DEV_CAP_MDEV:
-        g_free(data->mdev.dev_config.type);
+        g_free(data->mdev.defined_config.type);
+        g_free(data->mdev.active_config.type);
         g_free(data->mdev.uuid);
-        for (i = 0; i < data->mdev.dev_config.nattributes; i++)
-            virMediatedDeviceAttrFree(data->mdev.dev_config.attributes[i]);
-        g_free(data->mdev.dev_config.attributes);
+        for (i = 0; i < data->mdev.defined_config.nattributes; i++)
+            virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
+        g_free(data->mdev.defined_config.attributes);
+        for (i = 0; i < data->mdev.active_config.nattributes; i++)
+            virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
+        g_free(data->mdev.active_config.attributes);
         g_free(data->mdev.parent_addr);
         break;
     case VIR_NODE_DEV_CAP_CSS_DEV:
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index f100496272..f59440dbb9 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -153,7 +153,8 @@ typedef struct _virNodeDevCapMdev virNodeDevCapMdev;
 struct _virNodeDevCapMdev {
     unsigned int iommuGroupNumber;
     char *uuid;
-    virMediatedDeviceConfig dev_config;
+    virMediatedDeviceConfig defined_config;
+    virMediatedDeviceConfig active_config;
     char *parent_addr;
     bool autostart;
 };
@@ -360,7 +361,7 @@ struct _virNodeDeviceDef {
 };
 
 char *
-virNodeDeviceDefFormat(const virNodeDeviceDef *def);
+virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags);
 
 
 typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef *dev,
diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
index f0f99bc020..9cc5c6466b 100644
--- a/src/libvirt-nodedev.c
+++ b/src/libvirt-nodedev.c
@@ -264,7 +264,7 @@ virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
 /**
  * virNodeDeviceGetXMLDesc:
  * @dev: pointer to the node device
- * @flags: extra flags; not used yet, so callers should always pass 0
+ * @flags: bitwise-OR of virNodeDeviceGetXMLDescFlags
  *
  * Fetch an XML document describing all aspects of
  * the device.
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 0c8612eb71..d67c95d68d 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -338,7 +338,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
     virNodeDeviceDef *def;
     char *ret = NULL;
 
-    virCheckFlags(0, NULL);
+    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
 
     if (nodeDeviceInitWait() < 0)
         return NULL;
@@ -356,7 +356,19 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
     if (virNodeDeviceUpdateCaps(def) < 0)
         goto cleanup;
 
-    ret = virNodeDeviceDefFormat(def);
+    if (flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) {
+        if (!virNodeDeviceObjIsPersistent(obj)) {
+            virReportError(VIR_ERR_OPERATION_INVALID,
+                           _("node device '%1$s' is not persistent"),
+                           def->name);
+            goto cleanup;
+        }
+    } else {
+        if (!virNodeDeviceObjIsActive(obj))
+            flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
+    }
+
+    ret = virNodeDeviceDefFormat(def, flags);
 
  cleanup:
     virNodeDeviceObjEndAPI(&obj);
@@ -629,19 +641,20 @@ nodeDeviceAttributesToJSON(virJSONValue *json,
 /* format a json string that provides configuration information about this mdev
  * to the mdevctl utility */
 static int
-nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf)
+nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf, bool defined)
 {
     virNodeDevCapMdev *mdev = &def->caps->data.mdev;
+    virMediatedDeviceConfig mdev_config = defined ? mdev->defined_config : mdev->active_config;
     g_autoptr(virJSONValue) json = virJSONValueNewObject();
     const char *startval = mdev->autostart ? "auto" : "manual";
 
-    if (virJSONValueObjectAppendString(json, "mdev_type", mdev->dev_config.type) < 0)
+    if (virJSONValueObjectAppendString(json, "mdev_type", mdev_config.type) < 0)
         return -1;
 
     if (virJSONValueObjectAppendString(json, "start", startval) < 0)
         return -1;
 
-    if (nodeDeviceAttributesToJSON(json, mdev->dev_config) < 0)
+    if (nodeDeviceAttributesToJSON(json, mdev_config) < 0)
         return -1;
 
     *buf = virJSONValueToString(json, false);
@@ -760,7 +773,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
             return NULL;
         }
 
-        if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
+        if (nodeDeviceDefToMdevctlConfig(def, &inbuf, true) < 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("couldn't convert node device def to mdevctl JSON"));
             return NULL;
@@ -1138,9 +1151,11 @@ nodeDeviceParseMdevctlAttribs(virMediatedDeviceConfig *config,
 
 static virNodeDeviceDef*
 nodeDeviceParseMdevctlChildDevice(const char *parent,
-                                  virJSONValue *json)
+                                  virJSONValue *json,
+                                  bool defined)
 {
     virNodeDevCapMdev *mdev;
+    virMediatedDeviceConfig *mdev_config;
     const char *uuid;
     virJSONValue *props;
     g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
@@ -1170,14 +1185,15 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
     child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;
 
     mdev = &child->caps->data.mdev;
+    mdev_config = defined ? &mdev->defined_config : &mdev->active_config;
     mdev->uuid = g_strdup(uuid);
     mdev->parent_addr = g_strdup(parent);
-    mdev->dev_config.type =
+    mdev_config->type =
         g_strdup(virJSONValueObjectGetString(props, "mdev_type"));
     start = virJSONValueObjectGetString(props, "start");
     mdev->autostart = STREQ_NULLABLE(start, "auto");
 
-    if (nodeDeviceParseMdevctlAttribs(&mdev->dev_config,
+    if (nodeDeviceParseMdevctlAttribs(mdev_config,
                                       virJSONValueObjectGet(props, "attrs")) < 0)
         return NULL;
 
@@ -1189,7 +1205,8 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
 
 int
 nodeDeviceParseMdevctlJSON(const char *jsonstring,
-                           virNodeDeviceDef ***devs)
+                           virNodeDeviceDef ***devs,
+                           bool defined)
 {
     int n;
     g_autoptr(virJSONValue) json_devicelist = NULL;
@@ -1259,7 +1276,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
             g_autoptr(virNodeDeviceDef) child = NULL;
             virJSONValue *child_obj = virJSONValueArrayGet(child_array, j);
 
-            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) {
+            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj, defined))) {
                 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                                _("Unable to parse child device"));
                 goto error;
@@ -1402,7 +1419,7 @@ nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def,
         /* Active devices contain some additional information (e.g. sysfs
          * path) that is not provided by mdevctl, so re-use the existing
          * definition and copy over new mdev data */
-        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned);
+        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned, defined);
 
         if (was_defined && !changed) {
             /* if this device was already defined and the definition
@@ -1672,7 +1689,7 @@ virMdevctlList(bool defined,
         return -1;
     }
 
-    return nodeDeviceParseMdevctlJSON(output, devs);
+    return nodeDeviceParseMdevctlJSON(output, devs, defined);
 }
 
 
@@ -1831,16 +1848,24 @@ virMediatedDeviceAttrsCopy(virMediatedDeviceConfig *dst_config,
  * Returns true if anything was copied, else returns false */
 bool
 nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
-                             virNodeDeviceDef *src)
+                             virNodeDeviceDef *src,
+                             bool defined)
 {
     bool ret = false;
     virNodeDevCapMdev *srcmdev = &src->caps->data.mdev;
     virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev;
+    virMediatedDeviceConfig *srcmdevconfig = &src->caps->data.mdev.active_config;
+    virMediatedDeviceConfig *dstmdevconfig = &dst->caps->data.mdev.active_config;
+
+    if (defined) {
+        srcmdevconfig = &src->caps->data.mdev.defined_config;
+        dstmdevconfig = &dst->caps->data.mdev.defined_config;
+    }
 
-    if (STRNEQ_NULLABLE(dstmdev->dev_config.type, srcmdev->dev_config.type)) {
+    if (STRNEQ_NULLABLE(dstmdevconfig->type, srcmdevconfig->type)) {
         ret = true;
-        g_free(dstmdev->dev_config.type);
-        dstmdev->dev_config.type = g_strdup(srcmdev->dev_config.type);
+        g_free(dstmdevconfig->type);
+        dstmdevconfig->type = g_strdup(srcmdevconfig->type);
     }
 
     if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) {
@@ -1849,7 +1874,7 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
         dstmdev->uuid = g_strdup(srcmdev->uuid);
     }
 
-    if (virMediatedDeviceAttrsCopy(&dstmdev->dev_config, &srcmdev->dev_config))
+    if (virMediatedDeviceAttrsCopy(dstmdevconfig, srcmdevconfig))
         ret = true;
 
     if (dstmdev->autostart != srcmdev->autostart) {
diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
index c7d5e22daf..4dce7e6f17 100644
--- a/src/node_device/node_device_driver.h
+++ b/src/node_device/node_device_driver.h
@@ -142,7 +142,8 @@ nodeDeviceGetMdevctlListCommand(bool defined,
 
 int
 nodeDeviceParseMdevctlJSON(const char *jsonstring,
-                           virNodeDeviceDef ***devs);
+                           virNodeDeviceDef ***devs,
+                           bool defined);
 
 int
 nodeDeviceUpdateMediatedDevices(void);
@@ -154,7 +155,8 @@ nodeDeviceGenerateName(virNodeDeviceDef *def,
                        const char *s);
 
 bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
-                                  virNodeDeviceDef *src);
+                                  virNodeDeviceDef *src,
+                                  bool defined);
 
 int
 nodeDeviceCreate(virNodeDevice *dev,
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 254e802c50..57368a96c3 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1069,7 +1069,7 @@ udevProcessMediatedDevice(struct udev_device *dev,
         return -1;
     }
 
-    data->dev_config.type = g_path_get_basename(canonicalpath);
+    data->active_config.type = g_path_get_basename(canonicalpath);
 
     data->uuid = g_strdup(udev_device_get_sysname(dev));
     if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0)
@@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device)
         objdef = virNodeDeviceObjGetDef(obj);
 
         if (is_mdev)
-            nodeDeviceDefCopyFromMdevctl(def, objdef);
+            nodeDeviceDefCopyFromMdevctl(def, objdef, false);
 
         persistent = virNodeDeviceObjIsPersistent(obj);
         autostart = virNodeDeviceObjIsAutostart(obj);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ed545848af..01863233bc 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -7514,12 +7514,12 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
     virNodeDeviceObj *obj;
     char *ret = NULL;
 
-    virCheckFlags(0, NULL);
+    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
 
     if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
         return NULL;
 
-    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj));
+    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);
 
     virNodeDeviceObjEndAPI(&obj);
     return ret;
@@ -7619,7 +7619,7 @@ testNodeDeviceMockCreateVport(testDriver *driver,
                                                    "scsi_host11")))
         goto cleanup;
 
-    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy));
+    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy), 0);
     virNodeDeviceObjEndAPI(&objcopy);
     if (!xml)
         goto cleanup;
diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
index e403328e5a..852d9ed6e7 100644
--- a/tests/nodedevmdevctltest.c
+++ b/tests/nodedevmdevctltest.c
@@ -229,13 +229,13 @@ testMdevctlParse(const void *data)
         return -1;
     }
 
-    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) {
+    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs, true)) < 0) {
         VIR_TEST_DEBUG("Unable to parse json for %s", filename);
         return -1;
     }
 
     for (i = 0; i < nmdevs; i++) {
-        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]);
+        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i], VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE);
         if (!devxml)
             goto cleanup;
         virBufferAddStr(&xmloutbuf, devxml);
diff --git a/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
new file mode 100644
index 0000000000..6926559efa
--- /dev/null
+++ b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
@@ -0,0 +1,14 @@
+<device>
+  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
+  <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
+  <parent>css_0_0_0052</parent>
+  <driver>
+    <name>vfio_ccw_mdev</name>
+  </driver>
+  <capability type='mdev'>
+    <type id='vfio_ccw-io'/>
+    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
+    <parent_addr>0.0.0052</parent_addr>
+    <iommuGroup number='4'/>
+  </capability>
+</device>
diff --git a/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
new file mode 120000
index 0000000000..f8ec7d8a32
--- /dev/null
+++ b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
@@ -0,0 +1 @@
+mdev_3627463d_b7f0_4fea_b468_f1da537d301b.xml
\ No newline at end of file
diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
new file mode 100644
index 0000000000..82c60cc065
--- /dev/null
+++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
@@ -0,0 +1,10 @@
+<device>
+  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
+  <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
+  <parent>css_0_0_0052</parent>
+  <capability type='mdev'>
+    <type id='vfio_ccw-io'/>
+    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
+    <iommuGroup number='4'/>
+  </capability>
+</device>
diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
new file mode 100644
index 0000000000..87c5ed1340
--- /dev/null
+++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
@@ -0,0 +1,9 @@
+<device>
+  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
+  <parent>css_0_0_0052</parent>
+  <capability type='mdev'>
+    <type id='vfio_ccw-io'/>
+    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
+    <iommuGroup number='4'/>
+  </capability>
+</device>
diff --git a/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
new file mode 120000
index 0000000000..f6d5789399
--- /dev/null
+++ b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
@@ -0,0 +1 @@
+mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml
\ No newline at end of file
diff --git a/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
new file mode 120000
index 0000000000..233a7506fb
--- /dev/null
+++ b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
@@ -0,0 +1 @@
+mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml
\ No newline at end of file
diff --git a/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
new file mode 100644
index 0000000000..8c4983f43c
--- /dev/null
+++ b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
@@ -0,0 +1,8 @@
+<device>
+  <name>mdev_ee0b88c4-f554-4dc1-809d-b2a01e8e48ad</name>
+  <parent>ap_matrix</parent>
+  <capability type='mdev'>
+    <type id='vfio_ap-passthrough'/>
+    <iommuGroup number='0'/>
+  </capability>
+</device>
diff --git a/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
new file mode 120000
index 0000000000..e053844177
--- /dev/null
+++ b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
@@ -0,0 +1 @@
+mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml
\ No newline at end of file
diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
index 068ec68769..be7a5b4df9 100644
--- a/tests/nodedevxml2xmltest.c
+++ b/tests/nodedevxml2xmltest.c
@@ -11,14 +11,20 @@
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
+struct TestData {
+    const char *filename;
+    unsigned int flags;
+};
+
 static int
-testCompareXMLToXMLFiles(const char *xml, const char *outfile)
+testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flags)
 {
     g_autofree char *xmlData = NULL;
     g_autofree char *actual = NULL;
     int ret = -1;
     virNodeDeviceDef *dev = NULL;
     virNodeDevCapsDef *caps;
+    size_t i;
 
     if (virTestLoadFile(xml, &xmlData) < 0)
         goto fail;
@@ -46,9 +52,22 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile)
                                            data->storage.logical_block_size;
             }
         }
+
+        if (caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
+            !(flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)) {
+            data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type);
+            for (i = 0; i < data->mdev.defined_config.nattributes; i++) {
+                g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1);
+                attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name);
+                attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value);
+                VIR_APPEND_ELEMENT(data->mdev.active_config.attributes,
+                                   data->mdev.active_config.nattributes,
+                                   attr);
+            }
+        }
     }
 
-    if (!(actual = virNodeDeviceDefFormat(dev)))
+    if (!(actual = virNodeDeviceDefFormat(dev, flags)))
         goto fail;
 
     if (virTestCompareToFile(actual, outfile) < 0)
@@ -65,16 +84,21 @@ static int
 testCompareXMLToXMLHelper(const void *data)
 {
     int result = -1;
+    const struct TestData *tdata = data;
     g_autofree char *xml = NULL;
     g_autofree char *outfile = NULL;
 
     xml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
-                          (const char *)data);
+                          tdata->filename);
 
-    outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir,
-                              (const char *)data);
+    if (tdata->flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
+        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s_inactive.xml", abs_srcdir,
+                                  tdata->filename);
+    else
+        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir,
+                                  tdata->filename);
 
-    result = testCompareXMLToXMLFiles(xml, outfile);
+    result = testCompareXMLToXMLFiles(xml, outfile, tdata->flags);
 
     return result;
 }
@@ -85,10 +109,20 @@ mymain(void)
 {
     int ret = 0;
 
+#define DO_TEST_FLAGS(desc, filename, flags) \
+    do { \
+        struct TestData data = { filename, flags }; \
+        if (virTestRun(desc, testCompareXMLToXMLHelper, &data) < 0) \
+            ret = -1; \
+       } \
+    while (0)
+
 #define DO_TEST(name) \
-    if (virTestRun("Node device XML-2-XML " name, \
-                   testCompareXMLToXMLHelper, (name)) < 0) \
-        ret = -1
+    DO_TEST_FLAGS("Node device XML-2-XML " name, name, 0)
+
+#define DO_TEST_INACTIVE(name) \
+    DO_TEST_FLAGS("Node device XML-2-XML INACTIVE " name, \
+                  name, VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
 
     DO_TEST("computer");
     DO_TEST("DVD_GCC_4247N");
@@ -121,6 +155,7 @@ mymain(void)
     DO_TEST("pci_0000_02_10_7_mdev_types");
     DO_TEST("pci_0000_42_00_0_vpd");
     DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
+    DO_TEST_INACTIVE("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
     DO_TEST("ccw_0_0_ffff");
     DO_TEST("css_0_0_ffff");
     DO_TEST("css_0_0_ffff_channel_dev_addr");
@@ -134,7 +169,13 @@ mymain(void)
     DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
     DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
     DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
+    DO_TEST_INACTIVE("mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad");
+    DO_TEST_INACTIVE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
+    DO_TEST_INACTIVE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
+    DO_TEST_INACTIVE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
     DO_TEST("hba_vport_ops");
+    DO_TEST("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
+    DO_TEST_INACTIVE("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
 
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }
-- 
2.42.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH 04/11] nodedev: add an active config to mdev

Posted by Jonathon Jongsma 8 months, 2 weeks ago

On 1/19/24 10:38 AM, Boris Fiuczynski wrote:
> The configuration of a defined mdev can be modified after the mdev is
> started. The defined configuration and the active configuration can
> therefore run out of sync. Handle this by storing the modifiable data
> which is the mdev type and attributes in two separate active and
> defined configurations.  mdevctl supports with callout scripts to do an
> attribute retrieval of started mdevs which is already implemented in
> libvirt.
> 
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
>   include/libvirt/libvirt-nodedev.h             | 11 ++++
>   src/conf/node_device_conf.c                   | 53 ++++++++++------
>   src/conf/node_device_conf.h                   |  5 +-
>   src/libvirt-nodedev.c                         |  2 +-
>   src/node_device/node_device_driver.c          | 61 +++++++++++++------
>   src/node_device/node_device_driver.h          |  6 +-
>   src/node_device/node_device_udev.c            |  4 +-
>   src/test/test_driver.c                        |  6 +-
>   tests/nodedevmdevctltest.c                    |  4 +-
>   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 14 +++++
>   ...d_b7f0_4fea_b468_f1da537d301b_inactive.xml |  1 +
>   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 10 +++
>   ...c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml |  9 +++
>   ...9_36ea_4111_8f0a_8c9a70e21366_inactive.xml |  1 +
>   ...9_495e_4243_ad9f_beb3f14c23d9_inactive.xml |  1 +
>   ...4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml |  8 +++
>   ...6_1ca8_49ac_b176_871d16c13076_inactive.xml |  1 +
>   tests/nodedevxml2xmltest.c                    | 59 +++++++++++++++---
>   18 files changed, 197 insertions(+), 59 deletions(-)
>   create mode 100644 tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>   create mode 120000 tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
>   create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>   create mode 100644 tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
>   create mode 120000 tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
>   create mode 120000 tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
>   create mode 100644 tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
>   create mode 120000 tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
> 
> diff --git a/include/libvirt/libvirt-nodedev.h b/include/libvirt/libvirt-nodedev.h
> index 428b0d722f..d18246e2f6 100644
> --- a/include/libvirt/libvirt-nodedev.h
> +++ b/include/libvirt/libvirt-nodedev.h
> @@ -117,6 +117,17 @@ int                     virNodeDeviceListCaps    (virNodeDevicePtr dev,
>                                                     char **const names,
>                                                     int maxnames);
>   
> +/**
> + * virNodeDeviceGetXMLDescFlags:
> + *
> + * Flags used to provide the state of the returned node device configuration.
> + *
> + * Since: 10.1.0
> + */
> +typedef enum {
> +    VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE            = 1 << 0, /* dump inactive device configuration (Since: 10.1.0) */
> +} virNodeDeviceGetXMLDescFlags;

In all of the other similar cases, this type is named 
vir$(OBJECT)XMLFlags and the flag itself is named 
VIR_$(OBJECT)_XML_INACTIVE. So for consistency, I'd remove the 'get' and 
the 'desc' from the names.

> +
>   char *                  virNodeDeviceGetXMLDesc (virNodeDevicePtr dev,
>                                                    unsigned int flags);
>   
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 9a0fc68e67..40e15ef4da 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -599,16 +599,22 @@ virNodeDeviceCapMdevAttrFormat(virBuffer *buf,
>   
>   static void
>   virNodeDeviceCapMdevDefFormat(virBuffer *buf,
> -                              const virNodeDevCapData *data)
> +                              const virNodeDevCapData *data,
> +                              bool defined)
>   {
> -    virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.dev_config.type);
> +    if (defined)
> +        virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.defined_config.type);
> +    else
> +        virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.active_config.type);
>       virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid);
>       virBufferEscapeString(buf, "<parent_addr>%s</parent_addr>\n",
>                             data->mdev.parent_addr);
>       virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",
>                         data->mdev.iommuGroupNumber);
> -
> -    virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.dev_config);
> +    if (defined)
> +        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.defined_config);
> +    else
> +        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.active_config);
>   }
>   
>   static void
> @@ -659,25 +665,28 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,
>   
>   
>   char *
> -virNodeDeviceDefFormat(const virNodeDeviceDef *def)
> +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags)
>   {
>       g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>       virNodeDevCapsDef *caps;
>       size_t i = 0;
> +    bool inactive_state = flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>   
>       virBufferAddLit(&buf, "<device>\n");
>       virBufferAdjustIndent(&buf, 2);
>       virBufferEscapeString(&buf, "<name>%s</name>\n", def->name);
> -    virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path);
> -    virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n",
> -                          def->devnode);
> -    if (def->devlinks) {
> -        for (i = 0; def->devlinks[i]; i++)
> -            virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n",
> -                                  def->devlinks[i]);
> +    if (!inactive_state) {
> +        virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path);
> +        virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n",
> +                              def->devnode);
> +        if (def->devlinks) {
> +            for (i = 0; def->devlinks[i]; i++)
> +                virBufferEscapeString(&buf, "<devnode type='link'>%s</devnode>\n",
> +                                      def->devlinks[i]);
> +        }
>       }
>       virBufferEscapeString(&buf, "<parent>%s</parent>\n", def->parent);
> -    if (def->driver) {
> +    if (def->driver && !inactive_state) {
>           virBufferAddLit(&buf, "<driver>\n");
>           virBufferAdjustIndent(&buf, 2);
>           virBufferEscapeString(&buf, "<name>%s</name>\n", def->driver);
> @@ -738,7 +747,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>               virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type));
>               break;
>           case VIR_NODE_DEV_CAP_MDEV:
> -            virNodeDeviceCapMdevDefFormat(&buf, data);
> +            virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state);
>               break;
>           case VIR_NODE_DEV_CAP_CCW_DEV:
>               virNodeDeviceCapCCWDefFormat(&buf, data);
> @@ -2226,7 +2235,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>   
>       ctxt->node = node;
>   
> -    if (!(mdev->dev_config.type = virXPathString("string(./type[1]/@id)", ctxt))) {
> +    if (!(mdev->defined_config.type = virXPathString("string(./type[1]/@id)", ctxt))) {
>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                          _("missing type id attribute for '%1$s'"), def->name);
>           return -1;
> @@ -2258,7 +2267,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>           return -1;
>   
>       for (i = 0; i < nattrs; i++)
> -        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->dev_config);
> +        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], &mdev->defined_config);
>   
>       return 0;
>   }
> @@ -2601,11 +2610,15 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
>           g_free(data->sg.path);
>           break;
>       case VIR_NODE_DEV_CAP_MDEV:
> -        g_free(data->mdev.dev_config.type);
> +        g_free(data->mdev.defined_config.type);
> +        g_free(data->mdev.active_config.type);
>           g_free(data->mdev.uuid);
> -        for (i = 0; i < data->mdev.dev_config.nattributes; i++)
> -            virMediatedDeviceAttrFree(data->mdev.dev_config.attributes[i]);
> -        g_free(data->mdev.dev_config.attributes);
> +        for (i = 0; i < data->mdev.defined_config.nattributes; i++)
> +            virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
> +        g_free(data->mdev.defined_config.attributes);
> +        for (i = 0; i < data->mdev.active_config.nattributes; i++)
> +            virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
> +        g_free(data->mdev.active_config.attributes);
>           g_free(data->mdev.parent_addr);
>           break;
>       case VIR_NODE_DEV_CAP_CSS_DEV:
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index f100496272..f59440dbb9 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -153,7 +153,8 @@ typedef struct _virNodeDevCapMdev virNodeDevCapMdev;
>   struct _virNodeDevCapMdev {
>       unsigned int iommuGroupNumber;
>       char *uuid;
> -    virMediatedDeviceConfig dev_config;
> +    virMediatedDeviceConfig defined_config;
> +    virMediatedDeviceConfig active_config;
>       char *parent_addr;
>       bool autostart;
>   };
> @@ -360,7 +361,7 @@ struct _virNodeDeviceDef {
>   };
>   
>   char *
> -virNodeDeviceDefFormat(const virNodeDeviceDef *def);
> +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags);
>   
>   
>   typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef *dev,
> diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
> index f0f99bc020..9cc5c6466b 100644
> --- a/src/libvirt-nodedev.c
> +++ b/src/libvirt-nodedev.c
> @@ -264,7 +264,7 @@ virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>   /**
>    * virNodeDeviceGetXMLDesc:
>    * @dev: pointer to the node device
> - * @flags: extra flags; not used yet, so callers should always pass 0
> + * @flags: bitwise-OR of virNodeDeviceGetXMLDescFlags
>    *
>    * Fetch an XML document describing all aspects of
>    * the device.
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index 0c8612eb71..d67c95d68d 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -338,7 +338,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
>       virNodeDeviceDef *def;
>       char *ret = NULL;
>   
> -    virCheckFlags(0, NULL);
> +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
>   
>       if (nodeDeviceInitWait() < 0)
>           return NULL;
> @@ -356,7 +356,19 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
>       if (virNodeDeviceUpdateCaps(def) < 0)
>           goto cleanup;
>   
> -    ret = virNodeDeviceDefFormat(def);
> +    if (flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) {
> +        if (!virNodeDeviceObjIsPersistent(obj)) {
> +            virReportError(VIR_ERR_OPERATION_INVALID,
> +                           _("node device '%1$s' is not persistent"),
> +                           def->name);
> +            goto cleanup;
> +        }
> +    } else {
> +        if (!virNodeDeviceObjIsActive(obj))
> +            flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
> +    }
> +
> +    ret = virNodeDeviceDefFormat(def, flags);
>   
>    cleanup:
>       virNodeDeviceObjEndAPI(&obj);
> @@ -629,19 +641,20 @@ nodeDeviceAttributesToJSON(virJSONValue *json,
>   /* format a json string that provides configuration information about this mdev
>    * to the mdevctl utility */
>   static int
> -nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf)
> +nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf, bool defined)

As far as I can tell, you never actually call this function with 
'defined' set to false. It doesn't seem to be used in any of the 
following commits either.

>   {
>       virNodeDevCapMdev *mdev = &def->caps->data.mdev;
> +    virMediatedDeviceConfig mdev_config = defined ? mdev->defined_config : mdev->active_config;
>       g_autoptr(virJSONValue) json = virJSONValueNewObject();
>       const char *startval = mdev->autostart ? "auto" : "manual";
>   
> -    if (virJSONValueObjectAppendString(json, "mdev_type", mdev->dev_config.type) < 0)
> +    if (virJSONValueObjectAppendString(json, "mdev_type", mdev_config.type) < 0)
>           return -1;
>   
>       if (virJSONValueObjectAppendString(json, "start", startval) < 0)
>           return -1;
>   
> -    if (nodeDeviceAttributesToJSON(json, mdev->dev_config) < 0)
> +    if (nodeDeviceAttributesToJSON(json, mdev_config) < 0)
>           return -1;
>   
>       *buf = virJSONValueToString(json, false);
> @@ -760,7 +773,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
>               return NULL;
>           }
>   
> -        if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
> +        if (nodeDeviceDefToMdevctlConfig(def, &inbuf, true) < 0) {
>               virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                              _("couldn't convert node device def to mdevctl JSON"));
>               return NULL;
> @@ -1138,9 +1151,11 @@ nodeDeviceParseMdevctlAttribs(virMediatedDeviceConfig *config,
>   
>   static virNodeDeviceDef*
>   nodeDeviceParseMdevctlChildDevice(const char *parent,
> -                                  virJSONValue *json)
> +                                  virJSONValue *json,
> +                                  bool defined)
>   {
>       virNodeDevCapMdev *mdev;
> +    virMediatedDeviceConfig *mdev_config;
>       const char *uuid;
>       virJSONValue *props;
>       g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
> @@ -1170,14 +1185,15 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
>       child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;
>   
>       mdev = &child->caps->data.mdev;
> +    mdev_config = defined ? &mdev->defined_config : &mdev->active_config;
>       mdev->uuid = g_strdup(uuid);
>       mdev->parent_addr = g_strdup(parent);
> -    mdev->dev_config.type =
> +    mdev_config->type =
>           g_strdup(virJSONValueObjectGetString(props, "mdev_type"));
>       start = virJSONValueObjectGetString(props, "start");
>       mdev->autostart = STREQ_NULLABLE(start, "auto");
>   
> -    if (nodeDeviceParseMdevctlAttribs(&mdev->dev_config,
> +    if (nodeDeviceParseMdevctlAttribs(mdev_config,
>                                         virJSONValueObjectGet(props, "attrs")) < 0)
>           return NULL;
>   
> @@ -1189,7 +1205,8 @@ nodeDeviceParseMdevctlChildDevice(const char *parent,
>   
>   int
>   nodeDeviceParseMdevctlJSON(const char *jsonstring,
> -                           virNodeDeviceDef ***devs)
> +                           virNodeDeviceDef ***devs,
> +                           bool defined)
>   {
>       int n;
>       g_autoptr(virJSONValue) json_devicelist = NULL;
> @@ -1259,7 +1276,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
>               g_autoptr(virNodeDeviceDef) child = NULL;
>               virJSONValue *child_obj = virJSONValueArrayGet(child_array, j);
>   
> -            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj))) {
> +            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, child_obj, defined))) {
>                   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                  _("Unable to parse child device"));
>                   goto error;
> @@ -1402,7 +1419,7 @@ nodeDeviceUpdateMediatedDevice(virNodeDeviceDef *def,
>           /* Active devices contain some additional information (e.g. sysfs
>            * path) that is not provided by mdevctl, so re-use the existing
>            * definition and copy over new mdev data */
> -        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned);
> +        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned, defined);
>   
>           if (was_defined && !changed) {
>               /* if this device was already defined and the definition
> @@ -1672,7 +1689,7 @@ virMdevctlList(bool defined,
>           return -1;
>       }
>   
> -    return nodeDeviceParseMdevctlJSON(output, devs);
> +    return nodeDeviceParseMdevctlJSON(output, devs, defined);
>   }
>   
>   
> @@ -1831,16 +1848,24 @@ virMediatedDeviceAttrsCopy(virMediatedDeviceConfig *dst_config,
>    * Returns true if anything was copied, else returns false */
>   bool
>   nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
> -                             virNodeDeviceDef *src)
> +                             virNodeDeviceDef *src,
> +                             bool defined)
>   {
>       bool ret = false;
>       virNodeDevCapMdev *srcmdev = &src->caps->data.mdev;
>       virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev;
> +    virMediatedDeviceConfig *srcmdevconfig = &src->caps->data.mdev.active_config;
> +    virMediatedDeviceConfig *dstmdevconfig = &dst->caps->data.mdev.active_config;
> +
> +    if (defined) {
> +        srcmdevconfig = &src->caps->data.mdev.defined_config;
> +        dstmdevconfig = &dst->caps->data.mdev.defined_config;
> +    }
>   
> -    if (STRNEQ_NULLABLE(dstmdev->dev_config.type, srcmdev->dev_config.type)) {
> +    if (STRNEQ_NULLABLE(dstmdevconfig->type, srcmdevconfig->type)) {
>           ret = true;
> -        g_free(dstmdev->dev_config.type);
> -        dstmdev->dev_config.type = g_strdup(srcmdev->dev_config.type);
> +        g_free(dstmdevconfig->type);
> +        dstmdevconfig->type = g_strdup(srcmdevconfig->type);
>       }
>   
>       if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) {
> @@ -1849,7 +1874,7 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>           dstmdev->uuid = g_strdup(srcmdev->uuid);
>       }
>   
> -    if (virMediatedDeviceAttrsCopy(&dstmdev->dev_config, &srcmdev->dev_config))
> +    if (virMediatedDeviceAttrsCopy(dstmdevconfig, srcmdevconfig))
>           ret = true;
>   
>       if (dstmdev->autostart != srcmdev->autostart) {
> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
> index c7d5e22daf..4dce7e6f17 100644
> --- a/src/node_device/node_device_driver.h
> +++ b/src/node_device/node_device_driver.h
> @@ -142,7 +142,8 @@ nodeDeviceGetMdevctlListCommand(bool defined,
>   
>   int
>   nodeDeviceParseMdevctlJSON(const char *jsonstring,
> -                           virNodeDeviceDef ***devs);
> +                           virNodeDeviceDef ***devs,
> +                           bool defined);
>   
>   int
>   nodeDeviceUpdateMediatedDevices(void);
> @@ -154,7 +155,8 @@ nodeDeviceGenerateName(virNodeDeviceDef *def,
>                          const char *s);
>   
>   bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
> -                                  virNodeDeviceDef *src);
> +                                  virNodeDeviceDef *src,
> +                                  bool defined);
>   
>   int
>   nodeDeviceCreate(virNodeDevice *dev,
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 254e802c50..57368a96c3 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1069,7 +1069,7 @@ udevProcessMediatedDevice(struct udev_device *dev,
>           return -1;
>       }
>   
> -    data->dev_config.type = g_path_get_basename(canonicalpath);
> +    data->active_config.type = g_path_get_basename(canonicalpath);
>   
>       data->uuid = g_strdup(udev_device_get_sysname(dev));
>       if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0)
> @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device)
>           objdef = virNodeDeviceObjGetDef(obj);
>   
>           if (is_mdev)
> -            nodeDeviceDefCopyFromMdevctl(def, objdef);
> +            nodeDeviceDefCopyFromMdevctl(def, objdef, false);
>   
>           persistent = virNodeDeviceObjIsPersistent(obj);
>           autostart = virNodeDeviceObjIsAutostart(obj);
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index ed545848af..01863233bc 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -7514,12 +7514,12 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
>       virNodeDeviceObj *obj;
>       char *ret = NULL;
>   
> -    virCheckFlags(0, NULL);
> +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
>   
>       if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
>           return NULL;
>   
> -    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj));
> +    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);
>   
>       virNodeDeviceObjEndAPI(&obj);
>       return ret;
> @@ -7619,7 +7619,7 @@ testNodeDeviceMockCreateVport(testDriver *driver,
>                                                      "scsi_host11")))
>           goto cleanup;
>   
> -    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy));
> +    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy), 0);
>       virNodeDeviceObjEndAPI(&objcopy);
>       if (!xml)
>           goto cleanup;
> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
> index e403328e5a..852d9ed6e7 100644
> --- a/tests/nodedevmdevctltest.c
> +++ b/tests/nodedevmdevctltest.c
> @@ -229,13 +229,13 @@ testMdevctlParse(const void *data)
>           return -1;
>       }
>   
> -    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) {
> +    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs, true)) < 0) {
>           VIR_TEST_DEBUG("Unable to parse json for %s", filename);
>           return -1;
>       }
>   
>       for (i = 0; i < nmdevs; i++) {
> -        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]);
> +        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i], VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE);
>           if (!devxml)
>               goto cleanup;
>           virBufferAddStr(&xmloutbuf, devxml);
> diff --git a/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
> new file mode 100644
> index 0000000000..6926559efa
> --- /dev/null
> +++ b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
> @@ -0,0 +1,14 @@
> +<device>
> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
> +  <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
> +  <parent>css_0_0_0052</parent>
> +  <driver>
> +    <name>vfio_ccw_mdev</name>
> +  </driver>
> +  <capability type='mdev'>
> +    <type id='vfio_ccw-io'/>
> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
> +    <parent_addr>0.0.0052</parent_addr>
> +    <iommuGroup number='4'/>
> +  </capability>
> +</device>
> diff --git a/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
> new file mode 120000
> index 0000000000..f8ec7d8a32
> --- /dev/null
> +++ b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
> @@ -0,0 +1 @@
> +mdev_3627463d_b7f0_4fea_b468_f1da537d301b.xml
> \ No newline at end of file
> diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
> new file mode 100644
> index 0000000000..82c60cc065
> --- /dev/null
> +++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
> @@ -0,0 +1,10 @@
> +<device>
> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
> +  <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
> +  <parent>css_0_0_0052</parent>
> +  <capability type='mdev'>
> +    <type id='vfio_ccw-io'/>
> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
> +    <iommuGroup number='4'/>
> +  </capability>
> +</device>
> diff --git a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
> new file mode 100644
> index 0000000000..87c5ed1340
> --- /dev/null
> +++ b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
> @@ -0,0 +1,9 @@
> +<device>
> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
> +  <parent>css_0_0_0052</parent>
> +  <capability type='mdev'>
> +    <type id='vfio_ccw-io'/>
> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
> +    <iommuGroup number='4'/>
> +  </capability>
> +</device>
> diff --git a/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
> new file mode 120000
> index 0000000000..f6d5789399
> --- /dev/null
> +++ b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
> @@ -0,0 +1 @@
> +mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml
> \ No newline at end of file
> diff --git a/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
> new file mode 120000
> index 0000000000..233a7506fb
> --- /dev/null
> +++ b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
> @@ -0,0 +1 @@
> +mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml
> \ No newline at end of file
> diff --git a/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
> new file mode 100644
> index 0000000000..8c4983f43c
> --- /dev/null
> +++ b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
> @@ -0,0 +1,8 @@
> +<device>
> +  <name>mdev_ee0b88c4-f554-4dc1-809d-b2a01e8e48ad</name>
> +  <parent>ap_matrix</parent>
> +  <capability type='mdev'>
> +    <type id='vfio_ap-passthrough'/>
> +    <iommuGroup number='0'/>
> +  </capability>
> +</device>
> diff --git a/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
> new file mode 120000
> index 0000000000..e053844177
> --- /dev/null
> +++ b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
> @@ -0,0 +1 @@
> +mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml
> \ No newline at end of file
> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
> index 068ec68769..be7a5b4df9 100644
> --- a/tests/nodedevxml2xmltest.c
> +++ b/tests/nodedevxml2xmltest.c
> @@ -11,14 +11,20 @@
>   
>   #define VIR_FROM_THIS VIR_FROM_NONE
>   
> +struct TestData {
> +    const char *filename;
> +    unsigned int flags;
> +};
> +
>   static int
> -testCompareXMLToXMLFiles(const char *xml, const char *outfile)
> +testCompareXMLToXMLFiles(const char *xml, const char *outfile, unsigned int flags)
>   {
>       g_autofree char *xmlData = NULL;
>       g_autofree char *actual = NULL;
>       int ret = -1;
>       virNodeDeviceDef *dev = NULL;
>       virNodeDevCapsDef *caps;
> +    size_t i;
>   
>       if (virTestLoadFile(xml, &xmlData) < 0)
>           goto fail;
> @@ -46,9 +52,22 @@ testCompareXMLToXMLFiles(const char *xml, const char *outfile)
>                                              data->storage.logical_block_size;
>               }
>           }
> +
> +        if (caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
> +            !(flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)) {
> +            data->mdev.active_config.type = g_strdup(data->mdev.defined_config.type);
> +            for (i = 0; i < data->mdev.defined_config.nattributes; i++) {
> +                g_autoptr(virMediatedDeviceAttr) attr = g_new0(virMediatedDeviceAttr, 1);
> +                attr->name = g_strdup(data->mdev.defined_config.attributes[i]->name);
> +                attr->value = g_strdup(data->mdev.defined_config.attributes[i]->value);
> +                VIR_APPEND_ELEMENT(data->mdev.active_config.attributes,
> +                                   data->mdev.active_config.nattributes,
> +                                   attr);
> +            }
> +        }
>       }
>   
> -    if (!(actual = virNodeDeviceDefFormat(dev)))
> +    if (!(actual = virNodeDeviceDefFormat(dev, flags)))
>           goto fail;
>   
>       if (virTestCompareToFile(actual, outfile) < 0)
> @@ -65,16 +84,21 @@ static int
>   testCompareXMLToXMLHelper(const void *data)
>   {
>       int result = -1;
> +    const struct TestData *tdata = data;
>       g_autofree char *xml = NULL;
>       g_autofree char *outfile = NULL;
>   
>       xml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
> -                          (const char *)data);
> +                          tdata->filename);
>   
> -    outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir,
> -                              (const char *)data);
> +    if (tdata->flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
> +        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s_inactive.xml", abs_srcdir,
> +                                  tdata->filename);
> +    else
> +        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir,
> +                                  tdata->filename);
>   
> -    result = testCompareXMLToXMLFiles(xml, outfile);
> +    result = testCompareXMLToXMLFiles(xml, outfile, tdata->flags);
>   
>       return result;
>   }
> @@ -85,10 +109,20 @@ mymain(void)
>   {
>       int ret = 0;
>   
> +#define DO_TEST_FLAGS(desc, filename, flags) \
> +    do { \
> +        struct TestData data = { filename, flags }; \
> +        if (virTestRun(desc, testCompareXMLToXMLHelper, &data) < 0) \
> +            ret = -1; \
> +       } \
> +    while (0)
> +
>   #define DO_TEST(name) \
> -    if (virTestRun("Node device XML-2-XML " name, \
> -                   testCompareXMLToXMLHelper, (name)) < 0) \
> -        ret = -1
> +    DO_TEST_FLAGS("Node device XML-2-XML " name, name, 0)
> +
> +#define DO_TEST_INACTIVE(name) \
> +    DO_TEST_FLAGS("Node device XML-2-XML INACTIVE " name, \
> +                  name, VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
>   
>       DO_TEST("computer");
>       DO_TEST("DVD_GCC_4247N");
> @@ -121,6 +155,7 @@ mymain(void)
>       DO_TEST("pci_0000_02_10_7_mdev_types");
>       DO_TEST("pci_0000_42_00_0_vpd");
>       DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
> +    DO_TEST_INACTIVE("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
>       DO_TEST("ccw_0_0_ffff");
>       DO_TEST("css_0_0_ffff");
>       DO_TEST("css_0_0_ffff_channel_dev_addr");
> @@ -134,7 +169,13 @@ mymain(void)
>       DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
>       DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
>       DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
> +    DO_TEST_INACTIVE("mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad");
> +    DO_TEST_INACTIVE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
> +    DO_TEST_INACTIVE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
> +    DO_TEST_INACTIVE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
>       DO_TEST("hba_vport_ops");
> +    DO_TEST("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
> +    DO_TEST_INACTIVE("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
>   
>       return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>   }

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH 04/11] nodedev: add an active config to mdev

Posted by Boris Fiuczynski 8 months, 2 weeks ago

On 1/31/24 22:30, Jonathon Jongsma wrote:
> On 1/19/24 10:38 AM, Boris Fiuczynski wrote:
>> The configuration of a defined mdev can be modified after the mdev is
>> started. The defined configuration and the active configuration can
>> therefore run out of sync. Handle this by storing the modifiable data
>> which is the mdev type and attributes in two separate active and
>> defined configurations.  mdevctl supports with callout scripts to do an
>> attribute retrieval of started mdevs which is already implemented in
>> libvirt.
>>
>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>>   include/libvirt/libvirt-nodedev.h             | 11 ++++
>>   src/conf/node_device_conf.c                   | 53 ++++++++++------
>>   src/conf/node_device_conf.h                   |  5 +-
>>   src/libvirt-nodedev.c                         |  2 +-
>>   src/node_device/node_device_driver.c          | 61 +++++++++++++------
>>   src/node_device/node_device_driver.h          |  6 +-
>>   src/node_device/node_device_udev.c            |  4 +-
>>   src/test/test_driver.c                        |  6 +-
>>   tests/nodedevmdevctltest.c                    |  4 +-
>>   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 14 +++++
>>   ...d_b7f0_4fea_b468_f1da537d301b_inactive.xml |  1 +
>>   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 10 +++
>>   ...c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml |  9 +++
>>   ...9_36ea_4111_8f0a_8c9a70e21366_inactive.xml |  1 +
>>   ...9_495e_4243_ad9f_beb3f14c23d9_inactive.xml |  1 +
>>   ...4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml |  8 +++
>>   ...6_1ca8_49ac_b176_871d16c13076_inactive.xml |  1 +
>>   tests/nodedevxml2xmltest.c                    | 59 +++++++++++++++---
>>   18 files changed, 197 insertions(+), 59 deletions(-)
>>   create mode 100644 
>> tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>   create mode 120000 
>> tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
>>   create mode 100644 
>> tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>   create mode 100644 
>> tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
>>   create mode 120000 
>> tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
>>   create mode 120000 
>> tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
>>   create mode 100644 
>> tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
>>   create mode 120000 
>> tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
>>
>> diff --git a/include/libvirt/libvirt-nodedev.h 
>> b/include/libvirt/libvirt-nodedev.h
>> index 428b0d722f..d18246e2f6 100644
>> --- a/include/libvirt/libvirt-nodedev.h
>> +++ b/include/libvirt/libvirt-nodedev.h
>> @@ -117,6 +117,17 @@ int                     virNodeDeviceListCaps    
>> (virNodeDevicePtr dev,
>>                                                     char **const names,
>>                                                     int maxnames);
>> +/**
>> + * virNodeDeviceGetXMLDescFlags:
>> + *
>> + * Flags used to provide the state of the returned node device 
>> configuration.
>> + *
>> + * Since: 10.1.0
>> + */
>> +typedef enum {
>> +    VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE            = 1 << 0, /* 
>> dump inactive device configuration (Since: 10.1.0) */
>> +} virNodeDeviceGetXMLDescFlags;
> 
> In all of the other similar cases, this type is named 
> vir$(OBJECT)XMLFlags and the flag itself is named 
> VIR_$(OBJECT)_XML_INACTIVE. So for consistency, I'd remove the 'get' and 
> the 'desc' from the names.
> 

I disagree as all other methods that make use of flags base the flags 
names on the method name. Here are the examples:

virNodeDeviceCreateXML
virNodeDeviceCreateXMLFlags
vir Node Device Create XML
VIR_NODE_DEVICE_CREATE_XML_*

virNodeDeviceDefineXML
virNodeDeviceDefineXMLFlags
vir Node Device Define XML
VIR_NODE_DEVICE_DEFINE_XML_*

virConnectListAllNodeDevices
virConnectListAllNodeDeviceFlags
vir Connect List Node Device [All is removed]
VIR_CONNECT_LIST_NODE_DEVICES_*

These are the reasons I chose for consistency:
virNodeDeviceGetXMLDesc
virNodeDeviceGetXMLDescFlags
vir Node Device Get XML Desc
VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE


>> +
>>   char *                  virNodeDeviceGetXMLDesc (virNodeDevicePtr dev,
>>                                                    unsigned int flags);
>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>> index 9a0fc68e67..40e15ef4da 100644
>> --- a/src/conf/node_device_conf.c
>> +++ b/src/conf/node_device_conf.c
>> @@ -599,16 +599,22 @@ virNodeDeviceCapMdevAttrFormat(virBuffer *buf,
>>   static void
>>   virNodeDeviceCapMdevDefFormat(virBuffer *buf,
>> -                              const virNodeDevCapData *data)
>> +                              const virNodeDevCapData *data,
>> +                              bool defined)
>>   {
>> -    virBufferEscapeString(buf, "<type id='%s'/>\n", 
>> data->mdev.dev_config.type);
>> +    if (defined)
>> +        virBufferEscapeString(buf, "<type id='%s'/>\n", 
>> data->mdev.defined_config.type);
>> +    else
>> +        virBufferEscapeString(buf, "<type id='%s'/>\n", 
>> data->mdev.active_config.type);
>>       virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid);
>>       virBufferEscapeString(buf, "<parent_addr>%s</parent_addr>\n",
>>                             data->mdev.parent_addr);
>>       virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",
>>                         data->mdev.iommuGroupNumber);
>> -
>> -    virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.dev_config);
>> +    if (defined)
>> +        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.defined_config);
>> +    else
>> +        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.active_config);
>>   }
>>   static void
>> @@ -659,25 +665,28 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,
>>   char *
>> -virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>> +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags)
>>   {
>>       g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>>       virNodeDevCapsDef *caps;
>>       size_t i = 0;
>> +    bool inactive_state = flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>>       virBufferAddLit(&buf, "<device>\n");
>>       virBufferAdjustIndent(&buf, 2);
>>       virBufferEscapeString(&buf, "<name>%s</name>\n", def->name);
>> -    virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path);
>> -    virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n",
>> -                          def->devnode);
>> -    if (def->devlinks) {
>> -        for (i = 0; def->devlinks[i]; i++)
>> -            virBufferEscapeString(&buf, "<devnode 
>> type='link'>%s</devnode>\n",
>> -                                  def->devlinks[i]);
>> +    if (!inactive_state) {
>> +        virBufferEscapeString(&buf, "<path>%s</path>\n", 
>> def->sysfs_path);
>> +        virBufferEscapeString(&buf, "<devnode 
>> type='dev'>%s</devnode>\n",
>> +                              def->devnode);
>> +        if (def->devlinks) {
>> +            for (i = 0; def->devlinks[i]; i++)
>> +                virBufferEscapeString(&buf, "<devnode 
>> type='link'>%s</devnode>\n",
>> +                                      def->devlinks[i]);
>> +        }
>>       }
>>       virBufferEscapeString(&buf, "<parent>%s</parent>\n", def->parent);
>> -    if (def->driver) {
>> +    if (def->driver && !inactive_state) {
>>           virBufferAddLit(&buf, "<driver>\n");
>>           virBufferAdjustIndent(&buf, 2);
>>           virBufferEscapeString(&buf, "<name>%s</name>\n", def->driver);
>> @@ -738,7 +747,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>>               virBufferEscapeString(&buf, "<type>%s</type>\n", 
>> virNodeDevDRMTypeToString(data->drm.type));
>>               break;
>>           case VIR_NODE_DEV_CAP_MDEV:
>> -            virNodeDeviceCapMdevDefFormat(&buf, data);
>> +            virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state);
>>               break;
>>           case VIR_NODE_DEV_CAP_CCW_DEV:
>>               virNodeDeviceCapCCWDefFormat(&buf, data);
>> @@ -2226,7 +2235,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>>       ctxt->node = node;
>> -    if (!(mdev->dev_config.type = 
>> virXPathString("string(./type[1]/@id)", ctxt))) {
>> +    if (!(mdev->defined_config.type = 
>> virXPathString("string(./type[1]/@id)", ctxt))) {
>>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>                          _("missing type id attribute for '%1$s'"), 
>> def->name);
>>           return -1;
>> @@ -2258,7 +2267,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>>           return -1;
>>       for (i = 0; i < nattrs; i++)
>> -        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], 
>> &mdev->dev_config);
>> +        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], 
>> &mdev->defined_config);
>>       return 0;
>>   }
>> @@ -2601,11 +2610,15 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
>>           g_free(data->sg.path);
>>           break;
>>       case VIR_NODE_DEV_CAP_MDEV:
>> -        g_free(data->mdev.dev_config.type);
>> +        g_free(data->mdev.defined_config.type);
>> +        g_free(data->mdev.active_config.type);
>>           g_free(data->mdev.uuid);
>> -        for (i = 0; i < data->mdev.dev_config.nattributes; i++)
>> -            
>> virMediatedDeviceAttrFree(data->mdev.dev_config.attributes[i]);
>> -        g_free(data->mdev.dev_config.attributes);
>> +        for (i = 0; i < data->mdev.defined_config.nattributes; i++)
>> +            
>> virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
>> +        g_free(data->mdev.defined_config.attributes);
>> +        for (i = 0; i < data->mdev.active_config.nattributes; i++)
>> +            
>> virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
>> +        g_free(data->mdev.active_config.attributes);
>>           g_free(data->mdev.parent_addr);
>>           break;
>>       case VIR_NODE_DEV_CAP_CSS_DEV:
>> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
>> index f100496272..f59440dbb9 100644
>> --- a/src/conf/node_device_conf.h
>> +++ b/src/conf/node_device_conf.h
>> @@ -153,7 +153,8 @@ typedef struct _virNodeDevCapMdev virNodeDevCapMdev;
>>   struct _virNodeDevCapMdev {
>>       unsigned int iommuGroupNumber;
>>       char *uuid;
>> -    virMediatedDeviceConfig dev_config;
>> +    virMediatedDeviceConfig defined_config;
>> +    virMediatedDeviceConfig active_config;
>>       char *parent_addr;
>>       bool autostart;
>>   };
>> @@ -360,7 +361,7 @@ struct _virNodeDeviceDef {
>>   };
>>   char *
>> -virNodeDeviceDefFormat(const virNodeDeviceDef *def);
>> +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags);
>>   typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef *dev,
>> diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
>> index f0f99bc020..9cc5c6466b 100644
>> --- a/src/libvirt-nodedev.c
>> +++ b/src/libvirt-nodedev.c
>> @@ -264,7 +264,7 @@ virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>>   /**
>>    * virNodeDeviceGetXMLDesc:
>>    * @dev: pointer to the node device
>> - * @flags: extra flags; not used yet, so callers should always pass 0
>> + * @flags: bitwise-OR of virNodeDeviceGetXMLDescFlags
>>    *
>>    * Fetch an XML document describing all aspects of
>>    * the device.
>> diff --git a/src/node_device/node_device_driver.c 
>> b/src/node_device/node_device_driver.c
>> index 0c8612eb71..d67c95d68d 100644
>> --- a/src/node_device/node_device_driver.c
>> +++ b/src/node_device/node_device_driver.c
>> @@ -338,7 +338,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
>>       virNodeDeviceDef *def;
>>       char *ret = NULL;
>> -    virCheckFlags(0, NULL);
>> +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
>>       if (nodeDeviceInitWait() < 0)
>>           return NULL;
>> @@ -356,7 +356,19 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
>>       if (virNodeDeviceUpdateCaps(def) < 0)
>>           goto cleanup;
>> -    ret = virNodeDeviceDefFormat(def);
>> +    if (flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) {
>> +        if (!virNodeDeviceObjIsPersistent(obj)) {
>> +            virReportError(VIR_ERR_OPERATION_INVALID,
>> +                           _("node device '%1$s' is not persistent"),
>> +                           def->name);
>> +            goto cleanup;
>> +        }
>> +    } else {
>> +        if (!virNodeDeviceObjIsActive(obj))
>> +            flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>> +    }
>> +
>> +    ret = virNodeDeviceDefFormat(def, flags);
>>    cleanup:
>>       virNodeDeviceObjEndAPI(&obj);
>> @@ -629,19 +641,20 @@ nodeDeviceAttributesToJSON(virJSONValue *json,
>>   /* format a json string that provides configuration information 
>> about this mdev
>>    * to the mdevctl utility */
>>   static int
>> -nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf)
>> +nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf, bool 
>> defined)
> 
> As far as I can tell, you never actually call this function with 
> 'defined' set to false. It doesn't seem to be used in any of the 
> following commits either.

Correct. I did not want to hardcode it in the method.

> 
>>   {
>>       virNodeDevCapMdev *mdev = &def->caps->data.mdev;
>> +    virMediatedDeviceConfig mdev_config = defined ? 
>> mdev->defined_config : mdev->active_config;
>>       g_autoptr(virJSONValue) json = virJSONValueNewObject();
>>       const char *startval = mdev->autostart ? "auto" : "manual";
>> -    if (virJSONValueObjectAppendString(json, "mdev_type", 
>> mdev->dev_config.type) < 0)
>> +    if (virJSONValueObjectAppendString(json, "mdev_type", 
>> mdev_config.type) < 0)
>>           return -1;
>>       if (virJSONValueObjectAppendString(json, "start", startval) < 0)
>>           return -1;
>> -    if (nodeDeviceAttributesToJSON(json, mdev->dev_config) < 0)
>> +    if (nodeDeviceAttributesToJSON(json, mdev_config) < 0)
>>           return -1;
>>       *buf = virJSONValueToString(json, false);
>> @@ -760,7 +773,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
>>               return NULL;
>>           }
>> -        if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
>> +        if (nodeDeviceDefToMdevctlConfig(def, &inbuf, true) < 0) {
>>               virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                              _("couldn't convert node device def to 
>> mdevctl JSON"));
>>               return NULL;
>> @@ -1138,9 +1151,11 @@ 
>> nodeDeviceParseMdevctlAttribs(virMediatedDeviceConfig *config,
>>   static virNodeDeviceDef*
>>   nodeDeviceParseMdevctlChildDevice(const char *parent,
>> -                                  virJSONValue *json)
>> +                                  virJSONValue *json,
>> +                                  bool defined)
>>   {
>>       virNodeDevCapMdev *mdev;
>> +    virMediatedDeviceConfig *mdev_config;
>>       const char *uuid;
>>       virJSONValue *props;
>>       g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
>> @@ -1170,14 +1185,15 @@ nodeDeviceParseMdevctlChildDevice(const char 
>> *parent,
>>       child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;
>>       mdev = &child->caps->data.mdev;
>> +    mdev_config = defined ? &mdev->defined_config : 
>> &mdev->active_config;
>>       mdev->uuid = g_strdup(uuid);
>>       mdev->parent_addr = g_strdup(parent);
>> -    mdev->dev_config.type =
>> +    mdev_config->type =
>>           g_strdup(virJSONValueObjectGetString(props, "mdev_type"));
>>       start = virJSONValueObjectGetString(props, "start");
>>       mdev->autostart = STREQ_NULLABLE(start, "auto");
>> -    if (nodeDeviceParseMdevctlAttribs(&mdev->dev_config,
>> +    if (nodeDeviceParseMdevctlAttribs(mdev_config,
>>                                         virJSONValueObjectGet(props, 
>> "attrs")) < 0)
>>           return NULL;
>> @@ -1189,7 +1205,8 @@ nodeDeviceParseMdevctlChildDevice(const char 
>> *parent,
>>   int
>>   nodeDeviceParseMdevctlJSON(const char *jsonstring,
>> -                           virNodeDeviceDef ***devs)
>> +                           virNodeDeviceDef ***devs,
>> +                           bool defined)
>>   {
>>       int n;
>>       g_autoptr(virJSONValue) json_devicelist = NULL;
>> @@ -1259,7 +1276,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
>>               g_autoptr(virNodeDeviceDef) child = NULL;
>>               virJSONValue *child_obj = 
>> virJSONValueArrayGet(child_array, j);
>> -            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, 
>> child_obj))) {
>> +            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, 
>> child_obj, defined))) {
>>                   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                                  _("Unable to parse child device"));
>>                   goto error;
>> @@ -1402,7 +1419,7 @@ nodeDeviceUpdateMediatedDevice(virNodeDeviceDef 
>> *def,
>>           /* Active devices contain some additional information (e.g. 
>> sysfs
>>            * path) that is not provided by mdevctl, so re-use the 
>> existing
>>            * definition and copy over new mdev data */
>> -        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned);
>> +        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned, defined);
>>           if (was_defined && !changed) {
>>               /* if this device was already defined and the definition
>> @@ -1672,7 +1689,7 @@ virMdevctlList(bool defined,
>>           return -1;
>>       }
>> -    return nodeDeviceParseMdevctlJSON(output, devs);
>> +    return nodeDeviceParseMdevctlJSON(output, devs, defined);
>>   }
>> @@ -1831,16 +1848,24 @@ 
>> virMediatedDeviceAttrsCopy(virMediatedDeviceConfig *dst_config,
>>    * Returns true if anything was copied, else returns false */
>>   bool
>>   nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>> -                             virNodeDeviceDef *src)
>> +                             virNodeDeviceDef *src,
>> +                             bool defined)
>>   {
>>       bool ret = false;
>>       virNodeDevCapMdev *srcmdev = &src->caps->data.mdev;
>>       virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev;
>> +    virMediatedDeviceConfig *srcmdevconfig = 
>> &src->caps->data.mdev.active_config;
>> +    virMediatedDeviceConfig *dstmdevconfig = 
>> &dst->caps->data.mdev.active_config;
>> +
>> +    if (defined) {
>> +        srcmdevconfig = &src->caps->data.mdev.defined_config;
>> +        dstmdevconfig = &dst->caps->data.mdev.defined_config;
>> +    }
>> -    if (STRNEQ_NULLABLE(dstmdev->dev_config.type, 
>> srcmdev->dev_config.type)) {
>> +    if (STRNEQ_NULLABLE(dstmdevconfig->type, srcmdevconfig->type)) {
>>           ret = true;
>> -        g_free(dstmdev->dev_config.type);
>> -        dstmdev->dev_config.type = g_strdup(srcmdev->dev_config.type);
>> +        g_free(dstmdevconfig->type);
>> +        dstmdevconfig->type = g_strdup(srcmdevconfig->type);
>>       }
>>       if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) {
>> @@ -1849,7 +1874,7 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>>           dstmdev->uuid = g_strdup(srcmdev->uuid);
>>       }
>> -    if (virMediatedDeviceAttrsCopy(&dstmdev->dev_config, 
>> &srcmdev->dev_config))
>> +    if (virMediatedDeviceAttrsCopy(dstmdevconfig, srcmdevconfig))
>>           ret = true;
>>       if (dstmdev->autostart != srcmdev->autostart) {
>> diff --git a/src/node_device/node_device_driver.h 
>> b/src/node_device/node_device_driver.h
>> index c7d5e22daf..4dce7e6f17 100644
>> --- a/src/node_device/node_device_driver.h
>> +++ b/src/node_device/node_device_driver.h
>> @@ -142,7 +142,8 @@ nodeDeviceGetMdevctlListCommand(bool defined,
>>   int
>>   nodeDeviceParseMdevctlJSON(const char *jsonstring,
>> -                           virNodeDeviceDef ***devs);
>> +                           virNodeDeviceDef ***devs,
>> +                           bool defined);
>>   int
>>   nodeDeviceUpdateMediatedDevices(void);
>> @@ -154,7 +155,8 @@ nodeDeviceGenerateName(virNodeDeviceDef *def,
>>                          const char *s);
>>   bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>> -                                  virNodeDeviceDef *src);
>> +                                  virNodeDeviceDef *src,
>> +                                  bool defined);
>>   int
>>   nodeDeviceCreate(virNodeDevice *dev,
>> diff --git a/src/node_device/node_device_udev.c 
>> b/src/node_device/node_device_udev.c
>> index 254e802c50..57368a96c3 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1069,7 +1069,7 @@ udevProcessMediatedDevice(struct udev_device *dev,
>>           return -1;
>>       }
>> -    data->dev_config.type = g_path_get_basename(canonicalpath);
>> +    data->active_config.type = g_path_get_basename(canonicalpath);
>>       data->uuid = g_strdup(udev_device_get_sysname(dev));
>>       if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) < 0)
>> @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device)
>>           objdef = virNodeDeviceObjGetDef(obj);
>>           if (is_mdev)
>> -            nodeDeviceDefCopyFromMdevctl(def, objdef);
>> +            nodeDeviceDefCopyFromMdevctl(def, objdef, false);
>>           persistent = virNodeDeviceObjIsPersistent(obj);
>>           autostart = virNodeDeviceObjIsAutostart(obj);
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index ed545848af..01863233bc 100644
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -7514,12 +7514,12 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
>>       virNodeDeviceObj *obj;
>>       char *ret = NULL;
>> -    virCheckFlags(0, NULL);
>> +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
>>       if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
>>           return NULL;
>> -    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj));
>> +    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);
>>       virNodeDeviceObjEndAPI(&obj);
>>       return ret;
>> @@ -7619,7 +7619,7 @@ testNodeDeviceMockCreateVport(testDriver *driver,
>>                                                      "scsi_host11")))
>>           goto cleanup;
>> -    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy));
>> +    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy), 0);
>>       virNodeDeviceObjEndAPI(&objcopy);
>>       if (!xml)
>>           goto cleanup;
>> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
>> index e403328e5a..852d9ed6e7 100644
>> --- a/tests/nodedevmdevctltest.c
>> +++ b/tests/nodedevmdevctltest.c
>> @@ -229,13 +229,13 @@ testMdevctlParse(const void *data)
>>           return -1;
>>       }
>> -    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) {
>> +    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs, true)) < 0) {
>>           VIR_TEST_DEBUG("Unable to parse json for %s", filename);
>>           return -1;
>>       }
>>       for (i = 0; i < nmdevs; i++) {
>> -        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]);
>> +        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i], 
>> VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE);
>>           if (!devxml)
>>               goto cleanup;
>>           virBufferAddStr(&xmloutbuf, devxml);
>> diff --git 
>> a/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>> new file mode 100644
>> index 0000000000..6926559efa
>> --- /dev/null
>> +++ 
>> b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>> @@ -0,0 +1,14 @@
>> +<device>
>> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
>> +  
>> <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
>> +  <parent>css_0_0_0052</parent>
>> +  <driver>
>> +    <name>vfio_ccw_mdev</name>
>> +  </driver>
>> +  <capability type='mdev'>
>> +    <type id='vfio_ccw-io'/>
>> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
>> +    <parent_addr>0.0.0052</parent_addr>
>> +    <iommuGroup number='4'/>
>> +  </capability>
>> +</device>
>> diff --git 
>> a/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
>> new file mode 120000
>> index 0000000000..f8ec7d8a32
>> --- /dev/null
>> +++ 
>> b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
>> @@ -0,0 +1 @@
>> +mdev_3627463d_b7f0_4fea_b468_f1da537d301b.xml
>> \ No newline at end of file
>> diff --git 
>> a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>> new file mode 100644
>> index 0000000000..82c60cc065
>> --- /dev/null
>> +++ 
>> b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>> @@ -0,0 +1,10 @@
>> +<device>
>> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
>> +  
>> <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
>> +  <parent>css_0_0_0052</parent>
>> +  <capability type='mdev'>
>> +    <type id='vfio_ccw-io'/>
>> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
>> +    <iommuGroup number='4'/>
>> +  </capability>
>> +</device>
>> diff --git 
>> a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
>> new file mode 100644
>> index 0000000000..87c5ed1340
>> --- /dev/null
>> +++ 
>> b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
>> @@ -0,0 +1,9 @@
>> +<device>
>> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
>> +  <parent>css_0_0_0052</parent>
>> +  <capability type='mdev'>
>> +    <type id='vfio_ccw-io'/>
>> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
>> +    <iommuGroup number='4'/>
>> +  </capability>
>> +</device>
>> diff --git 
>> a/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
>> new file mode 120000
>> index 0000000000..f6d5789399
>> --- /dev/null
>> +++ 
>> b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
>> @@ -0,0 +1 @@
>> +mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml
>> \ No newline at end of file
>> diff --git 
>> a/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
>> new file mode 120000
>> index 0000000000..233a7506fb
>> --- /dev/null
>> +++ 
>> b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
>> @@ -0,0 +1 @@
>> +mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml
>> \ No newline at end of file
>> diff --git 
>> a/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
>> new file mode 100644
>> index 0000000000..8c4983f43c
>> --- /dev/null
>> +++ 
>> b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
>> @@ -0,0 +1,8 @@
>> +<device>
>> +  <name>mdev_ee0b88c4-f554-4dc1-809d-b2a01e8e48ad</name>
>> +  <parent>ap_matrix</parent>
>> +  <capability type='mdev'>
>> +    <type id='vfio_ap-passthrough'/>
>> +    <iommuGroup number='0'/>
>> +  </capability>
>> +</device>
>> diff --git 
>> a/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
>> new file mode 120000
>> index 0000000000..e053844177
>> --- /dev/null
>> +++ 
>> b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
>> @@ -0,0 +1 @@
>> +mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml
>> \ No newline at end of file
>> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
>> index 068ec68769..be7a5b4df9 100644
>> --- a/tests/nodedevxml2xmltest.c
>> +++ b/tests/nodedevxml2xmltest.c
>> @@ -11,14 +11,20 @@
>>   #define VIR_FROM_THIS VIR_FROM_NONE
>> +struct TestData {
>> +    const char *filename;
>> +    unsigned int flags;
>> +};
>> +
>>   static int
>> -testCompareXMLToXMLFiles(const char *xml, const char *outfile)
>> +testCompareXMLToXMLFiles(const char *xml, const char *outfile, 
>> unsigned int flags)
>>   {
>>       g_autofree char *xmlData = NULL;
>>       g_autofree char *actual = NULL;
>>       int ret = -1;
>>       virNodeDeviceDef *dev = NULL;
>>       virNodeDevCapsDef *caps;
>> +    size_t i;
>>       if (virTestLoadFile(xml, &xmlData) < 0)
>>           goto fail;
>> @@ -46,9 +52,22 @@ testCompareXMLToXMLFiles(const char *xml, const 
>> char *outfile)
>>                                              
>> data->storage.logical_block_size;
>>               }
>>           }
>> +
>> +        if (caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
>> +            !(flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)) {
>> +            data->mdev.active_config.type = 
>> g_strdup(data->mdev.defined_config.type);
>> +            for (i = 0; i < data->mdev.defined_config.nattributes; 
>> i++) {
>> +                g_autoptr(virMediatedDeviceAttr) attr = 
>> g_new0(virMediatedDeviceAttr, 1);
>> +                attr->name = 
>> g_strdup(data->mdev.defined_config.attributes[i]->name);
>> +                attr->value = 
>> g_strdup(data->mdev.defined_config.attributes[i]->value);
>> +                VIR_APPEND_ELEMENT(data->mdev.active_config.attributes,
>> +                                   data->mdev.active_config.nattributes,
>> +                                   attr);
>> +            }
>> +        }
>>       }
>> -    if (!(actual = virNodeDeviceDefFormat(dev)))
>> +    if (!(actual = virNodeDeviceDefFormat(dev, flags)))
>>           goto fail;
>>       if (virTestCompareToFile(actual, outfile) < 0)
>> @@ -65,16 +84,21 @@ static int
>>   testCompareXMLToXMLHelper(const void *data)
>>   {
>>       int result = -1;
>> +    const struct TestData *tdata = data;
>>       g_autofree char *xml = NULL;
>>       g_autofree char *outfile = NULL;
>>       xml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
>> -                          (const char *)data);
>> +                          tdata->filename);
>> -    outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", abs_srcdir,
>> -                              (const char *)data);
>> +    if (tdata->flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
>> +        outfile = 
>> g_strdup_printf("%s/nodedevxml2xmlout/%s_inactive.xml", abs_srcdir,
>> +                                  tdata->filename);
>> +    else
>> +        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", 
>> abs_srcdir,
>> +                                  tdata->filename);
>> -    result = testCompareXMLToXMLFiles(xml, outfile);
>> +    result = testCompareXMLToXMLFiles(xml, outfile, tdata->flags);
>>       return result;
>>   }
>> @@ -85,10 +109,20 @@ mymain(void)
>>   {
>>       int ret = 0;
>> +#define DO_TEST_FLAGS(desc, filename, flags) \
>> +    do { \
>> +        struct TestData data = { filename, flags }; \
>> +        if (virTestRun(desc, testCompareXMLToXMLHelper, &data) < 0) \
>> +            ret = -1; \
>> +       } \
>> +    while (0)
>> +
>>   #define DO_TEST(name) \
>> -    if (virTestRun("Node device XML-2-XML " name, \
>> -                   testCompareXMLToXMLHelper, (name)) < 0) \
>> -        ret = -1
>> +    DO_TEST_FLAGS("Node device XML-2-XML " name, name, 0)
>> +
>> +#define DO_TEST_INACTIVE(name) \
>> +    DO_TEST_FLAGS("Node device XML-2-XML INACTIVE " name, \
>> +                  name, VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
>>       DO_TEST("computer");
>>       DO_TEST("DVD_GCC_4247N");
>> @@ -121,6 +155,7 @@ mymain(void)
>>       DO_TEST("pci_0000_02_10_7_mdev_types");
>>       DO_TEST("pci_0000_42_00_0_vpd");
>>       DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
>> +    DO_TEST_INACTIVE("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
>>       DO_TEST("ccw_0_0_ffff");
>>       DO_TEST("css_0_0_ffff");
>>       DO_TEST("css_0_0_ffff_channel_dev_addr");
>> @@ -134,7 +169,13 @@ mymain(void)
>>       DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
>>       DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
>>       DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
>> +    DO_TEST_INACTIVE("mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad");
>> +    DO_TEST_INACTIVE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
>> +    DO_TEST_INACTIVE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
>> +    DO_TEST_INACTIVE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
>>       DO_TEST("hba_vport_ops");
>> +    DO_TEST("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
>> +    DO_TEST_INACTIVE("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
>>       return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>>   }
> 
> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

Thanks

> _______________________________________________
> Devel mailing list -- devel@lists.libvirt.org
> To unsubscribe send an email to devel-leave@lists.libvirt.org

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH 04/11] nodedev: add an active config to mdev

Posted by Jonathon Jongsma 8 months, 2 weeks ago

On 2/1/24 6:17 AM, Boris Fiuczynski wrote:
> On 1/31/24 22:30, Jonathon Jongsma wrote:
>> On 1/19/24 10:38 AM, Boris Fiuczynski wrote:
>>> The configuration of a defined mdev can be modified after the mdev is
>>> started. The defined configuration and the active configuration can
>>> therefore run out of sync. Handle this by storing the modifiable data
>>> which is the mdev type and attributes in two separate active and
>>> defined configurations.  mdevctl supports with callout scripts to do an
>>> attribute retrieval of started mdevs which is already implemented in
>>> libvirt.
>>>
>>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>>> ---
>>>   include/libvirt/libvirt-nodedev.h             | 11 ++++
>>>   src/conf/node_device_conf.c                   | 53 ++++++++++------
>>>   src/conf/node_device_conf.h                   |  5 +-
>>>   src/libvirt-nodedev.c                         |  2 +-
>>>   src/node_device/node_device_driver.c          | 61 +++++++++++++------
>>>   src/node_device/node_device_driver.h          |  6 +-
>>>   src/node_device/node_device_udev.c            |  4 +-
>>>   src/test/test_driver.c                        |  6 +-
>>>   tests/nodedevmdevctltest.c                    |  4 +-
>>>   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 14 +++++
>>>   ...d_b7f0_4fea_b468_f1da537d301b_inactive.xml |  1 +
>>>   ...v_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml | 10 +++
>>>   ...c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml |  9 +++
>>>   ...9_36ea_4111_8f0a_8c9a70e21366_inactive.xml |  1 +
>>>   ...9_495e_4243_ad9f_beb3f14c23d9_inactive.xml |  1 +
>>>   ...4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml |  8 +++
>>>   ...6_1ca8_49ac_b176_871d16c13076_inactive.xml |  1 +
>>>   tests/nodedevxml2xmltest.c                    | 59 +++++++++++++++---
>>>   18 files changed, 197 insertions(+), 59 deletions(-)
>>>   create mode 100644 
>>> tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>>   create mode 120000 
>>> tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
>>>   create mode 100644 
>>> tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>>   create mode 100644 
>>> tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
>>>   create mode 120000 
>>> tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
>>>   create mode 120000 
>>> tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
>>>   create mode 100644 
>>> tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
>>>   create mode 120000 
>>> tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
>>>
>>> diff --git a/include/libvirt/libvirt-nodedev.h 
>>> b/include/libvirt/libvirt-nodedev.h
>>> index 428b0d722f..d18246e2f6 100644
>>> --- a/include/libvirt/libvirt-nodedev.h
>>> +++ b/include/libvirt/libvirt-nodedev.h
>>> @@ -117,6 +117,17 @@ int                     virNodeDeviceListCaps 
>>> (virNodeDevicePtr dev,
>>>                                                     char **const names,
>>>                                                     int maxnames);
>>> +/**
>>> + * virNodeDeviceGetXMLDescFlags:
>>> + *
>>> + * Flags used to provide the state of the returned node device 
>>> configuration.
>>> + *
>>> + * Since: 10.1.0
>>> + */
>>> +typedef enum {
>>> +    VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE            = 1 << 0, /* 
>>> dump inactive device configuration (Since: 10.1.0) */
>>> +} virNodeDeviceGetXMLDescFlags;
>>
>> In all of the other similar cases, this type is named 
>> vir$(OBJECT)XMLFlags and the flag itself is named 
>> VIR_$(OBJECT)_XML_INACTIVE. So for consistency, I'd remove the 'get' 
>> and the 'desc' from the names.
>>
> 
> I disagree as all other methods that make use of flags base the flags 
> names on the method name. Here are the examples:
> 
> virNodeDeviceCreateXML
> virNodeDeviceCreateXMLFlags
> vir Node Device Create XML
> VIR_NODE_DEVICE_CREATE_XML_*
> 
> virNodeDeviceDefineXML
> virNodeDeviceDefineXMLFlags
> vir Node Device Define XML
> VIR_NODE_DEVICE_DEFINE_XML_*
> 
> virConnectListAllNodeDevices
> virConnectListAllNodeDeviceFlags
> vir Connect List Node Device [All is removed]
> VIR_CONNECT_LIST_NODE_DEVICES_*
> 
> These are the reasons I chose for consistency:
> virNodeDeviceGetXMLDesc
> virNodeDeviceGetXMLDescFlags
> vir Node Device Get XML Desc
> VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE


That's true in general, however for the *GetXMLDesc() functions, this 
pattern doesn't hold:

domain:
  - virDomainGetXMLDesc()
  - virDomainXMLFlags
    - VIR_DOMAIN_XML_INACTIVE

storage:
  - virStoragePoolGetXMLDesc()
  - virStorageXMLFlags
    - VIR_STORAGE_XML_INACTIVE

network:
  - virNetworkGetXMLDesc()
  - virNetworkXMLFlags
    - VIR_NETWORK_XML_INACTIVE

interface:
  - virInterfaceGetXMLDesc()
  - virInterfaceXMLFlags
    - VIR_INTERFACE_XML_INACTIVE

There are no types following the *GetXMLDescFlags pattern:
$ git grep XMLDescFlags include/
$


I don't feel personally strongly about this point because there are 
consistency arguments for both approaches. But I thought I'd mention it.

Jonathon


>>> +
>>>   char *                  virNodeDeviceGetXMLDesc (virNodeDevicePtr dev,
>>>                                                    unsigned int flags);
>>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
>>> index 9a0fc68e67..40e15ef4da 100644
>>> --- a/src/conf/node_device_conf.c
>>> +++ b/src/conf/node_device_conf.c
>>> @@ -599,16 +599,22 @@ virNodeDeviceCapMdevAttrFormat(virBuffer *buf,
>>>   static void
>>>   virNodeDeviceCapMdevDefFormat(virBuffer *buf,
>>> -                              const virNodeDevCapData *data)
>>> +                              const virNodeDevCapData *data,
>>> +                              bool defined)
>>>   {
>>> -    virBufferEscapeString(buf, "<type id='%s'/>\n", 
>>> data->mdev.dev_config.type);
>>> +    if (defined)
>>> +        virBufferEscapeString(buf, "<type id='%s'/>\n", 
>>> data->mdev.defined_config.type);
>>> +    else
>>> +        virBufferEscapeString(buf, "<type id='%s'/>\n", 
>>> data->mdev.active_config.type);
>>>       virBufferEscapeString(buf, "<uuid>%s</uuid>\n", data->mdev.uuid);
>>>       virBufferEscapeString(buf, "<parent_addr>%s</parent_addr>\n",
>>>                             data->mdev.parent_addr);
>>>       virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",
>>>                         data->mdev.iommuGroupNumber);
>>> -
>>> -    virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.dev_config);
>>> +    if (defined)
>>> +        virNodeDeviceCapMdevAttrFormat(buf, 
>>> &data->mdev.defined_config);
>>> +    else
>>> +        virNodeDeviceCapMdevAttrFormat(buf, &data->mdev.active_config);
>>>   }
>>>   static void
>>> @@ -659,25 +665,28 @@ virNodeDeviceCapCSSDefFormat(virBuffer *buf,
>>>   char *
>>> -virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>>> +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int flags)
>>>   {
>>>       g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
>>>       virNodeDevCapsDef *caps;
>>>       size_t i = 0;
>>> +    bool inactive_state = flags & 
>>> VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>>>       virBufferAddLit(&buf, "<device>\n");
>>>       virBufferAdjustIndent(&buf, 2);
>>>       virBufferEscapeString(&buf, "<name>%s</name>\n", def->name);
>>> -    virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path);
>>> -    virBufferEscapeString(&buf, "<devnode type='dev'>%s</devnode>\n",
>>> -                          def->devnode);
>>> -    if (def->devlinks) {
>>> -        for (i = 0; def->devlinks[i]; i++)
>>> -            virBufferEscapeString(&buf, "<devnode 
>>> type='link'>%s</devnode>\n",
>>> -                                  def->devlinks[i]);
>>> +    if (!inactive_state) {
>>> +        virBufferEscapeString(&buf, "<path>%s</path>\n", 
>>> def->sysfs_path);
>>> +        virBufferEscapeString(&buf, "<devnode 
>>> type='dev'>%s</devnode>\n",
>>> +                              def->devnode);
>>> +        if (def->devlinks) {
>>> +            for (i = 0; def->devlinks[i]; i++)
>>> +                virBufferEscapeString(&buf, "<devnode 
>>> type='link'>%s</devnode>\n",
>>> +                                      def->devlinks[i]);
>>> +        }
>>>       }
>>>       virBufferEscapeString(&buf, "<parent>%s</parent>\n", def->parent);
>>> -    if (def->driver) {
>>> +    if (def->driver && !inactive_state) {
>>>           virBufferAddLit(&buf, "<driver>\n");
>>>           virBufferAdjustIndent(&buf, 2);
>>>           virBufferEscapeString(&buf, "<name>%s</name>\n", def->driver);
>>> @@ -738,7 +747,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>>>               virBufferEscapeString(&buf, "<type>%s</type>\n", 
>>> virNodeDevDRMTypeToString(data->drm.type));
>>>               break;
>>>           case VIR_NODE_DEV_CAP_MDEV:
>>> -            virNodeDeviceCapMdevDefFormat(&buf, data);
>>> +            virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state);
>>>               break;
>>>           case VIR_NODE_DEV_CAP_CCW_DEV:
>>>               virNodeDeviceCapCCWDefFormat(&buf, data);
>>> @@ -2226,7 +2235,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>>>       ctxt->node = node;
>>> -    if (!(mdev->dev_config.type = 
>>> virXPathString("string(./type[1]/@id)", ctxt))) {
>>> +    if (!(mdev->defined_config.type = 
>>> virXPathString("string(./type[1]/@id)", ctxt))) {
>>>           virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>>                          _("missing type id attribute for '%1$s'"), 
>>> def->name);
>>>           return -1;
>>> @@ -2258,7 +2267,7 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>>>           return -1;
>>>       for (i = 0; i < nattrs; i++)
>>> -        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], 
>>> &mdev->dev_config);
>>> +        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], 
>>> &mdev->defined_config);
>>>       return 0;
>>>   }
>>> @@ -2601,11 +2610,15 @@ virNodeDevCapsDefFree(virNodeDevCapsDef *caps)
>>>           g_free(data->sg.path);
>>>           break;
>>>       case VIR_NODE_DEV_CAP_MDEV:
>>> -        g_free(data->mdev.dev_config.type);
>>> +        g_free(data->mdev.defined_config.type);
>>> +        g_free(data->mdev.active_config.type);
>>>           g_free(data->mdev.uuid);
>>> -        for (i = 0; i < data->mdev.dev_config.nattributes; i++)
>>> - virMediatedDeviceAttrFree(data->mdev.dev_config.attributes[i]);
>>> -        g_free(data->mdev.dev_config.attributes);
>>> +        for (i = 0; i < data->mdev.defined_config.nattributes; i++)
>>> + virMediatedDeviceAttrFree(data->mdev.defined_config.attributes[i]);
>>> +        g_free(data->mdev.defined_config.attributes);
>>> +        for (i = 0; i < data->mdev.active_config.nattributes; i++)
>>> + virMediatedDeviceAttrFree(data->mdev.active_config.attributes[i]);
>>> +        g_free(data->mdev.active_config.attributes);
>>>           g_free(data->mdev.parent_addr);
>>>           break;
>>>       case VIR_NODE_DEV_CAP_CSS_DEV:
>>> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
>>> index f100496272..f59440dbb9 100644
>>> --- a/src/conf/node_device_conf.h
>>> +++ b/src/conf/node_device_conf.h
>>> @@ -153,7 +153,8 @@ typedef struct _virNodeDevCapMdev virNodeDevCapMdev;
>>>   struct _virNodeDevCapMdev {
>>>       unsigned int iommuGroupNumber;
>>>       char *uuid;
>>> -    virMediatedDeviceConfig dev_config;
>>> +    virMediatedDeviceConfig defined_config;
>>> +    virMediatedDeviceConfig active_config;
>>>       char *parent_addr;
>>>       bool autostart;
>>>   };
>>> @@ -360,7 +361,7 @@ struct _virNodeDeviceDef {
>>>   };
>>>   char *
>>> -virNodeDeviceDefFormat(const virNodeDeviceDef *def);
>>> +virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int 
>>> flags);
>>>   typedef int (*virNodeDeviceDefPostParseCallback)(virNodeDeviceDef 
>>> *dev,
>>> diff --git a/src/libvirt-nodedev.c b/src/libvirt-nodedev.c
>>> index f0f99bc020..9cc5c6466b 100644
>>> --- a/src/libvirt-nodedev.c
>>> +++ b/src/libvirt-nodedev.c
>>> @@ -264,7 +264,7 @@ virNodeDeviceLookupSCSIHostByWWN(virConnectPtr conn,
>>>   /**
>>>    * virNodeDeviceGetXMLDesc:
>>>    * @dev: pointer to the node device
>>> - * @flags: extra flags; not used yet, so callers should always pass 0
>>> + * @flags: bitwise-OR of virNodeDeviceGetXMLDescFlags
>>>    *
>>>    * Fetch an XML document describing all aspects of
>>>    * the device.
>>> diff --git a/src/node_device/node_device_driver.c 
>>> b/src/node_device/node_device_driver.c
>>> index 0c8612eb71..d67c95d68d 100644
>>> --- a/src/node_device/node_device_driver.c
>>> +++ b/src/node_device/node_device_driver.c
>>> @@ -338,7 +338,7 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
>>>       virNodeDeviceDef *def;
>>>       char *ret = NULL;
>>> -    virCheckFlags(0, NULL);
>>> +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
>>>       if (nodeDeviceInitWait() < 0)
>>>           return NULL;
>>> @@ -356,7 +356,19 @@ nodeDeviceGetXMLDesc(virNodeDevicePtr device,
>>>       if (virNodeDeviceUpdateCaps(def) < 0)
>>>           goto cleanup;
>>> -    ret = virNodeDeviceDefFormat(def);
>>> +    if (flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE) {
>>> +        if (!virNodeDeviceObjIsPersistent(obj)) {
>>> +            virReportError(VIR_ERR_OPERATION_INVALID,
>>> +                           _("node device '%1$s' is not persistent"),
>>> +                           def->name);
>>> +            goto cleanup;
>>> +        }
>>> +    } else {
>>> +        if (!virNodeDeviceObjIsActive(obj))
>>> +            flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
>>> +    }
>>> +
>>> +    ret = virNodeDeviceDefFormat(def, flags);
>>>    cleanup:
>>>       virNodeDeviceObjEndAPI(&obj);
>>> @@ -629,19 +641,20 @@ nodeDeviceAttributesToJSON(virJSONValue *json,
>>>   /* format a json string that provides configuration information 
>>> about this mdev
>>>    * to the mdevctl utility */
>>>   static int
>>> -nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf)
>>> +nodeDeviceDefToMdevctlConfig(virNodeDeviceDef *def, char **buf, bool 
>>> defined)
>>
>> As far as I can tell, you never actually call this function with 
>> 'defined' set to false. It doesn't seem to be used in any of the 
>> following commits either.
> 
> Correct. I did not want to hardcode it in the method.
> 
>>
>>>   {
>>>       virNodeDevCapMdev *mdev = &def->caps->data.mdev;
>>> +    virMediatedDeviceConfig mdev_config = defined ? 
>>> mdev->defined_config : mdev->active_config;
>>>       g_autoptr(virJSONValue) json = virJSONValueNewObject();
>>>       const char *startval = mdev->autostart ? "auto" : "manual";
>>> -    if (virJSONValueObjectAppendString(json, "mdev_type", 
>>> mdev->dev_config.type) < 0)
>>> +    if (virJSONValueObjectAppendString(json, "mdev_type", 
>>> mdev_config.type) < 0)
>>>           return -1;
>>>       if (virJSONValueObjectAppendString(json, "start", startval) < 0)
>>>           return -1;
>>> -    if (nodeDeviceAttributesToJSON(json, mdev->dev_config) < 0)
>>> +    if (nodeDeviceAttributesToJSON(json, mdev_config) < 0)
>>>           return -1;
>>>       *buf = virJSONValueToString(json, false);
>>> @@ -760,7 +773,7 @@ nodeDeviceGetMdevctlCommand(virNodeDeviceDef *def,
>>>               return NULL;
>>>           }
>>> -        if (nodeDeviceDefToMdevctlConfig(def, &inbuf) < 0) {
>>> +        if (nodeDeviceDefToMdevctlConfig(def, &inbuf, true) < 0) {
>>>               virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>                              _("couldn't convert node device def to 
>>> mdevctl JSON"));
>>>               return NULL;
>>> @@ -1138,9 +1151,11 @@ 
>>> nodeDeviceParseMdevctlAttribs(virMediatedDeviceConfig *config,
>>>   static virNodeDeviceDef*
>>>   nodeDeviceParseMdevctlChildDevice(const char *parent,
>>> -                                  virJSONValue *json)
>>> +                                  virJSONValue *json,
>>> +                                  bool defined)
>>>   {
>>>       virNodeDevCapMdev *mdev;
>>> +    virMediatedDeviceConfig *mdev_config;
>>>       const char *uuid;
>>>       virJSONValue *props;
>>>       g_autoptr(virNodeDeviceDef) child = g_new0(virNodeDeviceDef, 1);
>>> @@ -1170,14 +1185,15 @@ nodeDeviceParseMdevctlChildDevice(const char 
>>> *parent,
>>>       child->caps->data.type = VIR_NODE_DEV_CAP_MDEV;
>>>       mdev = &child->caps->data.mdev;
>>> +    mdev_config = defined ? &mdev->defined_config : 
>>> &mdev->active_config;
>>>       mdev->uuid = g_strdup(uuid);
>>>       mdev->parent_addr = g_strdup(parent);
>>> -    mdev->dev_config.type =
>>> +    mdev_config->type =
>>>           g_strdup(virJSONValueObjectGetString(props, "mdev_type"));
>>>       start = virJSONValueObjectGetString(props, "start");
>>>       mdev->autostart = STREQ_NULLABLE(start, "auto");
>>> -    if (nodeDeviceParseMdevctlAttribs(&mdev->dev_config,
>>> +    if (nodeDeviceParseMdevctlAttribs(mdev_config,
>>>                                         virJSONValueObjectGet(props, 
>>> "attrs")) < 0)
>>>           return NULL;
>>> @@ -1189,7 +1205,8 @@ nodeDeviceParseMdevctlChildDevice(const char 
>>> *parent,
>>>   int
>>>   nodeDeviceParseMdevctlJSON(const char *jsonstring,
>>> -                           virNodeDeviceDef ***devs)
>>> +                           virNodeDeviceDef ***devs,
>>> +                           bool defined)
>>>   {
>>>       int n;
>>>       g_autoptr(virJSONValue) json_devicelist = NULL;
>>> @@ -1259,7 +1276,7 @@ nodeDeviceParseMdevctlJSON(const char *jsonstring,
>>>               g_autoptr(virNodeDeviceDef) child = NULL;
>>>               virJSONValue *child_obj = 
>>> virJSONValueArrayGet(child_array, j);
>>> -            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, 
>>> child_obj))) {
>>> +            if (!(child = nodeDeviceParseMdevctlChildDevice(parent, 
>>> child_obj, defined))) {
>>>                   virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>                                  _("Unable to parse child device"));
>>>                   goto error;
>>> @@ -1402,7 +1419,7 @@ nodeDeviceUpdateMediatedDevice(virNodeDeviceDef 
>>> *def,
>>>           /* Active devices contain some additional information (e.g. 
>>> sysfs
>>>            * path) that is not provided by mdevctl, so re-use the 
>>> existing
>>>            * definition and copy over new mdev data */
>>> -        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned);
>>> +        changed = nodeDeviceDefCopyFromMdevctl(olddef, owned, defined);
>>>           if (was_defined && !changed) {
>>>               /* if this device was already defined and the definition
>>> @@ -1672,7 +1689,7 @@ virMdevctlList(bool defined,
>>>           return -1;
>>>       }
>>> -    return nodeDeviceParseMdevctlJSON(output, devs);
>>> +    return nodeDeviceParseMdevctlJSON(output, devs, defined);
>>>   }
>>> @@ -1831,16 +1848,24 @@ 
>>> virMediatedDeviceAttrsCopy(virMediatedDeviceConfig *dst_config,
>>>    * Returns true if anything was copied, else returns false */
>>>   bool
>>>   nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>>> -                             virNodeDeviceDef *src)
>>> +                             virNodeDeviceDef *src,
>>> +                             bool defined)
>>>   {
>>>       bool ret = false;
>>>       virNodeDevCapMdev *srcmdev = &src->caps->data.mdev;
>>>       virNodeDevCapMdev *dstmdev = &dst->caps->data.mdev;
>>> +    virMediatedDeviceConfig *srcmdevconfig = 
>>> &src->caps->data.mdev.active_config;
>>> +    virMediatedDeviceConfig *dstmdevconfig = 
>>> &dst->caps->data.mdev.active_config;
>>> +
>>> +    if (defined) {
>>> +        srcmdevconfig = &src->caps->data.mdev.defined_config;
>>> +        dstmdevconfig = &dst->caps->data.mdev.defined_config;
>>> +    }
>>> -    if (STRNEQ_NULLABLE(dstmdev->dev_config.type, 
>>> srcmdev->dev_config.type)) {
>>> +    if (STRNEQ_NULLABLE(dstmdevconfig->type, srcmdevconfig->type)) {
>>>           ret = true;
>>> -        g_free(dstmdev->dev_config.type);
>>> -        dstmdev->dev_config.type = g_strdup(srcmdev->dev_config.type);
>>> +        g_free(dstmdevconfig->type);
>>> +        dstmdevconfig->type = g_strdup(srcmdevconfig->type);
>>>       }
>>>       if (STRNEQ_NULLABLE(dstmdev->uuid, srcmdev->uuid)) {
>>> @@ -1849,7 +1874,7 @@ nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef 
>>> *dst,
>>>           dstmdev->uuid = g_strdup(srcmdev->uuid);
>>>       }
>>> -    if (virMediatedDeviceAttrsCopy(&dstmdev->dev_config, 
>>> &srcmdev->dev_config))
>>> +    if (virMediatedDeviceAttrsCopy(dstmdevconfig, srcmdevconfig))
>>>           ret = true;
>>>       if (dstmdev->autostart != srcmdev->autostart) {
>>> diff --git a/src/node_device/node_device_driver.h 
>>> b/src/node_device/node_device_driver.h
>>> index c7d5e22daf..4dce7e6f17 100644
>>> --- a/src/node_device/node_device_driver.h
>>> +++ b/src/node_device/node_device_driver.h
>>> @@ -142,7 +142,8 @@ nodeDeviceGetMdevctlListCommand(bool defined,
>>>   int
>>>   nodeDeviceParseMdevctlJSON(const char *jsonstring,
>>> -                           virNodeDeviceDef ***devs);
>>> +                           virNodeDeviceDef ***devs,
>>> +                           bool defined);
>>>   int
>>>   nodeDeviceUpdateMediatedDevices(void);
>>> @@ -154,7 +155,8 @@ nodeDeviceGenerateName(virNodeDeviceDef *def,
>>>                          const char *s);
>>>   bool nodeDeviceDefCopyFromMdevctl(virNodeDeviceDef *dst,
>>> -                                  virNodeDeviceDef *src);
>>> +                                  virNodeDeviceDef *src,
>>> +                                  bool defined);
>>>   int
>>>   nodeDeviceCreate(virNodeDevice *dev,
>>> diff --git a/src/node_device/node_device_udev.c 
>>> b/src/node_device/node_device_udev.c
>>> index 254e802c50..57368a96c3 100644
>>> --- a/src/node_device/node_device_udev.c
>>> +++ b/src/node_device/node_device_udev.c
>>> @@ -1069,7 +1069,7 @@ udevProcessMediatedDevice(struct udev_device *dev,
>>>           return -1;
>>>       }
>>> -    data->dev_config.type = g_path_get_basename(canonicalpath);
>>> +    data->active_config.type = g_path_get_basename(canonicalpath);
>>>       data->uuid = g_strdup(udev_device_get_sysname(dev));
>>>       if ((iommugrp = virMediatedDeviceGetIOMMUGroupNum(data->uuid)) 
>>> < 0)
>>> @@ -1572,7 +1572,7 @@ udevAddOneDevice(struct udev_device *device)
>>>           objdef = virNodeDeviceObjGetDef(obj);
>>>           if (is_mdev)
>>> -            nodeDeviceDefCopyFromMdevctl(def, objdef);
>>> +            nodeDeviceDefCopyFromMdevctl(def, objdef, false);
>>>           persistent = virNodeDeviceObjIsPersistent(obj);
>>>           autostart = virNodeDeviceObjIsAutostart(obj);
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index ed545848af..01863233bc 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -7514,12 +7514,12 @@ testNodeDeviceGetXMLDesc(virNodeDevicePtr dev,
>>>       virNodeDeviceObj *obj;
>>>       char *ret = NULL;
>>> -    virCheckFlags(0, NULL);
>>> +    virCheckFlags(VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE, NULL);
>>>       if (!(obj = testNodeDeviceObjFindByName(driver, dev->name)))
>>>           return NULL;
>>> -    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj));
>>> +    ret = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(obj), flags);
>>>       virNodeDeviceObjEndAPI(&obj);
>>>       return ret;
>>> @@ -7619,7 +7619,7 @@ testNodeDeviceMockCreateVport(testDriver *driver,
>>>                                                      "scsi_host11")))
>>>           goto cleanup;
>>> -    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy));
>>> +    xml = virNodeDeviceDefFormat(virNodeDeviceObjGetDef(objcopy), 0);
>>>       virNodeDeviceObjEndAPI(&objcopy);
>>>       if (!xml)
>>>           goto cleanup;
>>> diff --git a/tests/nodedevmdevctltest.c b/tests/nodedevmdevctltest.c
>>> index e403328e5a..852d9ed6e7 100644
>>> --- a/tests/nodedevmdevctltest.c
>>> +++ b/tests/nodedevmdevctltest.c
>>> @@ -229,13 +229,13 @@ testMdevctlParse(const void *data)
>>>           return -1;
>>>       }
>>> -    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs)) < 0) {
>>> +    if ((nmdevs = nodeDeviceParseMdevctlJSON(buf, &mdevs, true)) < 0) {
>>>           VIR_TEST_DEBUG("Unable to parse json for %s", filename);
>>>           return -1;
>>>       }
>>>       for (i = 0; i < nmdevs; i++) {
>>> -        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i]);
>>> +        g_autofree char *devxml = virNodeDeviceDefFormat(mdevs[i], 
>>> VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE);
>>>           if (!devxml)
>>>               goto cleanup;
>>>           virBufferAddStr(&xmloutbuf, devxml);
>>> diff --git 
>>> a/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>> new file mode 100644
>>> index 0000000000..6926559efa
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevschemadata/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>> @@ -0,0 +1,14 @@
>>> +<device>
>>> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
>>> + 
>>> <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
>>> +  <parent>css_0_0_0052</parent>
>>> +  <driver>
>>> +    <name>vfio_ccw_mdev</name>
>>> +  </driver>
>>> +  <capability type='mdev'>
>>> +    <type id='vfio_ccw-io'/>
>>> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
>>> +    <parent_addr>0.0.0052</parent_addr>
>>> +    <iommuGroup number='4'/>
>>> +  </capability>
>>> +</device>
>>> diff --git 
>>> a/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
>>> new file mode 120000
>>> index 0000000000..f8ec7d8a32
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevxml2xmlout/mdev_3627463d_b7f0_4fea_b468_f1da537d301b_inactive.xml
>>> @@ -0,0 +1 @@
>>> +mdev_3627463d_b7f0_4fea_b468_f1da537d301b.xml
>>> \ No newline at end of file
>>> diff --git 
>>> a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>> new file mode 100644
>>> index 0000000000..82c60cc065
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c.xml
>>> @@ -0,0 +1,10 @@
>>> +<device>
>>> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
>>> + 
>>> <path>/sys/devices/css0/0.0.0052/c60cc60c-c60c-c60c-c60c-c60cc60cc60c</path>
>>> +  <parent>css_0_0_0052</parent>
>>> +  <capability type='mdev'>
>>> +    <type id='vfio_ccw-io'/>
>>> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
>>> +    <iommuGroup number='4'/>
>>> +  </capability>
>>> +</device>
>>> diff --git 
>>> a/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
>>> new file mode 100644
>>> index 0000000000..87c5ed1340
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevxml2xmlout/mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_inactive.xml
>>> @@ -0,0 +1,9 @@
>>> +<device>
>>> +  <name>mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0052</name>
>>> +  <parent>css_0_0_0052</parent>
>>> +  <capability type='mdev'>
>>> +    <type id='vfio_ccw-io'/>
>>> +    <uuid>c60cc60c-c60c-c60c-c60c-c60cc60cc60c</uuid>
>>> +    <iommuGroup number='4'/>
>>> +  </capability>
>>> +</device>
>>> diff --git 
>>> a/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
>>> new file mode 120000
>>> index 0000000000..f6d5789399
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevxml2xmlout/mdev_d069d019_36ea_4111_8f0a_8c9a70e21366_inactive.xml
>>> @@ -0,0 +1 @@
>>> +mdev_d069d019_36ea_4111_8f0a_8c9a70e21366.xml
>>> \ No newline at end of file
>>> diff --git 
>>> a/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
>>> new file mode 120000
>>> index 0000000000..233a7506fb
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevxml2xmlout/mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9_inactive.xml
>>> @@ -0,0 +1 @@
>>> +mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9.xml
>>> \ No newline at end of file
>>> diff --git 
>>> a/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
>>> new file mode 100644
>>> index 0000000000..8c4983f43c
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevxml2xmlout/mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad_inactive.xml
>>> @@ -0,0 +1,8 @@
>>> +<device>
>>> +  <name>mdev_ee0b88c4-f554-4dc1-809d-b2a01e8e48ad</name>
>>> +  <parent>ap_matrix</parent>
>>> +  <capability type='mdev'>
>>> +    <type id='vfio_ap-passthrough'/>
>>> +    <iommuGroup number='0'/>
>>> +  </capability>
>>> +</device>
>>> diff --git 
>>> a/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
>>> new file mode 120000
>>> index 0000000000..e053844177
>>> --- /dev/null
>>> +++ 
>>> b/tests/nodedevxml2xmlout/mdev_fedc4916_1ca8_49ac_b176_871d16c13076_inactive.xml
>>> @@ -0,0 +1 @@
>>> +mdev_fedc4916_1ca8_49ac_b176_871d16c13076.xml
>>> \ No newline at end of file
>>> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c
>>> index 068ec68769..be7a5b4df9 100644
>>> --- a/tests/nodedevxml2xmltest.c
>>> +++ b/tests/nodedevxml2xmltest.c
>>> @@ -11,14 +11,20 @@
>>>   #define VIR_FROM_THIS VIR_FROM_NONE
>>> +struct TestData {
>>> +    const char *filename;
>>> +    unsigned int flags;
>>> +};
>>> +
>>>   static int
>>> -testCompareXMLToXMLFiles(const char *xml, const char *outfile)
>>> +testCompareXMLToXMLFiles(const char *xml, const char *outfile, 
>>> unsigned int flags)
>>>   {
>>>       g_autofree char *xmlData = NULL;
>>>       g_autofree char *actual = NULL;
>>>       int ret = -1;
>>>       virNodeDeviceDef *dev = NULL;
>>>       virNodeDevCapsDef *caps;
>>> +    size_t i;
>>>       if (virTestLoadFile(xml, &xmlData) < 0)
>>>           goto fail;
>>> @@ -46,9 +52,22 @@ testCompareXMLToXMLFiles(const char *xml, const 
>>> char *outfile)
>>> data->storage.logical_block_size;
>>>               }
>>>           }
>>> +
>>> +        if (caps->data.type == VIR_NODE_DEV_CAP_MDEV &&
>>> +            !(flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)) {
>>> +            data->mdev.active_config.type = 
>>> g_strdup(data->mdev.defined_config.type);
>>> +            for (i = 0; i < data->mdev.defined_config.nattributes; 
>>> i++) {
>>> +                g_autoptr(virMediatedDeviceAttr) attr = 
>>> g_new0(virMediatedDeviceAttr, 1);
>>> +                attr->name = 
>>> g_strdup(data->mdev.defined_config.attributes[i]->name);
>>> +                attr->value = 
>>> g_strdup(data->mdev.defined_config.attributes[i]->value);
>>> +                VIR_APPEND_ELEMENT(data->mdev.active_config.attributes,
>>> +                                   
>>> data->mdev.active_config.nattributes,
>>> +                                   attr);
>>> +            }
>>> +        }
>>>       }
>>> -    if (!(actual = virNodeDeviceDefFormat(dev)))
>>> +    if (!(actual = virNodeDeviceDefFormat(dev, flags)))
>>>           goto fail;
>>>       if (virTestCompareToFile(actual, outfile) < 0)
>>> @@ -65,16 +84,21 @@ static int
>>>   testCompareXMLToXMLHelper(const void *data)
>>>   {
>>>       int result = -1;
>>> +    const struct TestData *tdata = data;
>>>       g_autofree char *xml = NULL;
>>>       g_autofree char *outfile = NULL;
>>>       xml = g_strdup_printf("%s/nodedevschemadata/%s.xml", abs_srcdir,
>>> -                          (const char *)data);
>>> +                          tdata->filename);
>>> -    outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", 
>>> abs_srcdir,
>>> -                              (const char *)data);
>>> +    if (tdata->flags & VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
>>> +        outfile = 
>>> g_strdup_printf("%s/nodedevxml2xmlout/%s_inactive.xml", abs_srcdir,
>>> +                                  tdata->filename);
>>> +    else
>>> +        outfile = g_strdup_printf("%s/nodedevxml2xmlout/%s.xml", 
>>> abs_srcdir,
>>> +                                  tdata->filename);
>>> -    result = testCompareXMLToXMLFiles(xml, outfile);
>>> +    result = testCompareXMLToXMLFiles(xml, outfile, tdata->flags);
>>>       return result;
>>>   }
>>> @@ -85,10 +109,20 @@ mymain(void)
>>>   {
>>>       int ret = 0;
>>> +#define DO_TEST_FLAGS(desc, filename, flags) \
>>> +    do { \
>>> +        struct TestData data = { filename, flags }; \
>>> +        if (virTestRun(desc, testCompareXMLToXMLHelper, &data) < 0) \
>>> +            ret = -1; \
>>> +       } \
>>> +    while (0)
>>> +
>>>   #define DO_TEST(name) \
>>> -    if (virTestRun("Node device XML-2-XML " name, \
>>> -                   testCompareXMLToXMLHelper, (name)) < 0) \
>>> -        ret = -1
>>> +    DO_TEST_FLAGS("Node device XML-2-XML " name, name, 0)
>>> +
>>> +#define DO_TEST_INACTIVE(name) \
>>> +    DO_TEST_FLAGS("Node device XML-2-XML INACTIVE " name, \
>>> +                  name, VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE)
>>>       DO_TEST("computer");
>>>       DO_TEST("DVD_GCC_4247N");
>>> @@ -121,6 +155,7 @@ mymain(void)
>>>       DO_TEST("pci_0000_02_10_7_mdev_types");
>>>       DO_TEST("pci_0000_42_00_0_vpd");
>>>       DO_TEST("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
>>> +    DO_TEST_INACTIVE("mdev_3627463d_b7f0_4fea_b468_f1da537d301b");
>>>       DO_TEST("ccw_0_0_ffff");
>>>       DO_TEST("css_0_0_ffff");
>>>       DO_TEST("css_0_0_ffff_channel_dev_addr");
>>> @@ -134,7 +169,13 @@ mymain(void)
>>>       DO_TEST("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
>>>       DO_TEST("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
>>>       DO_TEST("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
>>> +    DO_TEST_INACTIVE("mdev_ee0b88c4_f554_4dc1_809d_b2a01e8e48ad");
>>> +    DO_TEST_INACTIVE("mdev_d069d019_36ea_4111_8f0a_8c9a70e21366");
>>> +    DO_TEST_INACTIVE("mdev_d2441d39_495e_4243_ad9f_beb3f14c23d9");
>>> +    DO_TEST_INACTIVE("mdev_fedc4916_1ca8_49ac_b176_871d16c13076");
>>>       DO_TEST("hba_vport_ops");
>>> +    DO_TEST("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
>>> +    DO_TEST_INACTIVE("mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c");
>>>       return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
>>>   }
>>
>> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
> 
> Thanks
> 
>> _______________________________________________
>> Devel mailing list -- devel@lists.libvirt.org
>> To unsubscribe send an email to devel-leave@lists.libvirt.org
> 
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH 04/11] nodedev: add an active config to mdev

Posted by Boris Fiuczynski 8 months, 2 weeks ago

On 2/1/24 17:16, Jonathon Jongsma wrote:
>>>> +/**
>>>> + * virNodeDeviceGetXMLDescFlags:
>>>> + *
>>>> + * Flags used to provide the state of the returned node device 
>>>> configuration.
>>>> + *
>>>> + * Since: 10.1.0
>>>> + */
>>>> +typedef enum {
>>>> +    VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE            = 1 << 0, /* 
>>>> dump inactive device configuration (Since: 10.1.0) */
>>>> +} virNodeDeviceGetXMLDescFlags;
>>>
>>> In all of the other similar cases, this type is named 
>>> vir$(OBJECT)XMLFlags and the flag itself is named 
>>> VIR_$(OBJECT)_XML_INACTIVE. So for consistency, I'd remove the 'get' 
>>> and the 'desc' from the names.
>>>
>>
>> I disagree as all other methods that make use of flags base the flags 
>> names on the method name. Here are the examples:
>>
>> virNodeDeviceCreateXML
>> virNodeDeviceCreateXMLFlags
>> vir Node Device Create XML
>> VIR_NODE_DEVICE_CREATE_XML_*
>>
>> virNodeDeviceDefineXML
>> virNodeDeviceDefineXMLFlags
>> vir Node Device Define XML
>> VIR_NODE_DEVICE_DEFINE_XML_*
>>
>> virConnectListAllNodeDevices
>> virConnectListAllNodeDeviceFlags
>> vir Connect List Node Device [All is removed]
>> VIR_CONNECT_LIST_NODE_DEVICES_*
>>
>> These are the reasons I chose for consistency:
>> virNodeDeviceGetXMLDesc
>> virNodeDeviceGetXMLDescFlags
>> vir Node Device Get XML Desc
>> VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE
> 
> 
> That's true in general, however for the *GetXMLDesc() functions, this 
> pattern doesn't hold:
> 
> domain:
>   - virDomainGetXMLDesc()
>   - virDomainXMLFlags
>     - VIR_DOMAIN_XML_INACTIVE
> 
> storage:
>   - virStoragePoolGetXMLDesc()
>   - virStorageXMLFlags
>     - VIR_STORAGE_XML_INACTIVE
> 
> network:
>   - virNetworkGetXMLDesc()
>   - virNetworkXMLFlags
>     - VIR_NETWORK_XML_INACTIVE
> 
> interface:
>   - virInterfaceGetXMLDesc()
>   - virInterfaceXMLFlags
>     - VIR_INTERFACE_XML_INACTIVE
> 
> There are no types following the *GetXMLDescFlags pattern:
> $ git grep XMLDescFlags include/
> $
> 
> 
> I don't feel personally strongly about this point because there are 
> consistency arguments for both approaches. But I thought I'd mention it.
> 
> Jonathon
> 

Ok, following the cross driver pattern seems reasonable. I will change 
it to prevent a deviation from the exceptional GetXMLDesc pattern. :D
-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org