Mediated devices support arbitrary vendor-specific attributes that can
be attached to a mediated device. These attributes are ordered, and are
written to sysfs in order after a device is created. This patch adds
support for these attributes to the mdev data types and XML schema.

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 docs/schemas/nodedev.rng    |  6 ++++
 src/conf/node_device_conf.c | 58 +++++++++++++++++++++++++++++++++++--
 src/conf/node_device_conf.h |  2 ++
 src/libvirt_private.syms    |  2 ++
 src/util/virmdev.c          | 12 ++++++++
 src/util/virmdev.h          | 11 +++++++
 6 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
index fe6ffa0b53..a1ce09af54 100644
--- a/docs/schemas/nodedev.rng
+++ b/docs/schemas/nodedev.rng
@@ -634,6 +634,12 @@
         <ref name='unsignedInt'/>
       </attribute>
     </element>
+    <zeroOrMore>
+      <element name="attr">
+        <attribute name="name"/>
+        <attribute name="value"/>
+      </element>
+    </zeroOrMore>
   </define>
 
   <define name='capccwdev'>
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 4cf5b6e3d7..8cffe48d23 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -500,6 +500,20 @@ virNodeDeviceCapStorageDefFormat(virBufferPtr buf,
         virBufferAddLit(buf, "<capability type='hotpluggable'/>\n");
 }
 
+static void
+virNodeDeviceCapMdevDefFormat(virBufferPtr buf,
+                              const virNodeDevCapData *data)
+{
+    size_t i;
+    virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.type);
+    virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",
+                      data->mdev.iommuGroupNumber);
+    for (i = 0; i < data->mdev.nattributes; i++) {
+        virMediatedDeviceAttrPtr attr = data->mdev.attributes[i];
+        virBufferAsprintf(buf, "<attribute name='%s' value='%s'/>\n",
+                          attr->name, attr->value);
+    }
+}
 
 char *
 virNodeDeviceDefFormat(const virNodeDeviceDef *def)
@@ -583,9 +597,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
             virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type));
             break;
         case VIR_NODE_DEV_CAP_MDEV:
-            virBufferEscapeString(&buf, "<type id='%s'/>\n", data->mdev.type);
-            virBufferAsprintf(&buf, "<iommuGroup number='%u'/>\n",
-                              data->mdev.iommuGroupNumber);
+            virNodeDeviceCapMdevDefFormat(&buf, data);
             break;
         case VIR_NODE_DEV_CAP_CCW_DEV:
             virBufferAsprintf(&buf, "<cssid>0x%x</cssid>\n",
@@ -1788,6 +1800,35 @@ virNodeDevCapSystemParseXML(xmlXPathContextPtr ctxt,
     return ret;
 }
 
+static int
+virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt,
+                                   xmlNodePtr node,
+                                   virNodeDevCapMdevPtr mdev)
+{
+    xmlNodePtr orig_node = node;
+    ctxt->node = node;
+    int ret = -1;
+
+    g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew();
+    attr->name = virXPathString("string(./@name)", ctxt);
+    attr->value = virXPathString("string(./@value)", ctxt);
+    if (!attr->name || !attr->value) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("mdev attribute missing name or value"));
+        goto cleanup;
+    }
+
+    if (VIR_EXPAND_N(mdev->attributes, mdev->nattributes, 1) < 0)
+        goto cleanup;
+
+    mdev->attributes[mdev->nattributes - 1] = g_steal_pointer(&attr);
+
+    ret = 0;
+
+ cleanup:
+    ctxt->node = orig_node;
+    return ret;
+}
 
 static int
 virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
@@ -1797,6 +1838,9 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
 {
     xmlNodePtr orignode;
     int ret = -1;
+    int nattrs = 0;
+    xmlNodePtr *attrs;
+    size_t i;
 
     orignode = ctxt->node;
     ctxt->node = node;
@@ -1815,6 +1859,11 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
                                       "'%s'")) < 0)
         goto out;
 
+    if ((nattrs = virXPathNodeSet("./attribute", ctxt, &attrs)) < 0)
+        goto out;
+    for (i = 0; i < nattrs; i++)
+        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], mdev);
+
     ret = 0;
  out:
     ctxt->node = orignode;
@@ -2205,6 +2254,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
         break;
     case VIR_NODE_DEV_CAP_MDEV:
         VIR_FREE(data->mdev.type);
+        for (i = 0; i < data->mdev.nattributes; i++)
+            virMediatedDeviceAttrFree(data->mdev.attributes[i]);
+        VIR_FREE(data->mdev.attributes);
         break;
     case VIR_NODE_DEV_CAP_MDEV_TYPES:
     case VIR_NODE_DEV_CAP_DRM:
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 9e4b0847fb..e3e1e788d4 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -141,6 +141,8 @@ typedef virNodeDevCapMdev *virNodeDevCapMdevPtr;
 struct _virNodeDevCapMdev {
     char *type;
     unsigned int iommuGroupNumber;
+    virMediatedDeviceAttrPtr *attributes;
+    size_t nattributes;
 };
 
 typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 935ef7303b..94b206aa21 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2482,6 +2482,8 @@ virMacMapRemove;
 virMacMapWriteFile;
 
 # util/virmdev.h
