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

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

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


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@xxxxxxxxxx>
>
> Thanks
>
> > _______________________________________________
> > Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
> > To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx