The function does not reject '&', '<', '>' contrary to what it actually states. Move and adjust the comment. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpcivpd.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 39557c7347..248a9b2790 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -167,19 +167,15 @@ virPCIVPDResourceGetFieldValueFormat(const char *keyword) * value or text field value. The expectations are based on the keywords specified * in relevant sections of PCI(e) specifications * ("I.3. VPD Definitions" in PCI specs, "6.28.1 VPD Format" PCIe 4.0). + * + * The PCI(e) specs mention alphanumeric characters when talking about text fields + * and the string resource but also include spaces and dashes in the provided example. + * Dots, commas, equal signs have also been observed in values used by major device vendors. */ bool virPCIVPDResourceIsValidTextValue(const char *value) { size_t i = 0; - /* - * The PCI(e) specs mention alphanumeric characters when talking about text fields - * and the string resource but also include spaces and dashes in the provided example. - * Dots, commas, equal signs have also been observed in values used by major device vendors. - * The specs do not specify a full set of allowed code points and for Libvirt it is important - * to keep values in the ranges allowed within XML elements (mainly excluding less-than, - * greater-than and ampersand). - */ if (value == NULL) return false; -- 2.43.0

The custom field data is taken from PCI device data which can contain any printable characters, and thus must be escaped when putting into XML. Originally, based on the comment and XML schema which was fixed in previous commits the idea seemed to be that the parser would validate that only characters which don't break the XML would be present but that didn't seem to materialize. Switch to proper escaping of the XML. Fixes: 3954378d06a Resolves: https://issues.redhat.com/browse/RHEL-22314 Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/node_device_conf.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4826be6f42..87c046e571 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -242,23 +242,32 @@ virNodeDeviceCapMdevTypesFormat(virBuffer *buf, } static void -virNodeDeviceCapVPDFormatCustomVendorField(virPCIVPDResourceCustom *field, virBuffer *buf) +virNodeDeviceCapVPDFormatCustomField(virBuffer *buf, + const char *fieldtype, + virPCIVPDResourceCustom *field) { + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) content = VIR_BUFFER_INITIALIZER; + if (field == NULL || field->value == NULL) return; - virBufferAsprintf(buf, "<vendor_field index='%c'>%s</vendor_field>\n", field->idx, - field->value); + virBufferAsprintf(&attrBuf, " index='%c'", field->idx); + virBufferEscapeString(&content, "%s", field->value); + + virXMLFormatElementInternal(buf, fieldtype, &attrBuf, &content, false, false); } static void -virNodeDeviceCapVPDFormatCustomSystemField(virPCIVPDResourceCustom *field, virBuffer *buf) +virNodeDeviceCapVPDFormatCustomVendorField(virPCIVPDResourceCustom *field, virBuffer *buf) { - if (field == NULL || field->value == NULL) - return; + virNodeDeviceCapVPDFormatCustomField(buf, "vendor_field", field); +} - virBufferAsprintf(buf, "<system_field index='%c'>%s</system_field>\n", field->idx, - field->value); +static void +virNodeDeviceCapVPDFormatCustomSystemField(virPCIVPDResourceCustom *field, virBuffer *buf) +{ + virNodeDeviceCapVPDFormatCustomField(buf, "system_field", field); } static inline void -- 2.43.0

The check in 'virPCIVPDResourceIsValidTextValue' allows any printable characters, thus the XML schema should do the same. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/schemas/nodedev.rng | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/schemas/nodedev.rng b/src/conf/schemas/nodedev.rng index fba4021754..ff07313968 100644 --- a/src/conf/schemas/nodedev.rng +++ b/src/conf/schemas/nodedev.rng @@ -869,7 +869,7 @@ <define name="vpdFieldValueFormat"> <data type="string"> - <param name="pattern">[0-9a-zA-F -_,.:;=]{0,255}</param> + <param name="pattern">.{0,255}</param> </data> </define> -- 2.43.0

'virXPathNodeSet' returns -1 only when 'ctxt' or 'xpath' are NULL or when the 'xpath' string is invalid. Both are programmign errors. It doesn't make sense for the code to overwrite the error message for anything supposedly more relevant. The majority of calls to 'virXPathNodeSet' already didn't do this, so this patch fixes the rest to prevent it from spreading again. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 78 +++++++------------------------ src/conf/network_conf.c | 80 ++++++++------------------------ src/conf/node_device_conf.c | 17 ++----- src/conf/numa_conf.c | 15 +++--- src/cpu/cpu_ppc64.c | 5 +- src/qemu/qemu_capabilities.c | 30 +++--------- src/qemu/qemu_domain.c | 23 +++------ src/qemu/qemu_migration_cookie.c | 5 +- src/qemu/qemu_nbdkit.c | 5 +- src/vz/vz_sdk.c | 5 +- 10 files changed, 72 insertions(+), 191 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index fb5a5cc351..ea6fb1e48d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17780,11 +17780,8 @@ virDomainResctrlMonDefParse(virDomainDef *def, ctxt->node = node; - if ((n = virXPathNodeSet("./monitor", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot extract monitor nodes")); + if ((n = virXPathNodeSet("./monitor", ctxt, &nodes)) < 0) goto cleanup; - } for (i = 0; i < n; i++) { domresmon = g_new0(virDomainResctrlMonDef, 1); @@ -17912,11 +17909,8 @@ virDomainCachetuneDefParse(virDomainDef *def, if (virBitmapIsAllClear(vcpus)) return 0; - if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot extract cache nodes under cachetune")); + if ((n = virXPathNodeSet("./cache", ctxt, &nodes)) < 0) return -1; - } if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1; @@ -18182,11 +18176,8 @@ virDomainDefParseMemory(virDomainDef *def, if (virXPathNode("./memoryBacking/hugepages", ctxt)) { /* hugepages will be used */ - if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract hugepages nodes")); + if ((n = virXPathNodeSet("./memoryBacking/hugepages/page", ctxt, &nodes)) < 0) return -1; - } if (n) { def->mem.hugepages = g_new0(virDomainHugePage, n); @@ -18270,11 +18261,8 @@ virDomainMemorytuneDefParse(virDomainDef *def, if (virBitmapIsAllClear(vcpus)) return 0; - if ((n = virXPathNodeSet("./node", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot extract memory nodes under memorytune")); + if ((n = virXPathNodeSet("./node", ctxt, &nodes)) < 0) return -1; - } if (virDomainResctrlVcpuMatch(def, vcpus, &resctrl) < 0) return -1; @@ -18341,11 +18329,9 @@ virDomainDefTunablesParse(virDomainDef *def, &def->blkio.weight) < 0) def->blkio.weight = 0; - if ((n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot extract blkiotune nodes")); + if ((n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes)) < 0) return -1; - } + if (n) def->blkio.devices = g_new0(virBlkioDevice, n); @@ -18456,11 +18442,8 @@ virDomainDefTunablesParse(virDomainDef *def, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract emulatorpin nodes")); + if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, &nodes)) < 0) return -1; - } if (n) { if (n > 1) { @@ -18475,11 +18458,8 @@ virDomainDefTunablesParse(virDomainDef *def, VIR_FREE(nodes); - if ((n = virXPathNodeSet("./cputune/iothreadpin", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract iothreadpin nodes")); + if ((n = virXPathNodeSet("./cputune/iothreadpin", ctxt, &nodes)) < 0) return -1; - } for (i = 0; i < n; i++) { if (virDomainIOThreadPinDefParseXML(nodes[i], def) < 0) @@ -18487,11 +18467,8 @@ virDomainDefTunablesParse(virDomainDef *def, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract vcpusched nodes")); + if ((n = virXPathNodeSet("./cputune/vcpusched", ctxt, &nodes)) < 0) return -1; - } for (i = 0; i < n; i++) { if (virDomainVcpuThreadSchedParse(nodes[i], def) < 0) @@ -18499,11 +18476,8 @@ virDomainDefTunablesParse(virDomainDef *def, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./cputune/iothreadsched", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract iothreadsched nodes")); + if ((n = virXPathNodeSet("./cputune/iothreadsched", ctxt, &nodes)) < 0) return -1; - } for (i = 0; i < n; i++) { if (virDomainIOThreadSchedParse(nodes[i], def) < 0) @@ -18511,11 +18485,8 @@ virDomainDefTunablesParse(virDomainDef *def, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./cputune/emulatorsched", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract emulatorsched nodes")); + if ((n = virXPathNodeSet("./cputune/emulatorsched", ctxt, &nodes)) < 0) return -1; - } if (n) { if (n > 1) { @@ -18529,11 +18500,8 @@ virDomainDefTunablesParse(virDomainDef *def, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract cachetune nodes")); + if ((n = virXPathNodeSet("./cputune/cachetune", ctxt, &nodes)) < 0) return -1; - } for (i = 0; i < n; i++) { if (virDomainCachetuneDefParse(def, ctxt, nodes[i], flags) < 0) @@ -18541,11 +18509,8 @@ virDomainDefTunablesParse(virDomainDef *def, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./cputune/memorytune", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract memorytune nodes")); + if ((n = virXPathNodeSet("./cputune/memorytune", ctxt, &nodes)) < 0) return -1; - } for (i = 0; i < n; i++) { if (virDomainMemorytuneDefParse(def, ctxt, nodes[i], flags) < 0) @@ -18849,11 +18814,8 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt, !virDomainIOThreadIDArrayHasPin(def)) def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO; - if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot extract resource nodes")); + if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) return NULL; - } if (n > 1) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -18901,11 +18863,9 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt, return NULL; /* analysis of the resource leases */ - if ((n = virXPathNodeSet("./devices/lease", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot extract device leases")); + if ((n = virXPathNodeSet("./devices/lease", ctxt, &nodes)) < 0) return NULL; - } + if (n) def->leases = g_new0(virDomainLeaseDef *, n); for (i = 0; i < n; i++) { @@ -19024,11 +18984,9 @@ virDomainDefParseXML(xmlXPathContextPtr ctxt, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./devices/console", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("cannot extract console devices")); + if ((n = virXPathNodeSet("./devices/console", ctxt, &nodes)) < 0) return NULL; - } + if (n) def->consoles = g_new0(virDomainChrDef *, n); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ed2e72eddf..a2220c05a6 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -892,13 +892,9 @@ virNetworkDNSDefParseXML(const char *networkName, &def->forwardPlainNames) < 0) return -1; - nfwds = virXPathNodeSet("./forwarder", ctxt, &fwdNodes); - if (nfwds < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <forwarder> element found in <dns> of network %1$s"), - networkName); + if ((nfwds = virXPathNodeSet("./forwarder", ctxt, &fwdNodes)) < 0) return -1; - } + if (nfwds > 0) { def->forwarders = g_new0(virNetworkDNSForwarder, nfwds); @@ -922,13 +918,9 @@ virNetworkDNSDefParseXML(const char *networkName, } } - nhosts = virXPathNodeSet("./host", ctxt, &hostNodes); - if (nhosts < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <host> element found in <dns> of network %1$s"), - networkName); + if ((nhosts = virXPathNodeSet("./host", ctxt, &hostNodes)) < 0) return -1; - } + if (nhosts > 0) { def->hosts = g_new0(virNetworkDNSHostDef, nhosts); @@ -941,13 +933,9 @@ virNetworkDNSDefParseXML(const char *networkName, } } - nsrvs = virXPathNodeSet("./srv", ctxt, &srvNodes); - if (nsrvs < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <srv> element found in <dns> of network %1$s"), - networkName); + if ((nsrvs = virXPathNodeSet("./srv", ctxt, &srvNodes)) < 0) return -1; - } + if (nsrvs > 0) { def->srvs = g_new0(virNetworkDNSSrvDef, nsrvs); @@ -960,13 +948,9 @@ virNetworkDNSDefParseXML(const char *networkName, } } - ntxts = virXPathNodeSet("./txt", ctxt, &txtNodes); - if (ntxts < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <txt> element found in <dns> of network %1$s"), - networkName); + if ((ntxts = virXPathNodeSet("./txt", ctxt, &txtNodes)) < 0) return -1; - } + if (ntxts > 0) { def->txts = g_new0(virNetworkDNSTxtDef, ntxts); @@ -1222,13 +1206,10 @@ virNetworkForwardNatDefParseXML(const char *networkName, return -1; /* addresses for SNAT */ - nNatAddrs = virXPathNodeSet("./address", ctxt, &natAddrNodes); - if (nNatAddrs < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <address> element found in <forward> of network %1$s"), - networkName); + if ((nNatAddrs = virXPathNodeSet("./address", ctxt, &natAddrNodes)) < 0) return -1; - } else if (nNatAddrs > 1) { + + if (nNatAddrs > 1) { virReportError(VIR_ERR_XML_ERROR, _("Only one <address> element is allowed in <nat> in <forward> in network %1$s"), networkName); @@ -1284,13 +1265,10 @@ virNetworkForwardNatDefParseXML(const char *networkName, } /* ports for SNAT and MASQUERADE */ - nNatPorts = virXPathNodeSet("./port", ctxt, &natPortNodes); - if (nNatPorts < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <port> element found in <forward> of network %1$s"), - networkName); + if ((nNatPorts = virXPathNodeSet("./port", ctxt, &natPortNodes)) < 0) return -1; - } else if (nNatPorts > 1) { + + if (nNatPorts > 1) { virReportError(VIR_ERR_XML_ERROR, _("Only one <port> element is allowed in <nat> in <forward> in network %1$s"), networkName); @@ -1358,37 +1336,19 @@ virNetworkForwardDefParseXML(const char *networkName, } /* bridge and hostdev modes can use a pool of physical interfaces */ - nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes); - if (nForwardIfs < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <interface> element found in <forward> of network %1$s"), - networkName); + if ((nForwardIfs = virXPathNodeSet("./interface", ctxt, &forwardIfNodes)) < 0) return -1; - } - nForwardAddrs = virXPathNodeSet("./address", ctxt, &forwardAddrNodes); - if (nForwardAddrs < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <address> element found in <forward> of network %1$s"), - networkName); + if ((nForwardAddrs = virXPathNodeSet("./address", ctxt, &forwardAddrNodes)) < 0) return -1; - } - nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes); - if (nForwardPfs < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <pf> element found in <forward> of network %1$s"), - networkName); + if ((nForwardPfs = virXPathNodeSet("./pf", ctxt, &forwardPfNodes)) < 0) return -1; - } - nForwardNats = virXPathNodeSet("./nat", ctxt, &forwardNatNodes); - if (nForwardNats < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("invalid <nat> element found in <forward> of network %1$s"), - networkName); + if ((nForwardNats = virXPathNodeSet("./nat", ctxt, &forwardNatNodes)) < 0) return -1; - } else if (nForwardNats > 1) { + + if (nForwardNats > 1) { virReportError(VIR_ERR_XML_ERROR, _("Only one <nat> element is allowed in <forward> of network %1$s"), networkName); diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 95de77abe9..dd174d3020 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -960,11 +960,9 @@ virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource g_autofree xmlNodePtr *nodes = NULL; size_t i = 0; - if ((nfields = virXPathNodeSet("./vendor_field[@index]", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("failed to evaluate <vendor_field> elements")); + if ((nfields = virXPathNodeSet("./vendor_field[@index]", ctxt, &nodes)) < 0) return -1; - } + for (i = 0; i < nfields; i++) { g_autofree char *value = NULL; g_autofree char *index = NULL; @@ -989,11 +987,9 @@ virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource VIR_FREE(nodes); if (!readOnly) { - if ((nfields = virXPathNodeSet("./system_field[@index]", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("failed to evaluate <system_field> elements")); + if ((nfields = virXPathNodeSet("./system_field[@index]", ctxt, &nodes)) < 0) return -1; - } + for (i = 0; i < nfields; i++) { g_autofree char *value = NULL; g_autofree char *index = NULL; @@ -1074,11 +1070,8 @@ virNodeDeviceCapVPDParseXML(xmlXPathContextPtr ctxt, virPCIVPDResource **res) return -1; } - if ((nfields = virXPathNodeSet("./fields[@access]", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("no VPD <fields> elements with an access type attribute found")); + if ((nfields = virXPathNodeSet("./fields[@access]", ctxt, &nodes)) < 0) return -1; - } for (i = 0; i < nfields; i++) { g_autofree char *access = NULL; diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index bcd7838e00..d8120de6d2 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -135,11 +135,8 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, size_t i = 0; g_autofree xmlNodePtr *nodes = NULL; - if ((n = virXPathNodeSet("./numatune/memnode", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot extract memnode nodes")); + if ((n = virXPathNodeSet("./numatune/memnode", ctxt, &nodes)) < 0) return -1; - } if (!n) return 0; @@ -700,7 +697,10 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, if (!virXPathNode("./distances[1]", ctxt)) return 0; - if ((sibling = virXPathNodeSet("./distances[1]/sibling", ctxt, &nodes)) <= 0) { + if ((sibling = virXPathNodeSet("./distances[1]/sibling", ctxt, &nodes)) < 0) + goto cleanup; + + if (sibling == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("NUMA distances defined without siblings")); goto cleanup; @@ -852,7 +852,10 @@ virDomainNumaDefParseXML(virDomainNuma *def, if (!virXPathNode("./cpu/numa[1]", ctxt)) return 0; - if ((n = virXPathNodeSet("./cpu/numa[1]/cell", ctxt, &cell)) <= 0) { + if ((n = virXPathNodeSet("./cpu/numa[1]/cell", ctxt, &cell)) < 0) + return -1; + + if (n == 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("NUMA topology defined without NUMA cells")); return -1; diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index e13cdbdf6b..448a0a7d85 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -334,7 +334,10 @@ ppc64ModelParse(xmlXPathContextPtr ctxt, } } - if ((n = virXPathNodeSet("./pvr", ctxt, &nodes)) <= 0) { + if ((n = virXPathNodeSet("./pvr", ctxt, &nodes)) < 0) + return -1; + + if (n == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Missing PVR information for CPU model %1$s"), model->name); diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e24b679060..f4b4f05479 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3986,11 +3986,8 @@ virQEMUCapsLoadCPUModels(virArch arch, int n; xmlNodePtr node; - if ((n = virXPathNodeSet(xpath, ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse qemu capabilities cpus")); + if ((n = virXPathNodeSet(xpath, ctxt, &nodes)) < 0) return -1; - } if (n == 0) return 0; @@ -4028,11 +4025,8 @@ virQEMUCapsLoadCPUModels(virArch arch, nblockers = virXPathNodeSet("./blocker", ctxt, &blockerNodes); ctxt->node = node; - if (nblockers < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse CPU blockers in QEMU capabilities")); + if (nblockers < 0) return -1; - } if (nblockers > 0) { size_t j; @@ -4071,11 +4065,8 @@ virQEMUCapsLoadMachines(virQEMUCapsAccel *caps, size_t i; int n; - if ((n = virXPathNodeSet(xpath, ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse qemu capabilities machines")); + if ((n = virXPathNodeSet(xpath, ctxt, &nodes)) < 0) return -1; - } if (n == 0) return 0; @@ -4288,11 +4279,8 @@ virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps, ctxt->node = sgxSections; nSgxSections = virXPathNodeSet("./section", ctxt, &sectionNodes); - if (nSgxSections < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse SGX sections in QEMU capabilities cache")); + if (nSgxSections < 0) return -1; - } sgx->nSgxSections = nSgxSections; sgx->sgxSections = g_new0(virSGXSection, nSgxSections); @@ -4375,11 +4363,8 @@ virQEMUCapsParseFlags(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt) size_t i; int n; - if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse qemu capabilities flags")); + if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) return -1; - } VIR_DEBUG("Got flags %d", n); for (i = 0; i < n; i++) { @@ -4413,11 +4398,8 @@ virQEMUCapsParseGIC(virQEMUCaps *qemuCaps, xmlXPathContextPtr ctxt) size_t i; int n; - if ((n = virXPathNodeSet("./gic", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse qemu capabilities gic")); + if ((n = virXPathNodeSet("./gic", ctxt, &nodes)) < 0) return -1; - } if (n > 0) { unsigned int uintValue; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dad89954d9..dac9e97ce9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3155,11 +3155,8 @@ qemuDomainObjPrivateXMLParseSlirpFeatures(xmlNodePtr featuresNode, ctxt->node = featuresNode; - if ((n = virXPathNodeSet("./feature", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to parse slirp-helper features")); + if ((n = virXPathNodeSet("./feature", ctxt, &nodes)) < 0) return -1; - } for (i = 0; i < n; i++) { g_autofree char *str = virXMLPropString(nodes[i], "name"); @@ -3273,11 +3270,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./qemuCaps/flag", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to parse qemu capabilities flags")); + if ((n = virXPathNodeSet("./qemuCaps/flag", ctxt, &nodes)) < 0) return -1; - } + if (n > 0) { qemuCaps = virQEMUCapsNew(); @@ -3305,11 +3300,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1; - if ((n = virXPathNodeSet("./devices/device", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse qemu device list")); + if ((n = virXPathNodeSet("./devices/device", ctxt, &nodes)) < 0) return -1; - } + if (n > 0) { /* NULL-terminated list */ priv->qemuDevices = g_new0(char *, n + 1); @@ -3325,11 +3318,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, } VIR_FREE(nodes); - if ((n = virXPathNodeSet("./slirp/helper", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse slirp helper list")); + if ((n = virXPathNodeSet("./slirp/helper", ctxt, &nodes)) < 0) return -1; - } + for (i = 0; i < n; i++) { g_autofree char *alias = virXMLPropString(nodes[i], "alias"); g_autofree char *pid = virXMLPropString(nodes[i], "pid"); diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 5505fdaf22..4361949cca 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -947,11 +947,8 @@ qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt) g_autofree xmlNodePtr *interfaces = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) - if ((n = virXPathNodeSet("./network/interface", ctxt, &interfaces)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing interface information")); + if ((n = virXPathNodeSet("./network/interface", ctxt, &interfaces)) < 0) return NULL; - } optr->nnets = n; optr->net = g_new0(qemuMigrationCookieNetData, optr->nnets); diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c index 1c72b6fe6a..acf961b59e 100644 --- a/src/qemu/qemu_nbdkit.c +++ b/src/qemu/qemu_nbdkit.c @@ -400,11 +400,8 @@ qemuNbdkitCapsParseFlags(qemuNbdkitCaps *nbdkitCaps, size_t i; int n; - if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to parse qemu capabilities flags")); + if ((n = virXPathNodeSet("./flag", ctxt, &nodes)) < 0) return -1; - } VIR_DEBUG("Got flags %d", n); for (i = 0; i < n; i++) { diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 6a15d60577..ce4586a3f5 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4612,11 +4612,8 @@ prlsdkParseSnapshotTree(const char *treexml) "ParallelsSavedStates", &ctxt, NULL, false))) goto cleanup; - if ((n = virXPathNodeSet("//SavedStateItem", ctxt, &nodes)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot extract snapshot nodes")); + if ((n = virXPathNodeSet("//SavedStateItem", ctxt, &nodes)) < 0) goto cleanup; - } for (i = 0; i < n; i++) { if (nodes[i]->parent == xmlDocGetRootElement(xml)) -- 2.43.0

The function is no longer used outside of virpcivpd.c Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt_private.syms | 1 - src/util/virpcivpd.c | 14 +------------- src/util/virpcivpdpriv.h | 3 --- 3 files changed, 1 insertion(+), 17 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fc26109029..dbc4e26d79 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3702,7 +3702,6 @@ virVHBAPathExists; virPCIVPDParse; virPCIVPDParseVPDLargeResourceFields; virPCIVPDParseVPDLargeResourceString; -virPCIVPDReadVPDBytes; virPCIVPDResourceCustomCompareIndex; virPCIVPDResourceCustomFree; virPCIVPDResourceCustomUpsertValue; diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 81c7c317b3..373321a836 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -401,7 +401,7 @@ virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, * descriptor, it is reported and -1 is returned to the caller. If EOF is occurred, 0 is returned * to the caller. */ -size_t +static size_t virPCIVPDReadVPDBytes(int vpdFileFd, uint8_t *buf, size_t count, off_t offset, uint8_t *csum) { ssize_t numRead = pread(vpdFileFd, buf, count, offset); @@ -732,18 +732,6 @@ virPCIVPDParse(int vpdFileFd) #else /* ! __linux__ */ -size_t -virPCIVPDReadVPDBytes(int vpdFileFd G_GNUC_UNUSED, - uint8_t *buf G_GNUC_UNUSED, - size_t count G_GNUC_UNUSED, - off_t offset G_GNUC_UNUSED, - uint8_t *csum G_GNUC_UNUSED) -{ - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("PCI VPD reporting not available on this platform")); - return 0; -} - bool virPCIVPDParseVPDLargeResourceString(int vpdFileFd G_GNUC_UNUSED, uint16_t resPos G_GNUC_UNUSED, diff --git a/src/util/virpcivpdpriv.h b/src/util/virpcivpdpriv.h index 0f565f81ae..17e6e14ab7 100644 --- a/src/util/virpcivpdpriv.h +++ b/src/util/virpcivpdpriv.h @@ -69,9 +69,6 @@ virPCIVPDResourceCustomCompareIndex(virPCIVPDResourceCustom *a, virPCIVPDResourc bool virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const value); -size_t -virPCIVPDReadVPDBytes(int vpdFileFd, uint8_t *buf, size_t count, off_t offset, uint8_t *csum); - bool virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, bool readOnly, uint8_t *csum, virPCIVPDResource *res); -- 2.43.0

The test case excercises 'virPCIVPDParseVPDLargeResourceString' which is also tested by other cases which parse the whole VPD block. Remove the specific test case as it's not adding any additional value. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/virpcivpdtest.c | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index aadd1b222b..fddb42f52c 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -429,42 +429,6 @@ testPCIVPDGetFieldValueFormat(const void *data G_GNUC_UNUSED) 'Y', 'E', 0x00, \ 'R', 'W', 0x02, 0x00, 0x00 -static int -testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED) -{ - VIR_AUTOCLOSE fd = -1; - uint8_t csum = 0; - size_t dataLen = 0; - bool result = false; - - g_autoptr(virPCIVPDResource) res = g_new0(virPCIVPDResource, 1); - const char *expectedValue = "testname"; - - const uint8_t stringResExample[] = { - VPD_STRING_RESOURCE_EXAMPLE_DATA - }; - - dataLen = G_N_ELEMENTS(stringResExample); - if ((fd = virCreateAnonymousFile(stringResExample, dataLen)) < 0) - return -1; - - result = virPCIVPDParseVPDLargeResourceString(fd, 0, dataLen, &csum, res); - - if (!result) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - "Could not parse the example resource."); - return -1; - } - - if (STRNEQ(expectedValue, res->name)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Unexpected string resource value: %s, expected: %s", - res->name, expectedValue); - return -1; - } - return 0; -} - static int testVirPCIVPDValidateExampleReadOnlyFields(virPCIVPDResource *res) { @@ -964,8 +928,6 @@ mymain(void) if (virTestRun("Determining a field value format by a key ", testPCIVPDGetFieldValueFormat, NULL) < 0) ret = -1; - if (virTestRun("Parsing VPD string resources ", testVirPCIVPDParseVPDStringResource, NULL) < 0) - ret = -1; if (virTestRun("Parsing a VPD resource with a zero-length RW ", testVirPCIVPDParseZeroLengthRW, NULL) < 0) ret = -1; -- 2.43.0

Use VIR_DEBUG instead of VIR_INFO as that's more appropriate and report relevant information for debugging. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpcivpd.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index b303e161ae..67065dec46 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -61,20 +61,20 @@ virPCIVPDResourceGetKeywordPrefix(const char *keyword) g_autofree char *key = NULL; /* Keywords must have a length of 2 bytes. */ - if (strlen(keyword) != 2) { - VIR_INFO("The keyword length is not 2 bytes: %s", keyword); - return NULL; - } else if (!(virPCIVPDResourceIsUpperOrNumber(keyword[0]) && - virPCIVPDResourceIsUpperOrNumber(keyword[1]))) { - VIR_INFO("The keyword is not comprised only of uppercase ASCII letters or digits"); - return NULL; - } + if (strlen(keyword) != 2 || + !(virPCIVPDResourceIsUpperOrNumber(keyword[0]) && + virPCIVPDResourceIsUpperOrNumber(keyword[1]))) + goto cleanup; + /* Special-case the system-specific keywords since they share the "Y" prefix with "YA". */ if (virPCIVPDResourceIsSystemKeyword(keyword) || virPCIVPDResourceIsVendorKeyword(keyword)) key = g_strndup(keyword, 1); else key = g_strndup(keyword, 2); + cleanup: + VIR_DEBUG("keyword='%s' key='%s'", keyword, NULLSTR(key)); + return g_steal_pointer(&key); } -- 2.43.0

The function are never called with NULL argument so the checks can be removed. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/node_device_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index dd174d3020..d7e1a23034 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1023,9 +1023,6 @@ virNodeDeviceCapVPDParseReadOnlyFields(xmlXPathContextPtr ctxt, virPCIVPDResourc "serial_number", "part_number", NULL}; size_t i = 0; - if (res == NULL) - return -1; - res->ro = virPCIVPDResourceRONew(); while (keywords[i]) { @@ -1061,9 +1058,6 @@ virNodeDeviceCapVPDParseXML(xmlXPathContextPtr ctxt, virPCIVPDResource **res) size_t i = 0; g_autoptr(virPCIVPDResource) newres = g_new0(virPCIVPDResource, 1); - if (res == NULL) - return -1; - if (!(newres->name = virXPathString("string(./name)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Could not read a device name from the <name> element")); -- 2.43.0

All callers satisfy these callers as they are just for programming errors. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpcivpd.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index f198faaf42..3beb405252 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -313,20 +313,7 @@ bool virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, const char *const keyword, const char *const value) { - if (!res) { - VIR_INFO("Cannot update the resource: a NULL resource pointer has been provided."); - return false; - } else if (!keyword) { - VIR_INFO("Cannot update the resource: a NULL keyword pointer has been provided."); - return false; - } - if (readOnly) { - if (!res->ro) { - VIR_INFO("Cannot update the read-only keyword: RO section not initialized."); - return false; - } - if (STREQ("EC", keyword) || STREQ("change_level", keyword)) { g_free(res->ro->change_level); res->ro->change_level = g_strdup(value); @@ -353,13 +340,7 @@ virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, /* The CP keyword is currently not supported and is skipped. */ return true; } - } else { - if (!res->rw) { - VIR_INFO("Cannot update the read-write keyword: read-write section not initialized."); - return false; - } - if (STREQ("YA", keyword) || STREQ("asset_tag", keyword)) { g_free(res->rw->asset_tag); res->rw->asset_tag = g_strdup(value); -- 2.43.0

Don't overwrite already reported errors and improve parsing of attributes. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/node_device_conf.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 0f2c341967..c68ac3af78 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1059,11 +1059,11 @@ virNodeDeviceCapVPDParseXML(xmlXPathContextPtr ctxt, virPCIVPDResource **res) if (!(newres->name = virXPathString("string(./name)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("Could not read a device name from the <name> element")); + _("Could not read a device name from the <name> element")); return -1; } - if ((nfields = virXPathNodeSet("./fields[@access]", ctxt, &nodes)) < 0) + if ((nfields = virXPathNodeSet("./fields", ctxt, &nodes)) < 0) return -1; for (i = 0; i < nfields; i++) { @@ -1071,27 +1071,19 @@ virNodeDeviceCapVPDParseXML(xmlXPathContextPtr ctxt, virPCIVPDResource **res) VIR_XPATH_NODE_AUTORESTORE(ctxt); ctxt->node = nodes[i]; - if (!(access = virXPathString("string(./@access[1])", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("VPD fields access type parsing has failed")); + + if (!(access = virXMLPropStringRequired(nodes[i], "access"))) return -1; - } if (STREQ(access, "readonly")) { - if (virNodeDeviceCapVPDParseReadOnlyFields(ctxt, newres) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Could not parse %1$s VPD resource fields"), access); + if (virNodeDeviceCapVPDParseReadOnlyFields(ctxt, newres) < 0) return -1; - } } else if (STREQ(access, "readwrite")) { - if (virNodeDeviceCapVPDParseReadWriteFields(ctxt, newres) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Could not parse %1$s VPD resource fields"), access); + if (virNodeDeviceCapVPDParseReadWriteFields(ctxt, newres) < 0) return -1; - } } else { virReportError(VIR_ERR_XML_ERROR, - _("Unsupported VPD field access type specified %1$s"), + _("Unsupported VPD field access type '%1$s'"), access); return -1; } -- 2.43.0

A checker function should not raise VIR_INFO or VIR_WARN messages especially if they contain information useful only for debugging. Turn the message into a VIR_DEBUG with universal meaning. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpci.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 6c04e57038..99e6e6cbb1 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -3081,17 +3081,12 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, bool virPCIDeviceHasVPD(virPCIDevice *dev) { - g_autofree char *vpdPath = NULL; + g_autofree char *vpdPath = virPCIFile(dev->name, "vpd"); + bool ret = virFileIsRegular(vpdPath); - vpdPath = virPCIFile(dev->name, "vpd"); - if (!virFileExists(vpdPath)) { - VIR_INFO("Device VPD file does not exist %s", vpdPath); - return false; - } else if (!virFileIsRegular(vpdPath)) { - VIR_WARN("VPD path does not point to a regular file %s", vpdPath); - return false; - } - return true; + VIR_DEBUG("path='%s', exists='%d'", vpdPath, ret); + + return ret; } /** -- 2.43.0

Until now 'virPCIDeviceGetVPD' couldn't reallistically raise an error, but that will change. Handle the errors by either resetting it if we'd be ignoring it or forward it. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/node_device_conf.c | 2 ++ tests/virpcitest.c | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index c68ac3af78..b8c91d6ecd 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -3052,6 +3052,8 @@ virNodeDeviceGetPCIVPDDynamicCap(virNodeDevCapPCIDev *devCapPCIDev) if ((res = virPCIDeviceGetVPD(pciDev))) { devCapPCIDev->flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VPD; devCapPCIDev->vpd = g_steal_pointer(&res); + } else { + virResetLastError(); } } return 0; diff --git a/tests/virpcitest.c b/tests/virpcitest.c index d69a1b5118..017c283a44 100644 --- a/tests/virpcitest.c +++ b/tests/virpcitest.c @@ -344,7 +344,8 @@ testVirPCIDeviceGetVPD(const void *opaque) if (!dev) return -1; - res = virPCIDeviceGetVPD(dev); + if (!(res = virPCIDeviceGetVPD(dev))) + return -1; /* Only basic checks - full parser validation is done elsewhere. */ if (res->ro == NULL) -- 2.43.0

Replace VIR_INFO being used as form of error reporting with proper virReportError and the usual return values. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpcivpd.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 10cabff0b9..ddd79fa8bc 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -557,10 +557,9 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re * @resDataLen: A length of the data portion of a resource. * @csum: A pointer to a 1-byte checksum. * - * Returns: a pointer to a VPDResource which needs to be freed by the caller or - * NULL if getting it failed for some reason. + * Returns: 0 on success -1 and an error on failure */ -static bool +static int virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, uint8_t *csum, virPCIVPDResource *res) { @@ -570,15 +569,16 @@ virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, g_autofree char *buf = g_malloc0(resDataLen + 1); if (virPCIVPDReadVPDBytes(vpdFileFd, (uint8_t *)buf, resDataLen, resPos, csum) < 0) - return false; + return -1; resValue = g_strdup(g_strstrip(buf)); if (!virPCIVPDResourceIsValidTextValue(resValue)) { - VIR_INFO("The string resource has invalid characters in its value"); - return false; + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to parse PCI VPD string value with invalid characters")); + return -1; } res->name = g_steal_pointer(&resValue); - return true; + return 0; } /** @@ -652,8 +652,11 @@ virPCIVPDParse(int vpdFileFd) switch (tag) { /* Large resource type which is also a string: 0x80 | 0x02 = 0x82 */ case PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_STRING_RESOURCE_FLAG: - isWellFormed = virPCIVPDParseVPDLargeResourceString(vpdFileFd, resPos, resDataLen, - &csum, res); + if (virPCIVPDParseVPDLargeResourceString(vpdFileFd, resPos, resDataLen, + &csum, res) < 0) + return NULL; + + isWellFormed = true; break; /* Large resource type which is also a VPD-R: 0x80 | 0x10 == 0x90 */ case PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG: -- 2.43.0

The 'fieldFormat' variable is guaranteed to have only the proper enum values by virPCIVPDResourceGetFieldValueFormat. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpcivpd.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index ba05014e40..25c4c2c5ec 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -470,9 +470,6 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re VIR_DEBUG("Could not determine a field value format for keyword: %s", fieldKeyword); bytesToRead = fieldDataLen; break; - default: - VIR_INFO("Unexpected field value format encountered."); - return false; } if (resPos + resDataLen < fieldPos + fieldDataLen) { -- 2.43.0

Rewrite the conditions after exiting the parser so that they are easier to understand. This partially decreases the granularity of "error" messages as they are not strictly necessary albeit for debugging. As it was already observed in this code the logic itself often does something else than the comment claims, thus the code logic is preserved. Changes: - any case when not all data was processed is agregated together and gets a common "error" message - absence of 'checksum' field is checked separately - helper variables are removed as they are no longer needed Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpcivpd.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 60e520c46f..be19f7b747 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -415,10 +415,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re g_autofree uint8_t *buf = g_malloc0(PCI_VPD_MAX_FIELD_SIZE + 1); uint16_t fieldDataLen = 0, bytesToRead = 0; uint16_t fieldPos = resPos; - bool hasChecksum = false; - bool hasRW = false; - bool endReached = false; /* Note the equal sign - fields may have a zero length in which case they will * just occupy 3 header bytes. In the in case of the RW field this may mean that @@ -507,7 +504,9 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR: /* Skip the read-write space since it is used for indication only. */ - hasRW = true; + /* The lack of RW is allowed on purpose in the read-write section since some vendors + * violate the PCI/PCIe specs and do not include it, however, this does not prevent parsing + * of valid data. */ goto done; case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD: @@ -531,6 +530,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re if (!res->rw) res->rw = virPCIVPDResourceRWNew(); } + /* The field format, keyword and value are determined. Attempt to update the resource. */ virPCIVPDResourceUpdateKeyword(res, readOnly, fieldKeyword, fieldValue); } @@ -538,27 +538,21 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re done: /* May have exited the loop prematurely in case RV or RW were encountered and * they were not the last fields in the section. */ - endReached = (fieldPos >= resPos + resDataLen); - if (readOnly && !(hasChecksum && endReached)) { - VIR_DEBUG("VPD-R does not contain the mandatory RV field as the last field"); + if ((fieldPos < resPos + resDataLen)) { + /* unparsed data still present */ + VIR_DEBUG("PCI VPD data parsing failed"); + return false; + } + + if (readOnly && !hasChecksum) { + VIR_DEBUG("VPD-R does not contain the mandatory checksum"); return false; - } else if (!readOnly && !endReached) { - /* The lack of RW is allowed on purpose in the read-write section since some vendors - * violate the PCI/PCIe specs and do not include it, however, this does not prevent parsing - * of valid data. If the RW is present, however, we make sure it is the last field in - * the read-write section. */ - if (hasRW) { - VIR_DEBUG("VPD-W section parsing ended prematurely (RW is not the last field)."); - return false; - } else { - VIR_DEBUG("VPD-W section parsing ended prematurely."); - return false; - } } return true; } + /** * virPCIVPDParseVPDLargeResourceString: * @vpdFileFd: A file descriptor associated with a file containing PCI device VPD. -- 2.43.0

Remove the wannabe error reporting via 'VIR_DEBUG/VIR_INFO' in favor of proper errors. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virpcivpd.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 4a440c2aea..16df468875 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -616,7 +616,7 @@ virPCIVPDParse(int vpdFileFd) uint16_t resPos = 0, resDataLen; uint8_t tag = 0; - bool endResReached = false, hasReadOnly = false; + bool hasReadOnly = false; g_autoptr(virPCIVPDResource) res = g_new0(virPCIVPDResource, 1); @@ -628,9 +628,8 @@ virPCIVPDParse(int vpdFileFd) /* 0x80 == 0b10000000 - the large resource data type flag. */ if (tag & PCI_VPD_LARGE_RESOURCE_FLAG) { if (resPos > PCI_VPD_ADDR_MASK + 1 - 3) { - /* Bail if the large resource starts at the position - * where the end tag should be. */ - break; + /* Bail if the large resource starts at the position where the end tag should be. */ + goto malformed; } /* Read the two length bytes of the large resource record. */ @@ -649,14 +648,21 @@ virPCIVPDParse(int vpdFileFd) /* Change the position to the byte past the byte containing tag and length bits. */ resPos += 1; } + if (tag == PCI_VPD_RESOURCE_END_TAG) { /* Stop VPD traversal since the end tag was encountered. */ - endResReached = true; - break; + if (!hasReadOnly) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: missing read-only section")); + return NULL; + } + + return g_steal_pointer(&res); } + if (resDataLen > PCI_VPD_ADDR_MASK + 1 - resPos) { /* Bail if the resource is too long to fit into the VPD address space. */ - break; + goto malformed; } switch (tag) { @@ -686,22 +692,16 @@ virPCIVPDParse(int vpdFileFd) /* While we cannot parse unknown resource types, they can still be skipped * based on the header and data length. */ VIR_DEBUG("Encountered an unexpected VPD resource tag: %#x", tag); - resPos += resDataLen; - continue; } /* Continue processing other resource records. */ resPos += resDataLen; } - if (!hasReadOnly) { - VIR_DEBUG("Encountered an invalid VPD: does not have a VPD-R record"); - return NULL; - } else if (!endResReached) { - /* Does not have an end tag. */ - VIR_DEBUG("Encountered an invalid VPD"); - return NULL; - } - return g_steal_pointer(&res); + + malformed: + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: malformed data")); + return NULL; } #else /* ! __linux__ */ -- 2.43.0

On Wed, Jan 31, 2024 at 09:06:06 +0100, Ján Tomko wrote: Good question. Originally this was the code that reminded me that I can't just add proper error reporting to that function which I did originally. I decided to keep this to preserve this fact in the code, because the comments/documentation for this particular code are/were in many cases misleading. I guess converting this expectation into a comment would be possible, which would allow us to remove the test. I can do this as a follow up if you are okay with this solution.