+virMediatedDeviceAttrFree;
+virMediatedDeviceAttrNew;
 virMediatedDeviceFree;
 virMediatedDeviceGetIOMMUGroupDev;
 virMediatedDeviceGetIOMMUGroupNum;
diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index c2499c0a20..6c37eae00b 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -518,3 +518,15 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
 
     return 0;
 }
+
+virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void)
+{
+    return g_new0(virMediatedDeviceAttr, 1);
+}
+
+void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr)
+{
+    g_free(attr->name);
+    g_free(attr->value);
+    g_free(attr);
+}
diff --git a/src/util/virmdev.h b/src/util/virmdev.h
index 51f7f608a2..eb167ccb48 100644
--- a/src/util/virmdev.h
+++ b/src/util/virmdev.h
@@ -37,6 +37,17 @@ typedef struct _virMediatedDevice virMediatedDevice;
 typedef virMediatedDevice *virMediatedDevicePtr;
 typedef struct _virMediatedDeviceList virMediatedDeviceList;
 typedef virMediatedDeviceList *virMediatedDeviceListPtr;
+typedef struct _virMediatedDeviceAttr virMediatedDeviceAttr;
+typedef virMediatedDeviceAttr *virMediatedDeviceAttrPtr;
+
+struct _virMediatedDeviceAttr {
+    char *name;
+    char *value;
+};
+
+virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void);
+void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceAttr, virMediatedDeviceAttrFree);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceList, virObjectUnref);
 
-- 
2.21.1

On 4/30/20 11:42 PM, Jonathon Jongsma wrote:
> Mediated devices support arbitrary vendor-specific attributes that can
> be attached to a mediated device. These attributes are ordered, and are
> written to sysfs in order after a device is created. This patch adds
> support for these attributes to the mdev data types and XML schema.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>   docs/schemas/nodedev.rng    |  6 ++++
>   src/conf/node_device_conf.c | 58 +++++++++++++++++++++++++++++++++++--
>   src/conf/node_device_conf.h |  2 ++
>   src/libvirt_private.syms    |  2 ++
>   src/util/virmdev.c          | 12 ++++++++
>   src/util/virmdev.h          | 11 +++++++
>   6 files changed, 88 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng
> index fe6ffa0b53..a1ce09af54 100644
> --- a/docs/schemas/nodedev.rng
> +++ b/docs/schemas/nodedev.rng
> @@ -634,6 +634,12 @@
>           <ref name='unsignedInt'/>
>         </attribute>
>       </element>
> +    <zeroOrMore>
> +      <element name="attr">
> +        <attribute name="name"/>
> +        <attribute name="value"/>
> +      </element>
> +    </zeroOrMore>
>     </define>
>   
>     <define name='capccwdev'>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index 4cf5b6e3d7..8cffe48d23 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -500,6 +500,20 @@ virNodeDeviceCapStorageDefFormat(virBufferPtr buf,
>           virBufferAddLit(buf, "<capability type='hotpluggable'/>\n");
>   }
>   
> +static void
> +virNodeDeviceCapMdevDefFormat(virBufferPtr buf,
> +                              const virNodeDevCapData *data)
> +{
> +    size_t i;

We like declarations separated by an empty line from the rest of the code.

> +    virBufferEscapeString(buf, "<type id='%s'/>\n", data->mdev.type);
> +    virBufferAsprintf(buf, "<iommuGroup number='%u'/>\n",
> +                      data->mdev.iommuGroupNumber);
> +    for (i = 0; i < data->mdev.nattributes; i++) {
> +        virMediatedDeviceAttrPtr attr = data->mdev.attributes[i];
> +        virBufferAsprintf(buf, "<attribute name='%s' value='%s'/>\n",
> +                          attr->name, attr->value);
> +    }
> +}
>   
>   char *
>   virNodeDeviceDefFormat(const virNodeDeviceDef *def)
> @@ -583,9 +597,7 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def)
>               virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type));
>               break;
>           case VIR_NODE_DEV_CAP_MDEV:
> -            virBufferEscapeString(&buf, "<type id='%s'/>\n", data->mdev.type);
> -            virBufferAsprintf(&buf, "<iommuGroup number='%u'/>\n",
> -                              data->mdev.iommuGroupNumber);
> +            virNodeDeviceCapMdevDefFormat(&buf, data);
>               break;
>           case VIR_NODE_DEV_CAP_CCW_DEV:
>               virBufferAsprintf(&buf, "<cssid>0x%x</cssid>\n",
> @@ -1788,6 +1800,35 @@ virNodeDevCapSystemParseXML(xmlXPathContextPtr ctxt,
>       return ret;
>   }
>   
> +static int
> +virNodeDevCapMdevAttributeParseXML(xmlXPathContextPtr ctxt,
> +                                   xmlNodePtr node,
> +                                   virNodeDevCapMdevPtr mdev)
> +{
> +    xmlNodePtr orig_node = node;
> +    ctxt->node = node;
> +    int ret = -1;
> +
> +    g_autoptr(virMediatedDeviceAttr) attr = virMediatedDeviceAttrNew();

Here I'd move the empty line below this @attr declaration.

> +    attr->name = virXPathString("string(./@name)", ctxt);
> +    attr->value = virXPathString("string(./@value)", ctxt);
> +    if (!attr->name || !attr->value) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("mdev attribute missing name or value"));
> +        goto cleanup;
> +    }
> +
> +    if (VIR_EXPAND_N(mdev->attributes, mdev->nattributes, 1) < 0)
> +        goto cleanup;
> +
> +    mdev->attributes[mdev->nattributes - 1] = g_steal_pointer(&attr);

We have VIR_APPEND_ELEMENT() which would set attr = NULL on success.

> +
> +    ret = 0;
> +
> + cleanup:
> +    ctxt->node = orig_node;
> +    return ret;
> +}
>   
>   static int
>   virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
> @@ -1797,6 +1838,9 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>   {
>       xmlNodePtr orignode;
>       int ret = -1;
> +    int nattrs = 0;
> +    xmlNodePtr *attrs;
> +    size_t i;
>   
>       orignode = ctxt->node;
>       ctxt->node = node;
> @@ -1815,6 +1859,11 @@ virNodeDevCapMdevParseXML(xmlXPathContextPtr ctxt,
>                                         "'%s'")) < 0)
>           goto out;
>   
> +    if ((nattrs = virXPathNodeSet("./attribute", ctxt, &attrs)) < 0)
> +        goto out;

This allocates @attrs, so they must be VIR_FREE()-d or g_autofree()-d on 
return from the function. And to avoid freeing uninitialized pointer, 
@attrs should be set to NULL.

> +    for (i = 0; i < nattrs; i++)
> +        virNodeDevCapMdevAttributeParseXML(ctxt, attrs[i], mdev);

This needs to have its retval checked, because otherwise we would accept 
incomplete XML (e.g. with missing attribute name).

> +
>       ret = 0;
>    out:
>       ctxt->node = orignode;
> @@ -2205,6 +2254,9 @@ virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
>           break;
>       case VIR_NODE_DEV_CAP_MDEV:
>           VIR_FREE(data->mdev.type);
> +        for (i = 0; i < data->mdev.nattributes; i++)
> +            virMediatedDeviceAttrFree(data->mdev.attributes[i]);
> +        VIR_FREE(data->mdev.attributes);
>           break;
>       case VIR_NODE_DEV_CAP_MDEV_TYPES:
>       case VIR_NODE_DEV_CAP_DRM:
> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
> index 9e4b0847fb..e3e1e788d4 100644
> --- a/src/conf/node_device_conf.h
> +++ b/src/conf/node_device_conf.h
> @@ -141,6 +141,8 @@ typedef virNodeDevCapMdev *virNodeDevCapMdevPtr;
>   struct _virNodeDevCapMdev {
>       char *type;
>       unsigned int iommuGroupNumber;
> +    virMediatedDeviceAttrPtr *attributes;
> +    size_t nattributes;
>   };
>   
>   typedef struct _virNodeDevCapPCIDev virNodeDevCapPCIDev;
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 935ef7303b..94b206aa21 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2482,6 +2482,8 @@ virMacMapRemove;
>   virMacMapWriteFile;
>   
>   # util/virmdev.h
> +virMediatedDeviceAttrFree;
> +virMediatedDeviceAttrNew;
>   virMediatedDeviceFree;
>   virMediatedDeviceGetIOMMUGroupDev;
>   virMediatedDeviceGetIOMMUGroupNum;
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index c2499c0a20..6c37eae00b 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -518,3 +518,15 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath,
>   
>       return 0;
>   }
> +
> +virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void)
> +{
> +    return g_new0(virMediatedDeviceAttr, 1);
> +}
> +
> +void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr)
> +{

All free functions must accept NULL. Because only then it's safe to use 
g_autoptr() in non-trivial functions.

> +    g_free(attr->name);
> +    g_free(attr->value);
> +    g_free(attr);
> +}
> diff --git a/src/util/virmdev.h b/src/util/virmdev.h
> index 51f7f608a2..eb167ccb48 100644
> --- a/src/util/virmdev.h
> +++ b/src/util/virmdev.h
> @@ -37,6 +37,17 @@ typedef struct _virMediatedDevice virMediatedDevice;
>   typedef virMediatedDevice *virMediatedDevicePtr;
>   typedef struct _virMediatedDeviceList virMediatedDeviceList;
>   typedef virMediatedDeviceList *virMediatedDeviceListPtr;
> +typedef struct _virMediatedDeviceAttr virMediatedDeviceAttr;
> +typedef virMediatedDeviceAttr *virMediatedDeviceAttrPtr;
> +
> +struct _virMediatedDeviceAttr {
> +    char *name;
> +    char *value;
> +};
> +
> +virMediatedDeviceAttrPtr virMediatedDeviceAttrNew(void);
> +void virMediatedDeviceAttrFree(virMediatedDeviceAttrPtr attr);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceAttr, virMediatedDeviceAttrFree);
>   
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMediatedDeviceList, virObjectUnref);
>   
> 

Michal