[libvirt] [PATCH 0/5] Add rendernode selection support

From: Marc-André Lureau <marcandre.lureau@redhat.com> Hi, The following series implements DRM nodedev, exposes the devices /dev path and symlinks, and allows to configure qemu to use a specific DRM rendernode. (also included a small unrelated cosmetic change) thanks! Marc-André Lureau (5): nodedev: fix extra space in dump nodedev: add <devnode> paths nodedev: parse <path> nodedev: add drm capability qemu: add rendernode argument docs/formatdomain.html.in | 7 +- docs/formatnode.html.in | 16 ++++ docs/schemas/domaincommon.rng | 5 + docs/schemas/nodedev.rng | 30 ++++++ src/conf/domain_conf.c | 28 +++++- src/conf/domain_conf.h | 1 + src/conf/node_device_conf.c | 102 ++++++++++++++++++++- src/conf/node_device_conf.h | 27 ++++++ src/node_device/node_device_driver.c | 1 + src/node_device/node_device_udev.c | 72 +++++++++++++++ src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 10 ++ tests/nodedevschemadata/drm_renderD129.xml | 10 ++ ...ge_serial_3600c0ff000d7a2a5d463ff4902000000.xml | 4 + tests/nodedevxml2xmltest.c | 2 + .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +- .../qemuxml2argv-video-virtio-gpu-spice-gl.xml | 2 +- tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-video-virtio-gpu-spice-gl.xml | 2 +- 20 files changed, 315 insertions(+), 10 deletions(-) create mode 100644 tests/nodedevschemadata/drm_renderD129.xml -- 2.11.0.295.gd7dffce1c.dirty

From: Marc-André Lureau <marcandre.lureau@redhat.com> This is a cosmetic change, shouldn't change XML parsing, and doesn't break any test. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/conf/node_device_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4d3268f12..6163fd5ed 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -678,7 +678,7 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) data->storage.num_blocks); } if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE) - virBufferAddLit(&buf, "<capability type='hotpluggable' />\n"); + virBufferAddLit(&buf, "<capability type='hotpluggable'/>\n"); break; case VIR_NODE_DEV_CAP_SCSI_GENERIC: virBufferEscapeString(&buf, "<char>%s</char>\n", -- 2.11.0.295.gd7dffce1c.dirty

On Wed, Feb 15, 2017 at 01:04:09AM +0400, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This is a cosmetic change, shouldn't change XML parsing, and doesn't break any test.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/conf/node_device_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

From: Marc-André Lureau <marcandre.lureau@redhat.com> Add new <devnode> top-level <device> element, that list the associated /dev files. Distinguish the main /dev name from symlinks with a 'type' attribute of value 'dev' or 'symlink'. Update a test to check XML schema, and actually add it to the test list since it was missing. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatnode.html.in | 6 +++ docs/schemas/nodedev.rng | 16 +++++++ src/conf/node_device_conf.c | 54 +++++++++++++++++++++- src/conf/node_device_conf.h | 12 +++++ src/node_device/node_device_udev.c | 31 +++++++++++++ ...ge_serial_3600c0ff000d7a2a5d463ff4902000000.xml | 4 ++ tests/nodedevxml2xmltest.c | 1 + 7 files changed, 123 insertions(+), 1 deletion(-) diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index f8d0e1234..ecdd1dbcb 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -37,6 +37,12 @@ <dd>If this element is present, it names the parent device (that is, a controller to which this node belongs). </dd> + <dt><code>devnode</code></dt> + <dd>This node appears for each associated <code>/dev</code> + special file. A mandatory attribute <code>type</code> specify + the kind of file path, which may be either <code>dev</code> for + the main name, or <code>link</code> for additional symlinks. + </dd> <dt><code>capability</code></dt> <dd>This node appears for each capability that libvirt associates with a node. A mandatory diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index b100a6e16..62e29b6cc 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -16,6 +16,22 @@ <element name="path"><text/></element> </optional> <optional> + <element name="devnode"> + <attribute name='type'> + <value>dev</value> + </attribute> + <text/> + </element> + </optional> + <zeroOrMore> + <element name="devnode"> + <attribute name='type'> + <value>link</value> + </attribute> + <text/> + </element> + </zeroOrMore> + <optional> <ref name="parent"/> </optional> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 6163fd5ed..49ecc8897 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -40,6 +40,10 @@ #define VIR_FROM_THIS VIR_FROM_NODEDEV +VIR_ENUM_IMPL(virNodeDevDevnode, VIR_NODE_DEV_DEVNODE_LAST, + "dev", + "link") + VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST, "system", "pci", @@ -252,6 +256,8 @@ void virNodeDeviceDefFree(virNodeDeviceDefPtr def) VIR_FREE(def->driver); VIR_FREE(def->sysfs_path); VIR_FREE(def->parent_sysfs_path); + VIR_FREE(def->devnode); + virStringListFree(def->devlinks); caps = def->caps; while (caps) { @@ -387,6 +393,14 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferAdjustIndent(&buf, 2); virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); virBufferEscapeString(&buf, "<path>%s</path>\n", def->sysfs_path); + if (def->devnode) + 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 (def->parent) virBufferEscapeString(&buf, "<parent>%s</parent>\n", def->parent); if (def->driver) { @@ -1703,7 +1717,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, virNodeDeviceDefPtr def; virNodeDevCapsDefPtr *next_cap; xmlNodePtr *nodes; - int n; + int n, m; size_t i; if (VIR_ALLOC(def) < 0) @@ -1722,6 +1736,44 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, goto error; } + /* Parse devnodes */ + nodes = NULL; + if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0) + goto error; + + if (VIR_ALLOC_N(def->devlinks, n + 1) < 0) + goto error; + + for (i = 0, m = 0; i < n; i++) { + xmlNodePtr node = nodes[i]; + char *tmp = virXMLPropString(node, "type"); + virNodeDevDevnodeType type; + + if (!tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing devnode type")); + goto error; + } + + if ((type = virNodeDevDevnodeTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown devnode type '%s'"), tmp); + VIR_FREE(tmp); + goto error; + } + + switch (type) { + case VIR_NODE_DEV_DEVNODE_DEV: + def->devnode = (char*)xmlNodeGetContent(node); + break; + case VIR_NODE_DEV_DEVNODE_LINK: + def->devlinks[m++] = (char*)xmlNodeGetContent(node); + break; + case VIR_NODE_DEV_DEVNODE_LAST: + break; + } + } + /* Extract device parent, if any */ def->parent = virXPathString("string(./parent[1])", ctxt); def->parent_wwnn = virXPathString("string(./parent[1]/@wwnn)", ctxt); diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 163448333..f46e9841a 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -40,6 +40,16 @@ typedef enum { /* Keep in sync with VIR_ENUM_IMPL in node_device_conf.c */ + VIR_NODE_DEV_DEVNODE_DEV, + VIR_NODE_DEV_DEVNODE_LINK, + + VIR_NODE_DEV_DEVNODE_LAST +} virNodeDevDevnodeType; + +VIR_ENUM_DECL(virNodeDevDevnode) + +typedef enum { + /* Keep in sync with VIR_ENUM_IMPL in node_device_conf.c */ VIR_NODE_DEV_CAP_SYSTEM, /* System capability */ VIR_NODE_DEV_CAP_PCI_DEV, /* PCI device */ VIR_NODE_DEV_CAP_USB_DEV, /* USB device */ @@ -204,6 +214,8 @@ struct _virNodeDeviceDef { char *parent_wwpn; /* optional parent wwpn */ char *parent_fabric_wwn; /* optional parent fabric_wwn */ char *driver; /* optional driver name */ + char *devnode; /* /dev path */ + char **devlinks; /* /dev links */ virNodeDevCapsDefPtr caps; /* optional device capabilities */ }; diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 4b813127c..d7658410a 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -918,6 +918,34 @@ udevProcessSCSIGeneric(struct udev_device *dev, } static int +udevGetDeviceNodes(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + const char *devnode = NULL; + struct udev_list_entry *list_entry = NULL; + int n = 0; + + devnode = udev_device_get_devnode(device); + + if (VIR_STRDUP(def->devnode, devnode) < 0) + return -1; + + udev_list_entry_foreach(list_entry, udev_device_get_devlinks_list_entry(device)) + n++; + + if (VIR_ALLOC_N(def->devlinks, n + 1) < 0) + return -1; + + n = 0; + udev_list_entry_foreach(list_entry, udev_device_get_devlinks_list_entry(device)) { + if (VIR_STRDUP(def->devlinks[n++], udev_list_entry_get_name(list_entry)) < 0) + return -1; + } + + return 0; +} + +static int udevGetDeviceType(struct udev_device *device, virNodeDevCapType *type) { @@ -1125,6 +1153,9 @@ static int udevAddOneDevice(struct udev_device *device) if (udevGetDeviceType(device, &def->caps->data.type) != 0) goto cleanup; + if (udevGetDeviceNodes(device, def) != 0) + goto cleanup; + if (udevGetDeviceDetails(device, def) != 0) goto cleanup; diff --git a/tests/nodedevschemadata/storage_serial_3600c0ff000d7a2a5d463ff4902000000.xml b/tests/nodedevschemadata/storage_serial_3600c0ff000d7a2a5d463ff4902000000.xml index d9d61da44..d225dca8f 100644 --- a/tests/nodedevschemadata/storage_serial_3600c0ff000d7a2a5d463ff4902000000.xml +++ b/tests/nodedevschemadata/storage_serial_3600c0ff000d7a2a5d463ff4902000000.xml @@ -1,5 +1,9 @@ <device> <name>storage_serial_3600c0ff000d7a2a5d463ff4902000000</name> + <devnode type='dev'>/dev/sdb</devnode> + <devnode type='link'>/dev/disk/by-id/usb-SanDisk_Ultra_Fit_4C530001051009112405-0:0</devnode> + <devnode type='link'>/dev/disk/by-path/pci-0000:00:14.0-usb-0:1:1.0-scsi-0:0:0:0</devnode> + <devnode type='link'>/dev/disk/by-uuid/661A1A460111DA18</devnode> <parent>pci_10df_fe00_scsi_host_scsi_device_lun8</parent> <capability type='storage'> <block>/dev/sdj</block> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index eb32dd31d..ec96943cb 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -87,6 +87,7 @@ mymain(void) DO_TEST("pci_8086_27c5_scsi_host_scsi_host"); DO_TEST("pci_8086_27c5_scsi_host"); DO_TEST("storage_serial_SATA_HTS721010G9SA00_MPCZ12Y0GNGWSE"); + DO_TEST("storage_serial_3600c0ff000d7a2a5d463ff4902000000"); DO_TEST("usb_device_1d6b_1_0000_00_1d_0_if0"); DO_TEST("usb_device_1d6b_1_0000_00_1d_0"); DO_TEST("pci_8086_4238_pcie_wireless"); -- 2.11.0.295.gd7dffce1c.dirty

On Wed, Feb 15, 2017 at 01:04:10AM +0400, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Add new <devnode> top-level <device> element, that list the associated /dev files. Distinguish the main /dev name from symlinks with a 'type' attribute of value 'dev' or 'symlink'.
Update a test to check XML schema, and actually add it to the test list since it was missing.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatnode.html.in | 6 +++ docs/schemas/nodedev.rng | 16 +++++++ src/conf/node_device_conf.c | 54 +++++++++++++++++++++- src/conf/node_device_conf.h | 12 +++++ src/node_device/node_device_udev.c | 31 +++++++++++++ ...ge_serial_3600c0ff000d7a2a5d463ff4902000000.xml | 4 ++ tests/nodedevxml2xmltest.c | 1 + 7 files changed, 123 insertions(+), 1 deletion(-)
@@ -1722,6 +1736,44 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, goto error; }
+ /* Parse devnodes */ + nodes = NULL; + if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0) + goto error; + + if (VIR_ALLOC_N(def->devlinks, n + 1) < 0) + goto error;
Ok, n + 1 because we want to be able to treat it as a string list with a trailing NULL element. Technically this is still one element too large, since the first devnode is probably going to be in dev->devnode intead of dev->devlinks, but that's harmless enough that we can ignore it.
+ + for (i = 0, m = 0; i < n; i++) { + xmlNodePtr node = nodes[i]; + char *tmp = virXMLPropString(node, "type"); + virNodeDevDevnodeType type; + + if (!tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("missing devnode type")); + goto error; + } + + if ((type = virNodeDevDevnodeTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown devnode type '%s'"), tmp); + VIR_FREE(tmp); + goto error; + } + + switch (type) { + case VIR_NODE_DEV_DEVNODE_DEV: + def->devnode = (char*)xmlNodeGetContent(node); + break; + case VIR_NODE_DEV_DEVNODE_LINK: + def->devlinks[m++] = (char*)xmlNodeGetContent(node); + break; + case VIR_NODE_DEV_DEVNODE_LAST: + break; + } + } +
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

From: Marc-André Lureau <marcandre.lureau@redhat.com> This should have been added with c4a4603de (or 0bdefd9b04). Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/conf/node_device_conf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 49ecc8897..0d1dafcd6 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1736,6 +1736,8 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, goto error; } + def->sysfs_path = virXPathString("string(./path[1])", ctxt); + /* Parse devnodes */ nodes = NULL; if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0) -- 2.11.0.295.gd7dffce1c.dirty

