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

[Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>

Thanks

> _______________________________________________
> Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
> To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx

--
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@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx