[libvirt PATCH 2/6] nodedev: add support for mdev attributes
Michal Privoznik
mprivozn at redhat.com
Mon May 11 13:28:00 UTC 2020
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 at 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
More information about the libvir-list
mailing list