On Wed, Feb 15, 2017 at 01:04:11AM +0400, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
This should have been added with c4a4603de (or 0bdefd9b04).
Technically the node dev parser only needs to parse enough to deal with NPIV device creation - everything else is output only. But it is good practice to full parse everything regardless IMHO.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- src/conf/node_device_conf.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 49ecc8897..0d1dafcd6 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1736,6 +1736,8 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, goto error; }
+ def->sysfs_path = virXPathString("string(./path[1])", ctxt); + /* Parse devnodes */ nodes = NULL; if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

From: Marc-André Lureau <marcandre.lureau@redhat.com> Add a new 'drm' capability for Direct Rendering Manager (DRM) devices, providing device type information. Teach the udev backend to populate those devices. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatnode.html.in | 10 +++++++ docs/schemas/nodedev.rng | 14 ++++++++++ src/conf/node_device_conf.c | 44 +++++++++++++++++++++++++++++- src/conf/node_device_conf.h | 15 ++++++++++ src/node_device/node_device_driver.c | 1 + src/node_device/node_device_udev.c | 41 ++++++++++++++++++++++++++++ tests/nodedevschemadata/drm_renderD129.xml | 10 +++++++ tests/nodedevxml2xmltest.c | 1 + 8 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/nodedevschemadata/drm_renderD129.xml diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index ecdd1dbcb..a368ffc07 100644 --- a/docs/formatnode.html.in +++ b/docs/formatnode.html.in @@ -314,6 +314,16 @@ and <code>media_label</code>.</dd> </dl> </dd> + <dt><code>drm</code></dt> + <dd>Describes a Direct Rendering Manager (DRM) device. + Sub-elements include: + <dl> + <dt><code>type</code></dt> + <dd>The type of DRM device. Could be + <code>primary</code>, <code>control</code> or + <code>render</code>.</dd> + </dl> + </dd> </dl> </dd> </dl> diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng index 62e29b6cc..0f90a73c8 100644 --- a/docs/schemas/nodedev.rng +++ b/docs/schemas/nodedev.rng @@ -82,6 +82,7 @@ <ref name="capscsitarget"/> <ref name="capscsi"/> <ref name="capstorage"/> + <ref name="capdrm"/> </choice> </element> </define> @@ -540,6 +541,19 @@ </element> </define> + <define name='capdrm'> + <attribute name='type'> + <value>drm</value> + </attribute> + <element name='type'> + <choice> + <value>primary</value> + <value>control</value> + <value>render</value> + </choice> + </element> + </define> + <define name='address'> <element name='address'> <attribute name='domain'><ref name='hexuint'/></attribute> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 0d1dafcd6..04b63cc93 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -56,12 +56,18 @@ VIR_ENUM_IMPL(virNodeDevCap, VIR_NODE_DEV_CAP_LAST, "storage", "fc_host", "vports", - "scsi_generic") + "scsi_generic", + "drm") VIR_ENUM_IMPL(virNodeDevNetCap, VIR_NODE_DEV_CAP_NET_LAST, "80203", "80211") +VIR_ENUM_IMPL(virNodeDevDRM, VIR_NODE_DEV_DRM_LAST, + "primary", + "control", + "render") + static int virNodeDevCapsDefParseString(const char *xpath, xmlXPathContextPtr ctxt, @@ -698,6 +704,9 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDef *def) virBufferEscapeString(&buf, "<char>%s</char>\n", data->sg.path); break; + case VIR_NODE_DEV_CAP_DRM: + virBufferEscapeString(&buf, "<type>%s</type>\n", virNodeDevDRMTypeToString(data->drm.type)); + break; case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_LAST: @@ -799,6 +808,35 @@ virNodeDevCapsDefParseULongLong(const char *xpath, } static int +virNodeDevCapDRMParseXML(xmlXPathContextPtr ctxt, + virNodeDeviceDefPtr def, + xmlNodePtr node, + virNodeDevCapDataPtr data) +{ + xmlNodePtr orignode; + int ret = -1; + char *type = NULL; + + orignode = ctxt->node; + ctxt->node = node; + + type = virXPathString("string(./type[1])", ctxt); + + if ((data->drm.type = virNodeDevDRMTypeFromString(type)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown drm type '%s' for '%s'"), type, def->name); + goto out; + } + + ret = 0; + +out: + VIR_FREE(type); + ctxt->node = orignode; + return ret; +} + +static int virNodeDevCapStorageParseXML(xmlXPathContextPtr ctxt, virNodeDeviceDefPtr def, xmlNodePtr node, @@ -1689,6 +1727,9 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, case VIR_NODE_DEV_CAP_STORAGE: ret = virNodeDevCapStorageParseXML(ctxt, def, node, &caps->data); break; + case VIR_NODE_DEV_CAP_DRM: + ret = virNodeDevCapDRMParseXML(ctxt, def, node, &caps->data); + break; case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_SCSI_GENERIC: @@ -2116,6 +2157,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) case VIR_NODE_DEV_CAP_SCSI_GENERIC: VIR_FREE(data->sg.path); break; + case VIR_NODE_DEV_CAP_DRM: case VIR_NODE_DEV_CAP_FC_HOST: case VIR_NODE_DEV_CAP_VPORTS: case VIR_NODE_DEV_CAP_LAST: diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index f46e9841a..be7e0e003 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -62,6 +62,7 @@ typedef enum { VIR_NODE_DEV_CAP_FC_HOST, /* FC Host Bus Adapter */ VIR_NODE_DEV_CAP_VPORTS, /* HBA which is capable of vports */ VIR_NODE_DEV_CAP_SCSI_GENERIC, /* SCSI generic device */ + VIR_NODE_DEV_CAP_DRM, /* DRM device */ VIR_NODE_DEV_CAP_LAST } virNodeDevCapType; @@ -93,6 +94,17 @@ typedef enum { VIR_NODE_DEV_CAP_FLAG_PCIE = (1 << 2), } virNodeDevPCICapFlags; +typedef enum { + /* Keep in sync with VIR_ENUM_IMPL in node_device_conf.c */ + VIR_NODE_DEV_DRM_PRIMARY, + VIR_NODE_DEV_DRM_CONTROL, + VIR_NODE_DEV_DRM_RENDER, + + VIR_NODE_DEV_DRM_LAST +} virNodeDevDRMType; + +VIR_ENUM_DECL(virNodeDevDRM) + typedef struct _virNodeDevCapData { virNodeDevCapType type; union { @@ -192,6 +204,9 @@ typedef struct _virNodeDevCapData { struct { char *path; } sg; /* SCSI generic device */ + struct { + virNodeDevDRMType type; + } drm; }; } virNodeDevCapData, *virNodeDevCapDataPtr; diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index 5238e231d..d04713f5e 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -72,6 +72,7 @@ static int update_caps(virNodeDeviceObjPtr dev) /* all types that (supposedly) don't require any updates * relative to what's in the cache. */ + case VIR_NODE_DEV_CAP_DRM: case VIR_NODE_DEV_CAP_SYSTEM: case VIR_NODE_DEV_CAP_USB_DEV: case VIR_NODE_DEV_CAP_USB_INTERFACE: diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index d7658410a..6a91e0722 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -410,6 +410,42 @@ static int udevProcessPCI(struct udev_device *device, return ret; } +static int drmGetMinorType(int minor) +{ + int type = minor >> 6; + + if (minor < 0) + return -1; + + switch (type) { + case VIR_NODE_DEV_DRM_PRIMARY: + case VIR_NODE_DEV_DRM_CONTROL: + case VIR_NODE_DEV_DRM_RENDER: + return type; + default: + return -1; + } +} + +static int udevProcessDRMDevice(struct udev_device *device, + virNodeDeviceDefPtr def) +{ + virNodeDevCapDataPtr data = &def->caps->data; + int minor; + + if (udevGenerateDeviceName(device, def, NULL) != 0) + return -1; + + if (udevGetIntProperty(device, "MINOR", &minor, 10) < 0) + return -1; + + if ((minor = drmGetMinorType(minor)) == -1) + return -1; + + data->drm.type = minor; + + return 0; +} static int udevProcessUSBDevice(struct udev_device *device, virNodeDeviceDefPtr def) @@ -971,6 +1007,8 @@ udevGetDeviceType(struct udev_device *device, *type = VIR_NODE_DEV_CAP_STORAGE; else if (STREQ(devtype, "wlan")) *type = VIR_NODE_DEV_CAP_NET; + else if (STREQ(devtype, "drm_minor")) + *type = VIR_NODE_DEV_CAP_DRM; } else { /* PCI devices don't set the DEVTYPE property. */ if (udevHasDeviceProperty(device, "PCI_CLASS")) @@ -1039,6 +1077,9 @@ static int udevGetDeviceDetails(struct udev_device *device, case VIR_NODE_DEV_CAP_SCSI_GENERIC: ret = udevProcessSCSIGeneric(device, def); break; + case VIR_NODE_DEV_CAP_DRM: + ret = udevProcessDRMDevice(device, def); + break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown device type %d"), def->caps->data.type); diff --git a/tests/nodedevschemadata/drm_renderD129.xml b/tests/nodedevschemadata/drm_renderD129.xml new file mode 100644 index 000000000..161481624 --- /dev/null +++ b/tests/nodedevschemadata/drm_renderD129.xml @@ -0,0 +1,10 @@ +<device> + <name>drm_renderD129</name> + <path>/sys/devices/pci0000:00/0000:00:02.0/drm/renderD129</path> + <devnode type='dev'>/dev/dri/renderD129</devnode> + <devnode type='link'>/dev/dri/by-path/pci-0000:00:02.0-render</devnode> + <parent>pci_0000_00_02_0</parent> + <capability type='drm'> + <type>render</type> + </capability> +</device> diff --git a/tests/nodedevxml2xmltest.c b/tests/nodedevxml2xmltest.c index ec96943cb..5e1ae170c 100644 --- a/tests/nodedevxml2xmltest.c +++ b/tests/nodedevxml2xmltest.c @@ -100,6 +100,7 @@ mymain(void) DO_TEST("pci_0000_02_10_7_sriov_zero_vfs_max_count"); DO_TEST("pci_0000_02_10_7_sriov_pf_vfs_all"); DO_TEST("pci_0000_02_10_7_sriov_pf_vfs_all_header_type"); + DO_TEST("drm_renderD129"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.11.0.295.gd7dffce1c.dirty

On Wed, Feb 15, 2017 at 01:04:12AM +0400, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Add a new 'drm' capability for Direct Rendering Manager (DRM) devices, providing device type information.
Teach the udev backend to populate those devices.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatnode.html.in | 10 +++++++ docs/schemas/nodedev.rng | 14 ++++++++++ src/conf/node_device_conf.c | 44 +++++++++++++++++++++++++++++- src/conf/node_device_conf.h | 15 ++++++++++ src/node_device/node_device_driver.c | 1 + src/node_device/node_device_udev.c | 41 ++++++++++++++++++++++++++++ tests/nodedevschemadata/drm_renderD129.xml | 10 +++++++ tests/nodedevxml2xmltest.c | 1 + 8 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 tests/nodedevschemadata/drm_renderD129.xml
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

From: Marc-André Lureau <marcandre.lureau@redhat.com> Add a new attribute 'rendernode' to <gl> spice element. Give it to QEMU if qemu supports it (queued for 2.9). Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 28 +++++++++++++++++++--- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 10 ++++++++ .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +- .../qemuxml2argv-video-virtio-gpu-spice-gl.xml | 2 +- tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-video-virtio-gpu-spice-gl.xml | 2 +- 11 files changed, 54 insertions(+), 7 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0a115f5dc..67f486747 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5619,7 +5619,7 @@ qemu-kvm -net nic,model=? /dev/null <clipboard copypaste='no'/> <mouse mode='client'/> <filetransfer enable='no'/> - <gl enable='yes'/> + <gl enable='yes' rendernode='/dev/dri/by-path/pci-0000:00:02.0-render'/> </graphics></pre> <p> Spice supports variable compression settings for audio, images and @@ -5665,6 +5665,11 @@ qemu-kvm -net nic,model=? /dev/null the <code>gl</code> element, by setting the <code>enable</code> property. (QEMU only, <span class="since">since 1.3.3</span>). </p> + <p> + By default, QEMU will pick the first available GPU DRM render node. + You may specify a DRM render node path to use instead. (QEMU only, + <span class="since">since 3.1</span>). + </p> </dd> <dt><code>rdp</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d715bff29..c5f101325 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3033,6 +3033,11 @@ <attribute name="enable"> <ref name="virYesNo"/> </attribute> + <optional> + <attribute name="rendernode"> + <ref name="absFilePath"/> + </attribute> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1bc72a4e9..b577d0aba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1300,6 +1300,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) break; case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + VIR_FREE(def->data.spice.rendernode); VIR_FREE(def->data.spice.keymap); virDomainGraphicsAuthDefClear(&def->data.spice.auth); break; @@ -12141,6 +12142,7 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, def->data.spice.filetransfer = enableVal; } else if (xmlStrEqual(cur->name, BAD_CAST "gl")) { char *enable = virXMLPropString(cur, "enable"); + char *rendernode = virXMLPropString(cur, "rendernode"); int enableVal; if (!enable) { @@ -12159,6 +12161,8 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, VIR_FREE(enable); def->data.spice.gl = enableVal; + def->data.spice.rendernode = rendernode; + } else if (xmlStrEqual(cur->name, BAD_CAST "mouse")) { char *mode = virXMLPropString(cur, "mode"); int modeVal; @@ -22839,6 +22843,23 @@ virDomainGraphicsListenDefFormatAddr(virBufferPtr buf, virBufferAsprintf(buf, " listen='%s'", glisten->address); } +static int +virDomainSpiceGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def) +{ + if (def->data.spice.gl == VIR_TRISTATE_BOOL_ABSENT) { + return 0; + } + + virBufferAsprintf(buf, "<gl enable='%s'", + virTristateBoolTypeToString(def->data.spice.gl)); + + if (def->data.spice.rendernode) + virBufferEscapeString(buf, " rendernode='%s'", def->data.spice.rendernode); + + virBufferAddLit(buf, "/>\n"); + + return 0; +} static int virDomainGraphicsDefFormat(virBufferPtr buf, @@ -23082,9 +23103,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, if (def->data.spice.filetransfer) virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", virTristateBoolTypeToString(def->data.spice.filetransfer)); - if (def->data.spice.gl) - virBufferAsprintf(buf, "<gl enable='%s'/>\n", - virTristateBoolTypeToString(def->data.spice.gl)); + + if (virDomainSpiceGLDefFormat(buf, def) < 0) { + return -1; + } } if (children) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index dd79206f6..7e1afa475 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1544,6 +1544,7 @@ struct _virDomainGraphicsDef { virTristateBool copypaste; virTristateBool filetransfer; virTristateBool gl; + char *rendernode; } spice; } data; /* nListens, listens, and *port are only useful if type is vnc, diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 399e31447..e851eec7a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -357,6 +357,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "query-cpu-model-expansion", /* 245 */ "virtio-net.host_mtu", + "spice-rendernode", ); @@ -2950,6 +2951,7 @@ static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = { { "spice", "unix", QEMU_CAPS_SPICE_UNIX }, { "drive", "throttling.bps-total-max-length", QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH }, { "drive", "throttling.group", QEMU_CAPS_DRIVE_IOTUNE_GROUP }, + { "spice", "rendernode", QEMU_CAPS_SPICE_RENDERNODE }, }; static int diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 95bb67d44..0f998c473 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -393,6 +393,7 @@ typedef enum { /* 245 */ QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */ QEMU_CAPS_VIRTIO_NET_HOST_MTU, /* virtio-net-*.host_mtu */ + QEMU_CAPS_SPICE_RENDERNODE, /* -spice rendernode */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c00a47a91..f4bcfd467 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7920,6 +7920,16 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, * TristateSwitch helper */ virBufferAsprintf(&opt, "gl=%s,", virTristateSwitchTypeToString(graphics->data.spice.gl)); + + if (graphics->data.spice.rendernode) { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_RENDERNODE)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("This QEMU doesn't support spice OpenGL rendernode")); + goto error; + } + + virBufferAsprintf(&opt, "rendernode=%s,", graphics->data.spice.rendernode); + } } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args index f1ebb62e4..84439cddc 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.args @@ -19,6 +19,6 @@ QEMU_AUDIO_DRV=spice \ -drive file=/var/lib/libvirt/images/QEMUGuest1,format=qcow2,if=none,\ id=drive-ide0-0-0,cache=none \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --spice port=0,gl=on \ +-spice port=0,gl=on,rendernode=/dev/dri/foo \ -device virtio-gpu-pci,id=video0,virgl=on,bus=pci.0,addr=0x2 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml index b9c7c8af0..fd2580635 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-video-virtio-gpu-spice-gl.xml @@ -26,7 +26,7 @@ <input type='mouse' bus='ps2'/> <input type='keyboard' bus='ps2'/> <graphics type='spice' autoport='no'> - <gl enable='yes'/> + <gl enable='yes' rendernode='/dev/dri/foo'/> </graphics> <video> <model type='virtio' heads='1'> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index faa99c64c..f55b04b05 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1725,6 +1725,7 @@ mymain(void) QEMU_CAPS_VIRTIO_GPU_VIRGL, QEMU_CAPS_SPICE, QEMU_CAPS_SPICE_GL, + QEMU_CAPS_SPICE_RENDERNODE, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); DO_TEST("video-virtio-gpu-secondary", QEMU_CAPS_DEVICE_VIRTIO_GPU, diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml index 9fb533ad9..2ce838fab 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-video-virtio-gpu-spice-gl.xml @@ -31,7 +31,7 @@ <input type='keyboard' bus='ps2'/> <graphics type='spice'> <listen type='none'/> - <gl enable='yes'/> + <gl enable='yes' rendernode='/dev/dri/foo'/> </graphics> <video> <model type='virtio' heads='1' primary='yes'> -- 2.11.0.295.gd7dffce1c.dirty

On Wed, Feb 15, 2017 at 01:04:13AM +0400, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Add a new attribute 'rendernode' to <gl> spice element.
Give it to QEMU if qemu supports it (queued for 2.9).
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 28 +++++++++++++++++++--- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 10 ++++++++ .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +- .../qemuxml2argv-video-virtio-gpu-spice-gl.xml | 2 +- tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-video-virtio-gpu-spice-gl.xml | 2 +- 11 files changed, 54 insertions(+), 7 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|

On 02/14/2017 10:04 PM, marcandre.lureau@redhat.com wrote:
From: Marc-André Lureau <marcandre.lureau@redhat.com>
Add a new attribute 'rendernode' to <gl> spice element.
Give it to QEMU if qemu supports it (queued for 2.9).
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- docs/formatdomain.html.in | 7 +++++- docs/schemas/domaincommon.rng | 5 ++++ src/conf/domain_conf.c | 28 +++++++++++++++++++--- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 10 ++++++++ .../qemuxml2argv-video-virtio-gpu-spice-gl.args | 2 +- .../qemuxml2argv-video-virtio-gpu-spice-gl.xml | 2 +- tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-video-virtio-gpu-spice-gl.xml | 2 +- 11 files changed, 54 insertions(+), 7 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0a115f5dc..67f486747 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5619,7 +5619,7 @@ qemu-kvm -net nic,model=? /dev/null <clipboard copypaste='no'/> <mouse mode='client'/> <filetransfer enable='no'/> - <gl enable='yes'/> + <gl enable='yes' rendernode='/dev/dri/by-path/pci-0000:00:02.0-render'/> </graphics></pre> <p> Spice supports variable compression settings for audio, images and @@ -5665,6 +5665,11 @@ qemu-kvm -net nic,model=? /dev/null the <code>gl</code> element, by setting the <code>enable</code> property. (QEMU only, <span class="since">since 1.3.3</span>). </p> + <p> + By default, QEMU will pick the first available GPU DRM render node. + You may specify a DRM render node path to use instead. (QEMU only, + <span class="since">since 3.1</span>). + </p> </dd> <dt><code>rdp</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d715bff29..c5f101325 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3033,6 +3033,11 @@ <attribute name="enable"> <ref name="virYesNo"/> </attribute> + <optional> + <attribute name="rendernode"> + <ref name="absFilePath"/> + </attribute> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1bc72a4e9..b577d0aba 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1300,6 +1300,7 @@ void virDomainGraphicsDefFree(virDomainGraphicsDefPtr def) break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + VIR_FREE(def->data.spice.rendernode); VIR_FREE(def->data.spice.keymap); virDomainGraphicsAuthDefClear(&def->data.spice.auth); break; @@ -12141,6 +12142,7 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, def->data.spice.filetransfer = enableVal; } else if (xmlStrEqual(cur->name, BAD_CAST "gl")) { char *enable = virXMLPropString(cur, "enable"); + char *rendernode = virXMLPropString(cur, "rendernode"); int enableVal;
This might be leaked if control reaches 'goto' lines in between this and the following hunk.
if (!enable) { @@ -12159,6 +12161,8 @@ virDomainGraphicsDefParseXMLSpice(virDomainGraphicsDefPtr def, VIR_FREE(enable);
def->data.spice.gl = enableVal; + def->data.spice.rendernode = rendernode; + } else if (xmlStrEqual(cur->name, BAD_CAST "mouse")) { char *mode = virXMLPropString(cur, "mode"); int modeVal;
@@ -22839,6 +22843,23 @@ virDomainGraphicsListenDefFormatAddr(virBufferPtr buf, virBufferAsprintf(buf, " listen='%s'", glisten->address); }
+static int +virDomainSpiceGLDefFormat(virBufferPtr buf, virDomainGraphicsDefPtr def) +{ + if (def->data.spice.gl == VIR_TRISTATE_BOOL_ABSENT) { + return 0; + }
Firstly, no need for this function to return an int. We usually have functions like this return nothing (void). Secondly, there's no need to put curly braces around one line body.
+ + virBufferAsprintf(buf, "<gl enable='%s'", + virTristateBoolTypeToString(def->data.spice.gl)); + + if (def->data.spice.rendernode) + virBufferEscapeString(buf, " rendernode='%s'", def->data.spice.rendernode);
The virBufferEscapeString() is no-op if the last arg is NULL.
+ + virBufferAddLit(buf, "/>\n"); + + return 0; +}
static int virDomainGraphicsDefFormat(virBufferPtr buf, @@ -23082,9 +23103,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, if (def->data.spice.filetransfer) virBufferAsprintf(buf, "<filetransfer enable='%s'/>\n", virTristateBoolTypeToString(def->data.spice.filetransfer)); - if (def->data.spice.gl) - virBufferAsprintf(buf, "<gl enable='%s'/>\n", - virTristateBoolTypeToString(def->data.spice.gl)); + + if (virDomainSpiceGLDefFormat(buf, def) < 0) { + return -1; + }
Again. This will never happen and also there's no need for {}.
}
if (children) {
I am fixing all of these small nits that I've raised and pushing all of the patches. I have couple of follow up patches ready so expect them to be send shortly. Michal
participants (3)
-
Daniel P. Berrange
-
marcandre.lureau@redhat.com
-
Michal Privoznik