[PATCH 00/31] pci vpd: Fix broken XML formatter and refactor questionable error reporting
The first part of the series fixes the XML formatter of nodedevs to not generate invalid XML if a PCI device custom field contains '>' which would be printed unescaped The rest fixes questionable and broken error reporting from the pci vpd device code which actually parses the above data. https://gitlab.com/pipo.sk/libvirt/-/pipelines/1157912774 Peter Krempa (31): virPCIVPDResourceIsValidTextValue: Adjust comment to reflect actual code util: pcivpd: Refactor virPCIVPDResourceIsValidTextValue virNodeDeviceCapVPDFormatCustom*: Escape unsanitized strings virNodeDeviceCapVPDFormat: Properly escape system-originated strings schema: nodedev: Adjust allowed characters in 'vpdFieldValueFormat' tests: Test the previously mishandled PCI VPD characters Don't overwrite error message from 'virXPathNodeSet' tests: virpcivpdtest: Remove 'testVirPCIVPDReadVPDBytes' case util: virpcivpd: Unexport 'virPCIVPDReadVPDBytes' util: pcivpd: Unexport virPCIVPDParseVPDLargeResourceFields tests: virpcivpd: Remove 'testVirPCIVPDParseVPDStringResource' case util: virpcivpd: Unexport 'virPCIVPDParseVPDLargeResourceString' virPCIVPDResourceGetKeywordPrefix: Fix logging util: virpcivpd: Remove return value from virPCIVPDResourceCustomUpsertValue conf: virNodeDeviceCapVPDParse*: Remove pointless NULL checks virpcivpdtest: testPCIVPDResourceBasic: Remove tests for uninitialized 'ro'/'rw' section util: virPCIVPDResourceUpdateKeyword: Remove impossible checks conf: node_device: Refactor 'virNodeDeviceCapVPDParseCustomFields' to fix error reporting virNodeDeviceCapVPDParseXML: Fix error reporting util: virpcivpd: Remove return value from virPCIVPDResourceUpdateKeyword virPCIDeviceHasVPD: Refactor "debug" messages virPCIDeviceGetVPD: Fix multiple error handling bugs virPCIDeviceGetVPD: Handle errors in callers virPCIVPDReadVPDBytes: Refactor error handling virPCIVPDParseVPDLargeResourceString: Properly report errors virPCIVPDParseVPDLargeResourceFields: Merge logic conditions virPCIVPDParseVPDLargeResourceFields: Remove impossible 'default' swithch case virPCIVPDParseVPDLargeResourceFields: Refactor processing of read data virPCIVPDParseVPDLargeResourceFields: Refactor return logic virPCIVPDParseVPDLargeResourceFields: Report proper errors virPCIVPDParse: Do reasonable error reporting src/conf/domain_conf.c | 78 +-- src/conf/network_conf.c | 80 +-- src/conf/node_device_conf.c | 177 +++---- src/conf/numa_conf.c | 15 +- src/conf/schemas/nodedev.rng | 2 +- src/cpu/cpu_ppc64.c | 5 +- src/libvirt_private.syms | 3 - 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/util/virpci.c | 34 +- src/util/virpcivpd.c | 454 ++++++++---------- src/util/virpcivpd.h | 8 +- src/util/virpcivpdpriv.h | 11 +- src/vz/vz_sdk.c | 5 +- .../pci_0000_42_00_0_vpd.xml | 4 +- tests/virpcimock.c | 4 +- tests/virpcitest.c | 3 +- tests/virpcivpdtest.c | 160 +----- 20 files changed, 386 insertions(+), 720 deletions(-) -- 2.43.0
The function does not reject '&', '<', '>' contrary to what it actually states. Move and adjust the comment. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 function is never called with NULL argument. Remove the check and refactor the rest including the debug statement. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 248a9b2790..81c7c317b3 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -175,23 +175,18 @@ virPCIVPDResourceGetFieldValueFormat(const char *keyword) bool virPCIVPDResourceIsValidTextValue(const char *value) { - size_t i = 0; + const char *v; + bool ret = true; - if (value == NULL) - return false; - - /* An empty string is a valid value. */ - if (STREQ(value, "")) - return true; - - while (i < strlen(value)) { - if (!g_ascii_isprint(value[i])) { - VIR_DEBUG("The provided value contains non-ASCII printable characters: %s", value); - return false; + for (v = value; *v; v++) { + if (!g_ascii_isprint(*v)) { + ret = false; + break; } - ++i; } - return true; + + VIR_DEBUG("val='%s' ret='%d'", value, ret); + return ret; } void -- 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@redhat.com> --- 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
Similarly to previous commit other specific fields which come from the system data and aren't sanitized enough to be safe for XML were also formatted via virBufferAsprintf. Other static and safe strings used virBufferEscapeString instead of virBufferAddLit. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/node_device_conf.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 87c046e571..95de77abe9 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -270,14 +270,6 @@ virNodeDeviceCapVPDFormatCustomSystemField(virPCIVPDResourceCustom *field, virBu virNodeDeviceCapVPDFormatCustomField(buf, "system_field", field); } -static inline void -virNodeDeviceCapVPDFormatRegularField(virBuffer *buf, const char *keyword, const char *value) -{ - if (keyword == NULL || value == NULL) - return; - - virBufferAsprintf(buf, "<%s>%s</%s>\n", keyword, value, keyword); -} static void virNodeDeviceCapVPDFormat(virBuffer *buf, virPCIVPDResource *res) @@ -290,31 +282,33 @@ virNodeDeviceCapVPDFormat(virBuffer *buf, virPCIVPDResource *res) virBufferEscapeString(buf, "<name>%s</name>\n", res->name); if (res->ro != NULL) { - virBufferEscapeString(buf, "<fields access='%s'>\n", "readonly"); - + virBufferAddLit(buf, "<fields access='readonly'>\n"); virBufferAdjustIndent(buf, 2); - virNodeDeviceCapVPDFormatRegularField(buf, "change_level", res->ro->change_level); - virNodeDeviceCapVPDFormatRegularField(buf, "manufacture_id", res->ro->manufacture_id); - virNodeDeviceCapVPDFormatRegularField(buf, "part_number", res->ro->part_number); - virNodeDeviceCapVPDFormatRegularField(buf, "serial_number", res->ro->serial_number); + + virBufferEscapeString(buf, "<change_level>%s</change_level>\n", res->ro->change_level); + virBufferEscapeString(buf, "<manufacture_id>%s</manufacture_id>\n", res->ro->manufacture_id); + virBufferEscapeString(buf, "<part_number>%s</part_number>\n", res->ro->part_number); + virBufferEscapeString(buf, "<serial_number>%s</serial_number>\n", res->ro->serial_number); + g_ptr_array_foreach(res->ro->vendor_specific, (GFunc)virNodeDeviceCapVPDFormatCustomVendorField, buf); - virBufferAdjustIndent(buf, -2); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</fields>\n"); } if (res->rw != NULL) { - virBufferEscapeString(buf, "<fields access='%s'>\n", "readwrite"); - + virBufferAddLit(buf, "<fields access='readwrite'>\n"); virBufferAdjustIndent(buf, 2); - virNodeDeviceCapVPDFormatRegularField(buf, "asset_tag", res->rw->asset_tag); + + virBufferEscapeString(buf, "<asset_tag>%s</asset_tag>\n", res->rw->asset_tag); + g_ptr_array_foreach(res->rw->vendor_specific, (GFunc)virNodeDeviceCapVPDFormatCustomVendorField, buf); g_ptr_array_foreach(res->rw->system_specific, (GFunc)virNodeDeviceCapVPDFormatCustomSystemField, buf); - virBufferAdjustIndent(buf, -2); + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</fields>\n"); } -- 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@redhat.com> --- 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
Modify the test data to validate '<>' and other characters. Unfortunately the test suite doesn't have a proper end-to-end test, thus we just add a XML->XML variant and also add data to the binary parser. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml | 4 ++-- tests/virpcimock.c | 4 ++-- tests/virpcivpdtest.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml index 8b56e4f6b4..c9a2901381 100644 --- a/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml +++ b/tests/nodedevschemadata/pci_0000_42_00_0_vpd.xml @@ -15,7 +15,7 @@ <change_level>B1</change_level> <manufacture_id>foobar</manufacture_id> <part_number>MBF2H332A-AEEOT</part_number> - <serial_number>MT2113X00000</serial_number> + <serial_number>MT2113X00000><</serial_number> <vendor_field index='0'>PCIeGen4 x8</vendor_field> <vendor_field index='2'>MBF2H332A-AEEOT</vendor_field> <vendor_field index='3'>3c53d07eec484d8aab34dabd24fe575aa</vendor_field> @@ -25,7 +25,7 @@ <asset_tag>fooasset</asset_tag> <vendor_field index='0'>vendorfield0</vendor_field> <vendor_field index='2'>vendorfield2</vendor_field> - <vendor_field index='A'>vendorfieldA</vendor_field> + <vendor_field index='A'>!@#$./><</vendor_field> <system_field index='B'>systemfieldB</system_field> <system_field index='0'>systemfield0</system_field> </fields> diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 13b37bb23d..2f98b0cf13 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -966,9 +966,9 @@ init_env(void) 't', 'e', 's', 't', 'n', 'a', 'm', 'e', PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_ONLY_LARGE_RESOURCE_FLAG, 0x16, 0x00, 'P', 'N', 0x02, '4', '2', - 'E', 'C', 0x04, '4', '2', '4', '2', + 'E', 'C', 0x04, '4', '<', '>', '2', 'V', 'A', 0x02, 'E', 'X', - 'R', 'V', 0x02, 0x31, 0x00, + 'R', 'V', 0x02, 0x1D, 0x00, PCI_VPD_RESOURCE_END_VAL }; struct pciVPD exampleVPD = { diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index b4dd68b7aa..ae5772d3f5 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -424,7 +424,7 @@ testPCIVPDGetFieldValueFormat(const void *data G_GNUC_UNUSED) # define VPD_W_EXAMPLE_FIELDS \ 'V', 'Z', 0x02, '4', '2', \ - 'Y', 'A', 0x04, 'I', 'D', '4', '2', \ + 'Y', 'A', 0x04, '!', '<', '>', ':', \ 'Y', 'F', 0x02, 'E', 'X', \ 'Y', 'E', 0x00, \ 'R', 'W', 0x02, 0x00, 0x00 @@ -579,7 +579,7 @@ testVirPCIVPDParseFullVPD(const void *opaque G_GNUC_UNUSED) if (testVirPCIVPDValidateExampleReadOnlyFields(res)) return -1; - if (STRNEQ_NULLABLE(res->rw->asset_tag, "ID42")) + if (STRNEQ_NULLABLE(res->rw->asset_tag, "!<>:")) return -1; if (!res->rw->vendor_specific) -- 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@redhat.com> --- 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, §ionNodes); - 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
On a Tuesday in 2024, Peter Krempa wrote:
'virXPathNodeSet' returns -1 only when 'ctxt' or 'xpath' are NULL or when the 'xpath' string is invalid. Both are programmign errors. It
*programming
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@redhat.com> --- 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(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The case checks only the 'virPCIVPDReadVPDBytes' which is also tested multiple times via 'virPCIVPDParse' as it's used to read the data, thus having a special case for this is pointless. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virpcivpdtest.c | 41 ----------------------------------------- 1 file changed, 41 deletions(-) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index ae5772d3f5..aadd1b222b 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -429,45 +429,6 @@ testPCIVPDGetFieldValueFormat(const void *data G_GNUC_UNUSED) 'Y', 'E', 0x00, \ 'R', 'W', 0x02, 0x00, 0x00 -static int -testVirPCIVPDReadVPDBytes(const void *opaque G_GNUC_UNUSED) -{ - VIR_AUTOCLOSE fd = -1; - g_autofree uint8_t *buf = NULL; - uint8_t csum = 0; - size_t readBytes = 0; - size_t dataLen = 0; - - /* An example of a valid VPD record with one VPD-R resource and 2 fields. */ - uint8_t fullVPDExample[] = { - VPD_STRING_RESOURCE_EXAMPLE_HEADER, VPD_STRING_RESOURCE_EXAMPLE_DATA, - VPD_R_FIELDS_EXAMPLE_HEADER, VPD_R_FIELDS_EXAMPLE_DATA, - PCI_VPD_RESOURCE_END_VAL - }; - dataLen = G_N_ELEMENTS(fullVPDExample) - 2; - buf = g_malloc0(dataLen); - - if ((fd = virCreateAnonymousFile(fullVPDExample, dataLen)) < 0) - return -1; - - readBytes = virPCIVPDReadVPDBytes(fd, buf, dataLen, 0, &csum); - - if (readBytes != dataLen) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "The number of bytes read %zu is lower than expected %zu ", - readBytes, dataLen); - return -1; - } - - if (csum) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "The sum of all VPD bytes up to and including the checksum byte" - "is equal to zero: 0x%02x", csum); - return -1; - } - return 0; -} - static int testVirPCIVPDParseVPDStringResource(const void *opaque G_GNUC_UNUSED) { @@ -1003,8 +964,6 @@ mymain(void) if (virTestRun("Determining a field value format by a key ", testPCIVPDGetFieldValueFormat, NULL) < 0) ret = -1; - if (virTestRun("Reading VPD bytes ", testVirPCIVPDReadVPDBytes, 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 ", -- 2.43.0
The function is no longer used outside of virpcivpd.c Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 function is not used in other files. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virpcivpd.c | 15 +-------------- src/util/virpcivpdpriv.h | 3 --- 3 files changed, 1 insertion(+), 18 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dbc4e26d79..89b0d01de6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3700,7 +3700,6 @@ virVHBAPathExists; # util/virpcivpd.h virPCIVPDParse; -virPCIVPDParseVPDLargeResourceFields; virPCIVPDParseVPDLargeResourceString; virPCIVPDResourceCustomCompareIndex; virPCIVPDResourceCustomFree; diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 373321a836..510be65cb6 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -436,7 +436,7 @@ virPCIVPDReadVPDBytes(int vpdFileFd, uint8_t *buf, size_t count, off_t offset, u * Returns: a pointer to a VPDResource which needs to be freed by the caller or * NULL if getting it failed for some reason. */ -bool +static bool virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, bool readOnly, uint8_t *csum, virPCIVPDResource *res) { @@ -744,19 +744,6 @@ virPCIVPDParseVPDLargeResourceString(int vpdFileFd G_GNUC_UNUSED, return false; } -bool -virPCIVPDParseVPDLargeResourceFields(int vpdFileFd G_GNUC_UNUSED, - uint16_t resPos G_GNUC_UNUSED, - uint16_t resDataLen G_GNUC_UNUSED, - bool readOnly G_GNUC_UNUSED, - uint8_t *csum G_GNUC_UNUSED, - virPCIVPDResource *res G_GNUC_UNUSED) -{ - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("PCI VPD reporting not available on this platform")); - return false; -} - virPCIVPDResource * virPCIVPDParse(int vpdFileFd G_GNUC_UNUSED) { diff --git a/src/util/virpcivpdpriv.h b/src/util/virpcivpdpriv.h index 17e6e14ab7..d84f1e9c8a 100644 --- a/src/util/virpcivpdpriv.h +++ b/src/util/virpcivpdpriv.h @@ -69,8 +69,5 @@ virPCIVPDResourceCustomCompareIndex(virPCIVPDResourceCustom *a, virPCIVPDResourc bool virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const value); -bool virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, - bool readOnly, uint8_t *csum, virPCIVPDResource *res); - bool virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, 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@redhat.com> --- 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
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 89b0d01de6..035f8c7b5d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3700,7 +3700,6 @@ virVHBAPathExists; # util/virpcivpd.h virPCIVPDParse; -virPCIVPDParseVPDLargeResourceString; virPCIVPDResourceCustomCompareIndex; virPCIVPDResourceCustomFree; virPCIVPDResourceCustomUpsertValue; diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 510be65cb6..b303e161ae 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -595,7 +595,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re * Returns: a pointer to a VPDResource which needs to be freed by the caller or * NULL if getting it failed for some reason. */ -bool +static bool virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, uint8_t *csum, virPCIVPDResource *res) { @@ -732,18 +732,6 @@ virPCIVPDParse(int vpdFileFd) #else /* ! __linux__ */ -bool -virPCIVPDParseVPDLargeResourceString(int vpdFileFd G_GNUC_UNUSED, - uint16_t resPos G_GNUC_UNUSED, - uint16_t resDataLen G_GNUC_UNUSED, - uint8_t *csum G_GNUC_UNUSED, - virPCIVPDResource *res G_GNUC_UNUSED) -{ - virReportError(VIR_ERR_NO_SUPPORT, "%s", - _("PCI VPD reporting not available on this platform")); - return false; -} - virPCIVPDResource * virPCIVPDParse(int vpdFileFd G_GNUC_UNUSED) { diff --git a/src/util/virpcivpdpriv.h b/src/util/virpcivpdpriv.h index d84f1e9c8a..617991930b 100644 --- a/src/util/virpcivpdpriv.h +++ b/src/util/virpcivpdpriv.h @@ -68,6 +68,3 @@ virPCIVPDResourceCustomCompareIndex(virPCIVPDResourceCustom *a, virPCIVPDResourc bool virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const value); - -bool virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, - uint8_t *csum, virPCIVPDResource *res); -- 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@redhat.com> --- 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
None of the callers pass NULL, so the NULL check is pointless. Remove it an remove the return value. The function is exported only for use in 'virpcivpdtest' thus marking the arguments as NONNULL is unnecessary. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 18 ++++-------------- src/util/virpcivpdpriv.h | 2 +- tests/virpcivpdtest.c | 12 ++++-------- 3 files changed, 9 insertions(+), 23 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 67065dec46..f198faaf42 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -270,7 +270,7 @@ virPCIVPDResourceCustomCompareIndex(virPCIVPDResourceCustom *a, virPCIVPDResourc * * Returns: true if a value has been updated successfully, false otherwise. */ -bool +void virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const value) { g_autoptr(virPCIVPDResourceCustom) custom = NULL; @@ -278,9 +278,6 @@ virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const guint pos = 0; bool found = false; - if (arr == NULL || value == NULL) - return false; - custom = g_new0(virPCIVPDResourceCustom, 1); custom->idx = index; custom->value = g_strdup(value); @@ -294,7 +291,6 @@ virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const } else { g_ptr_array_add(arr, g_steal_pointer(&custom)); } - return true; } /** @@ -348,9 +344,7 @@ virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, res->ro->serial_number = g_strdup(value); return true; } else if (virPCIVPDResourceIsVendorKeyword(keyword)) { - if (!virPCIVPDResourceCustomUpsertValue(res->ro->vendor_specific, keyword[1], value)) { - return false; - } + virPCIVPDResourceCustomUpsertValue(res->ro->vendor_specific, keyword[1], value); return true; } else if (STREQ("FG", keyword) || STREQ("LC", keyword) || STREQ("PG", keyword)) { /* Legacy PICMIG keywords are skipped on purpose. */ @@ -371,14 +365,10 @@ virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, res->rw->asset_tag = g_strdup(value); return true; } else if (virPCIVPDResourceIsVendorKeyword(keyword)) { - if (!virPCIVPDResourceCustomUpsertValue(res->rw->vendor_specific, keyword[1], value)) { - return false; - } + virPCIVPDResourceCustomUpsertValue(res->rw->vendor_specific, keyword[1], value); return true; } else if (virPCIVPDResourceIsSystemKeyword(keyword)) { - if (!virPCIVPDResourceCustomUpsertValue(res->rw->system_specific, keyword[1], value)) { - return false; - } + virPCIVPDResourceCustomUpsertValue(res->rw->system_specific, keyword[1], value); return true; } } diff --git a/src/util/virpcivpdpriv.h b/src/util/virpcivpdpriv.h index 617991930b..f26b64139d 100644 --- a/src/util/virpcivpdpriv.h +++ b/src/util/virpcivpdpriv.h @@ -66,5 +66,5 @@ bool virPCIVPDResourceIsValidTextValue(const char *value); gboolean virPCIVPDResourceCustomCompareIndex(virPCIVPDResourceCustom *a, virPCIVPDResourceCustom *b); -bool +void virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const value); diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index fddb42f52c..8a2f337e85 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -244,8 +244,7 @@ testPCIVPDResourceCustomUpsertValue(const void *data G_GNUC_UNUSED) { g_autoptr(GPtrArray) arr = g_ptr_array_new_full(0, (GDestroyNotify)virPCIVPDResourceCustomFree); virPCIVPDResourceCustom *custom = NULL; - if (!virPCIVPDResourceCustomUpsertValue(arr, 'A', "testval")) - return -1; + virPCIVPDResourceCustomUpsertValue(arr, 'A', "testval"); if (arr->len != 1) return -1; @@ -255,8 +254,7 @@ testPCIVPDResourceCustomUpsertValue(const void *data G_GNUC_UNUSED) return -1; /* Idempotency */ - if (!virPCIVPDResourceCustomUpsertValue(arr, 'A', "testval")) - return -1; + virPCIVPDResourceCustomUpsertValue(arr, 'A', "testval"); if (arr->len != 1) return -1; @@ -266,8 +264,7 @@ testPCIVPDResourceCustomUpsertValue(const void *data G_GNUC_UNUSED) return -1; /* Existing value updates. */ - if (!virPCIVPDResourceCustomUpsertValue(arr, 'A', "testvalnew")) - return -1; + virPCIVPDResourceCustomUpsertValue(arr, 'A', "testvalnew"); if (arr->len != 1) return -1; @@ -277,8 +274,7 @@ testPCIVPDResourceCustomUpsertValue(const void *data G_GNUC_UNUSED) return -1; /* Inserting multiple values */ - if (!virPCIVPDResourceCustomUpsertValue(arr, '1', "42")) - return -1; + virPCIVPDResourceCustomUpsertValue(arr, '1', "42"); if (arr->len != 2) return -1; -- 2.43.0
The function are never called with NULL argument so the checks can be removed. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
This is a synthetic case which tests the behaviour if the 'ro' or 'rw' struct members are uninitialized, basically excercising only a pointless programming-error NULL check in 'virPCIVPDResourceUpdateKeyword' as real usage does always pass a proper pointer. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virpcivpdtest.c | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 8a2f337e85..20545759d5 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -64,11 +64,6 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) {.keyword = "SN", .value = "serial2", .actual = &ro->serial_number}, {.keyword = "serial_number", .value = "serial3", .actual = &ro->serial_number}, }; - const TestPCIVPDKeywordValue readWriteCases[] = { - {.keyword = "YA", .value = "tag1", .actual = &ro->change_level}, - {.keyword = "YA", .value = "tag2", .actual = &ro->change_level}, - {.keyword = "asset_tag", .value = "tag3", .actual = &ro->change_level}, - }; const TestPCIVPDKeywordValue unsupportedFieldCases[] = { {.keyword = "FG", .value = "42", .actual = NULL}, {.keyword = "LC", .value = "42", .actual = NULL}, @@ -77,7 +72,6 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) {.keyword = "EX", .value = "42", .actual = NULL}, }; size_t numROCases = G_N_ELEMENTS(readOnlyCases); - size_t numRWCases = G_N_ELEMENTS(readWriteCases); size_t numUnsupportedCases = G_N_ELEMENTS(unsupportedFieldCases); g_autoptr(virPCIVPDResource) res = g_new0(virPCIVPDResource, 1); virPCIVPDResourceCustom *custom = NULL; @@ -85,20 +79,6 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) g_autofree char *val = g_strdup("testval"); res->name = g_steal_pointer(&val); - /* RO has not been initialized - make sure updates fail. */ - for (i = 0; i < numROCases; ++i) { - if (virPCIVPDResourceUpdateKeyword(res, true, - readOnlyCases[i].keyword, - readOnlyCases[i].value)) - return -1; - } - /* RW has not been initialized - make sure updates fail. */ - for (i = 0; i < numRWCases; ++i) { - if (virPCIVPDResourceUpdateKeyword(res, false, - readWriteCases[i].keyword, - readWriteCases[i].value)) - return -1; - } /* Initialize RO */ res->ro = g_steal_pointer(&ro); @@ -131,13 +111,6 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) return -1; } - /* Check that RW updates fail if RW has not been initialized. */ - if (virPCIVPDResourceUpdateKeyword(res, false, "YA", "tag1")) - return -1; - - if (virPCIVPDResourceUpdateKeyword(res, false, "asset_tag", "tag1")) - return -1; - /* Initialize RW */ res->rw = g_steal_pointer(&rw); if (!virPCIVPDResourceUpdateKeyword(res, false, "YA", "tag1") -- 2.43.0
All callers satisfy these callers as they are just for programming errors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
On a Tuesday in 2024, Peter Krempa wrote:
All callers satisfy these callers as they are just for programming
s/callers/checks/? or conditions?
errors.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 19 ------------------- 1 file changed, 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The errors raised in virNodeDeviceCapVPDParseCustomFields were actually ignored by continuing the parse rather than raised. Rather than just replace 'continue' by 'return -1' this patch refactors the whole parser to simplify it as well as report reasonable errors. Parsing of individual fields is done without XPath and is extracted into a common helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/node_device_conf.c | 81 ++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 41 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index d7e1a23034..0f2c341967 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -953,63 +953,62 @@ virNodeDevCapMdevTypesParseXML(xmlXPathContextPtr ctxt, return ret; } + static int -virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, virPCIVPDResource *res, bool readOnly) +virNodeDeviceCapVPDParseCustomFieldOne(xmlNodePtr node, + virPCIVPDResource *res, + bool read_only, + const char keyword_prefix) +{ + g_autofree char *value = NULL; + g_autofree char *index = NULL; + g_autofree char *keyword = NULL; + + if (!(index = virXMLPropStringRequired(node, "index"))) + return -1; + + if (strlen(index) != 1) { + virReportError(VIR_ERR_XML_ERROR, + _("'%1$s' 'index' value '%2$s' malformed"), + node->name, index); + return -1; + } + + keyword = g_strdup_printf("%c%c", keyword_prefix, index[0]); + + if (!(value = virXMLNodeContentString(node))) + return -1; + + virPCIVPDResourceUpdateKeyword(res, read_only, keyword, value); + return 0; +} + + +static int +virNodeDeviceCapVPDParseCustomFields(xmlXPathContextPtr ctxt, + virPCIVPDResource *res, + bool readOnly) { int nfields = -1; g_autofree xmlNodePtr *nodes = NULL; size_t i = 0; - if ((nfields = virXPathNodeSet("./vendor_field[@index]", ctxt, &nodes)) < 0) + if ((nfields = virXPathNodeSet("./vendor_field", ctxt, &nodes)) < 0) return -1; for (i = 0; i < nfields; i++) { - g_autofree char *value = NULL; - g_autofree char *index = NULL; - VIR_XPATH_NODE_AUTORESTORE(ctxt) - g_autofree char *keyword = NULL; - - ctxt->node = nodes[i]; - if (!(index = virXPathString("string(./@index[1])", ctxt)) || - strlen(index) > 1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("<vendor_field> evaluation has failed")); - continue; - } - if (!(value = virXPathString("string(./text())", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("<vendor_field> value evaluation has failed")); - continue; - } - keyword = g_strdup_printf("V%c", index[0]); - virPCIVPDResourceUpdateKeyword(res, readOnly, keyword, value); + if (virNodeDeviceCapVPDParseCustomFieldOne(nodes[i], res, readOnly, 'V') < 0) + return -1; } VIR_FREE(nodes); if (!readOnly) { - if ((nfields = virXPathNodeSet("./system_field[@index]", ctxt, &nodes)) < 0) + if ((nfields = virXPathNodeSet("./system_field", ctxt, &nodes)) < 0) return -1; for (i = 0; i < nfields; i++) { - g_autofree char *value = NULL; - g_autofree char *index = NULL; - g_autofree char *keyword = NULL; - VIR_XPATH_NODE_AUTORESTORE(ctxt); - - ctxt->node = nodes[i]; - if (!(index = virXPathString("string(./@index[1])", ctxt)) || - strlen(index) > 1) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("<system_field> evaluation has failed")); - continue; - } - if (!(value = virXPathString("string(./text())", ctxt))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("<system_field> value evaluation has failed")); - continue; - } - keyword = g_strdup_printf("Y%c", index[0]); - virPCIVPDResourceUpdateKeyword(res, readOnly, keyword, value); + if (virNodeDeviceCapVPDParseCustomFieldOne(nodes[i], res, readOnly, 'Y') < 0) + return -1; } } -- 2.43.0
Don't overwrite already reported errors and improve parsing of attributes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
The function always succeeded and after the removal of programing error checks doesn't even have a 'return false' case. Additionally one of the tests in 'virpcivpdtest' tested that this function never failed on wrong data. Embrace this logic and remove the return value and adjust logging to VIR_DEBUG level to avoid spamming logs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 31 +++++++++++-------------------- src/util/virpcivpd.h | 8 +++++--- tests/virpcivpdtest.c | 38 ++++++++++++++++++-------------------- 3 files changed, 34 insertions(+), 43 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 3beb405252..0021a88f2d 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -307,54 +307,48 @@ virPCIVPDResourceCustomUpsertValue(GPtrArray *arr, char index, const char *const * used in XML elements. For vendor-specific and system-specific keywords only V%s and Y%s * (except "YA" which is an asset tag) formatted values are accepted. * - * Returns: true if a keyword has been updated successfully, false otherwise. + * Unknown or malformed values are ignored. */ -bool -virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, - const char *const keyword, const char *const value) +void +virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, + const bool readOnly, + const char *const keyword, + const char *const value) { if (readOnly) { if (STREQ("EC", keyword) || STREQ("change_level", keyword)) { g_free(res->ro->change_level); res->ro->change_level = g_strdup(value); - return true; } else if (STREQ("MN", keyword) || STREQ("manufacture_id", keyword)) { g_free(res->ro->manufacture_id); res->ro->manufacture_id = g_strdup(value); - return true; } else if (STREQ("PN", keyword) || STREQ("part_number", keyword)) { g_free(res->ro->part_number); res->ro->part_number = g_strdup(value); - return true; } else if (STREQ("SN", keyword) || STREQ("serial_number", keyword)) { g_free(res->ro->serial_number); res->ro->serial_number = g_strdup(value); - return true; } else if (virPCIVPDResourceIsVendorKeyword(keyword)) { virPCIVPDResourceCustomUpsertValue(res->ro->vendor_specific, keyword[1], value); - return true; } else if (STREQ("FG", keyword) || STREQ("LC", keyword) || STREQ("PG", keyword)) { /* Legacy PICMIG keywords are skipped on purpose. */ - return true; } else if (STREQ("CP", keyword)) { /* The CP keyword is currently not supported and is skipped. */ - return true; + } else { + VIR_DEBUG("unhandled PCI VPD r/o keyword '%s'(val='%s')", keyword, value); } } else { if (STREQ("YA", keyword) || STREQ("asset_tag", keyword)) { g_free(res->rw->asset_tag); res->rw->asset_tag = g_strdup(value); - return true; } else if (virPCIVPDResourceIsVendorKeyword(keyword)) { virPCIVPDResourceCustomUpsertValue(res->rw->vendor_specific, keyword[1], value); - return true; } else if (virPCIVPDResourceIsSystemKeyword(keyword)) { virPCIVPDResourceCustomUpsertValue(res->rw->system_specific, keyword[1], value); - return true; + } else { + VIR_DEBUG("unhandled PCI VPD r/w keyword '%s'(val='%s')", keyword, value); } } - VIR_WARN("Tried to update an unsupported keyword %s: skipping.", keyword); - return true; } #ifdef __linux__ @@ -527,10 +521,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re res->rw = virPCIVPDResourceRWNew(); } /* The field format, keyword and value are determined. Attempt to update the resource. */ - if (!virPCIVPDResourceUpdateKeyword(res, readOnly, fieldKeyword, fieldValue)) { - VIR_INFO("Could not update the VPD resource keyword: %s", fieldKeyword); - return false; - } + virPCIVPDResourceUpdateKeyword(res, readOnly, fieldKeyword, fieldValue); } /* May have exited the loop prematurely in case RV or RW were encountered and diff --git a/src/util/virpcivpd.h b/src/util/virpcivpd.h index 9bfec43e03..d8d3dd3075 100644 --- a/src/util/virpcivpd.h +++ b/src/util/virpcivpd.h @@ -67,9 +67,11 @@ void virPCIVPDResourceRWFree(virPCIVPDResourceRW *rw); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virPCIVPDResourceRW, virPCIVPDResourceRWFree); -bool -virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, const bool readOnly, - const char *const keyword, const char *const value); +void +virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, + const bool readOnly, + const char *const keyword, + const char *const value); void virPCIVPDResourceCustomFree(virPCIVPDResourceCustom *custom); diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 20545759d5..a6311bfe76 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -84,17 +84,15 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) /* Update keywords one by one and compare actual values with the expected ones. */ for (i = 0; i < numROCases; ++i) { - if (!virPCIVPDResourceUpdateKeyword(res, true, - readOnlyCases[i].keyword, - readOnlyCases[i].value)) - return -1; + virPCIVPDResourceUpdateKeyword(res, true, + readOnlyCases[i].keyword, + readOnlyCases[i].value); if (STRNEQ(readOnlyCases[i].value, *readOnlyCases[i].actual)) return -1; } /* Do a basic vendor field check. */ - if (!virPCIVPDResourceUpdateKeyword(res, true, "V0", "vendor0")) - return -1; + virPCIVPDResourceUpdateKeyword(res, true, "V0", "vendor0"); if (res->ro->vendor_specific->len != 1) return -1; @@ -105,25 +103,23 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) /* Make sure unsupported RO keyword updates are not fatal. */ for (i = 0; i < numUnsupportedCases; ++i) { - if (!virPCIVPDResourceUpdateKeyword(res, true, - unsupportedFieldCases[i].keyword, - unsupportedFieldCases[i].value)) - return -1; + virPCIVPDResourceUpdateKeyword(res, true, + unsupportedFieldCases[i].keyword, + unsupportedFieldCases[i].value); } /* Initialize RW */ res->rw = g_steal_pointer(&rw); - if (!virPCIVPDResourceUpdateKeyword(res, false, "YA", "tag1") - || STRNEQ(res->rw->asset_tag, "tag1")) + virPCIVPDResourceUpdateKeyword(res, false, "YA", "tag1"); + if (STRNEQ(res->rw->asset_tag, "tag1")) return -1; - if (!virPCIVPDResourceUpdateKeyword(res, false, "asset_tag", "tag2") - || STRNEQ(res->rw->asset_tag, "tag2")) + virPCIVPDResourceUpdateKeyword(res, false, "asset_tag", "tag2"); + if (STRNEQ(res->rw->asset_tag, "tag2")) return -1; /* Do a basic system field check. */ - if (!virPCIVPDResourceUpdateKeyword(res, false, "Y0", "system0")) - return -1; + virPCIVPDResourceUpdateKeyword(res, false, "Y0", "system0"); if (res->rw->system_specific->len != 1) return -1; @@ -134,10 +130,12 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED) /* Make sure unsupported RW keyword updates are not fatal. */ for (i = 0; i < numUnsupportedCases; ++i) { - if (!virPCIVPDResourceUpdateKeyword(res, false, - unsupportedFieldCases[i].keyword, - unsupportedFieldCases[i].value)) - return -1; + /* This test is deliberately left in despite + * virPCIVPDResourceUpdateKeyword always succeeding to prevent + * possible regressions if the function is ever rewritten */ + virPCIVPDResourceUpdateKeyword(res, false, + unsupportedFieldCases[i].keyword, + unsupportedFieldCases[i].value); } -- 2.43.0
On a Tuesday in 2024, Peter Krempa wrote:
The function always succeeded and after the removal of programing error checks doesn't even have a 'return false' case. Additionally one of the tests in 'virpcivpdtest' tested that this function never failed on wrong data. Embrace this logic and remove the return value and adjust logging to VIR_DEBUG level to avoid spamming logs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 31 +++++++++++-------------------- src/util/virpcivpd.h | 8 +++++--- tests/virpcivpdtest.c | 38 ++++++++++++++++++-------------------- 3 files changed, 34 insertions(+), 43 deletions(-)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 20545759d5..a6311bfe76 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -134,10 +130,12 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED)
/* Make sure unsupported RW keyword updates are not fatal. */ for (i = 0; i < numUnsupportedCases; ++i) { - if (!virPCIVPDResourceUpdateKeyword(res, false, - unsupportedFieldCases[i].keyword, - unsupportedFieldCases[i].value)) - return -1; + /* This test is deliberately left in despite + * virPCIVPDResourceUpdateKeyword always succeeding to prevent + * possible regressions if the function is ever rewritten */
How is this different from any of the pointless tests you removed earlier? Jano
+ virPCIVPDResourceUpdateKeyword(res, false, + unsupportedFieldCases[i].keyword, + unsupportedFieldCases[i].value); }
-- 2.43.0 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On Wed, Jan 31, 2024 at 09:06:06 +0100, Ján Tomko wrote:
On a Tuesday in 2024, Peter Krempa wrote:
The function always succeeded and after the removal of programing error checks doesn't even have a 'return false' case. Additionally one of the tests in 'virpcivpdtest' tested that this function never failed on wrong data. Embrace this logic and remove the return value and adjust logging to VIR_DEBUG level to avoid spamming logs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 31 +++++++++++-------------------- src/util/virpcivpd.h | 8 +++++--- tests/virpcivpdtest.c | 38 ++++++++++++++++++-------------------- 3 files changed, 34 insertions(+), 43 deletions(-)
diff --git a/tests/virpcivpdtest.c b/tests/virpcivpdtest.c index 20545759d5..a6311bfe76 100644 --- a/tests/virpcivpdtest.c +++ b/tests/virpcivpdtest.c @@ -134,10 +130,12 @@ testPCIVPDResourceBasic(const void *data G_GNUC_UNUSED)
/* Make sure unsupported RW keyword updates are not fatal. */ for (i = 0; i < numUnsupportedCases; ++i) { - if (!virPCIVPDResourceUpdateKeyword(res, false, - unsupportedFieldCases[i].keyword, - unsupportedFieldCases[i].value)) - return -1; + /* This test is deliberately left in despite + * virPCIVPDResourceUpdateKeyword always succeeding to prevent + * possible regressions if the function is ever rewritten */
How is this different from any of the pointless tests you removed earlier?
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.
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@redhat.com> --- 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
- fix passing of 'errno' to 'virReportSystemError' The 'open' syscall returns '-1' and sets 'errno' on failure. The code passed '-fd' as 'errno' rather than errno itself, thus always reporting EPERM. - don't overwrite errors when closing FD Use VIR_AUTOCLOSE to avoid overwriting the errors from virPCIVPDParse. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpci.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 99e6e6cbb1..780b4f9eec 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -3103,28 +3103,21 @@ virPCIDeviceHasVPD(virPCIDevice *dev) virPCIVPDResource * virPCIDeviceGetVPD(virPCIDevice *dev) { - g_autofree char *vpdPath = NULL; - int fd; - g_autoptr(virPCIVPDResource) res = NULL; + g_autofree char *vpdPath = virPCIFile(dev->name, "vpd"); + VIR_AUTOCLOSE fd = -1; - vpdPath = virPCIFile(dev->name, "vpd"); if (!virPCIDeviceHasVPD(dev)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %1$s does not have a VPD"), - virPCIDeviceGetName(dev)); - return NULL; - } - if ((fd = open(vpdPath, O_RDONLY)) < 0) { - virReportSystemError(-fd, _("Failed to open a VPD file '%1$s'"), vpdPath); + virPCIDeviceGetName(dev)); return NULL; } - res = virPCIVPDParse(fd); - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, _("Unable to close the VPD file, fd: %1$d"), fd); + if ((fd = open(vpdPath, O_RDONLY)) < 0) { + virReportSystemError(errno, _("Failed to open a VPD file '%1$s'"), vpdPath); return NULL; } - return g_steal_pointer(&res); + return virPCIVPDParse(fd); } #else -- 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@redhat.com> --- 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
Each caller was checking that the function read as many bytes as it expected. Move the check inside virPCIVPDReadVPDBytes and make it report a proper error rather than just a combination of VIR_DEBUG inside the function and a random VIR_INFO in the caller. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 73 +++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 0021a88f2d..10cabff0b9 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -361,34 +361,40 @@ virPCIVPDResourceUpdateKeyword(virPCIVPDResource *res, * @offset: The offset at which bytes need to be read. * @csum: A pointer to a byte containing the current checksum value. Mutated by this function. * - * Returns: the number of VPD bytes read from the specified file descriptor. The csum value is + * Returns 0 if exactly @count bytes were read from @vpdFileFd. The csum value is * also modified as bytes are read. If an error occurs while reading data from the VPD file - * descriptor, it is reported and -1 is returned to the caller. If EOF is occurred, 0 is returned - * to the caller. + * descriptor, it is reported and -1 is returned to the caller. */ -static size_t -virPCIVPDReadVPDBytes(int vpdFileFd, uint8_t *buf, size_t count, off_t offset, uint8_t *csum) +static int +virPCIVPDReadVPDBytes(int vpdFileFd, + uint8_t *buf, + size_t count, + off_t offset, + uint8_t *csum) { ssize_t numRead = pread(vpdFileFd, buf, count, offset); - if (numRead == -1) { - VIR_DEBUG("Unable to read %zu bytes at offset %zd from fd: %d", - count, (ssize_t)offset, vpdFileFd); - } else if (numRead) { - /* - * Update the checksum for every byte read. Per the PCI(e) specs - * the checksum is correct if the sum of all bytes in VPD from - * VPD address 0 up to and including the VPD-R RV field's first - * data byte is zero. - */ - while (count--) { - *csum += *buf; - buf++; - } + if (numRead != count) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data")); + return -1; + } + + /* + * Update the checksum for every byte read. Per the PCI(e) specs + * the checksum is correct if the sum of all bytes in VPD from + * VPD address 0 up to and including the VPD-R RV field's first + * data byte is zero. + */ + while (count--) { + *csum += *buf; + buf++; } - return numRead; + + return 0; } + /** * virPCIVPDParseVPDLargeResourceFields: * @vpdFileFd: A file descriptor associated with a file containing PCI device VPD. @@ -423,12 +429,9 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re g_autofree char *fieldValue = NULL; /* Keyword resources consist of keywords (2 ASCII bytes per the spec) and 1-byte length. */ - if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) != 3) { - /* Invalid field encountered which means the resource itself is invalid too. Report - * That VPD has invalid format and bail. */ - VIR_INFO("Could not read a resource field header - VPD has invalid format"); + if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) < 0) return false; - } + fieldDataLen = buf[2]; /* Change the position to the field's data portion skipping the keyword and length bytes. */ fieldPos += 3; @@ -474,10 +477,10 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re VIR_INFO("A field data length violates the resource length boundary."); return false; } - if (virPCIVPDReadVPDBytes(vpdFileFd, buf, bytesToRead, fieldPos, csum) != bytesToRead) { - VIR_INFO("Could not parse a resource field data - VPD has invalid format"); + + if (virPCIVPDReadVPDBytes(vpdFileFd, buf, bytesToRead, fieldPos, csum) < 0) return false; - } + /* Advance the position to the first byte of the next field. */ fieldPos += fieldDataLen; @@ -566,10 +569,9 @@ virPCIVPDParseVPDLargeResourceString(int vpdFileFd, uint16_t resPos, /* The resource value is not NULL-terminated so add one more byte. */ g_autofree char *buf = g_malloc0(resDataLen + 1); - if (virPCIVPDReadVPDBytes(vpdFileFd, (uint8_t *)buf, resDataLen, resPos, csum) != resDataLen) { - VIR_INFO("Could not read a part of a resource - VPD has invalid format"); + if (virPCIVPDReadVPDBytes(vpdFileFd, (uint8_t *)buf, resDataLen, resPos, csum) < 0) return false; - } + resValue = g_strdup(g_strstrip(buf)); if (!virPCIVPDResourceIsValidTextValue(resValue)) { VIR_INFO("The string resource has invalid characters in its value"); @@ -610,8 +612,8 @@ virPCIVPDParse(int vpdFileFd) while (resPos <= PCI_VPD_ADDR_MASK) { /* Read the resource data type tag. */ - if (virPCIVPDReadVPDBytes(vpdFileFd, &tag, 1, resPos, &csum) != 1) - break; + if (virPCIVPDReadVPDBytes(vpdFileFd, &tag, 1, resPos, &csum) < 0) + return NULL; /* 0x80 == 0b10000000 - the large resource data type flag. */ if (tag & PCI_VPD_LARGE_RESOURCE_FLAG) { @@ -620,9 +622,10 @@ virPCIVPDParse(int vpdFileFd) * where the end tag should be. */ break; } + /* Read the two length bytes of the large resource record. */ - if (virPCIVPDReadVPDBytes(vpdFileFd, headerBuf, 2, resPos + 1, &csum) != 2) - break; + if (virPCIVPDReadVPDBytes(vpdFileFd, headerBuf, 2, resPos + 1, &csum) < 0) + return NULL; resDataLen = headerBuf[0] + (headerBuf[1] << 8); /* Change the position to the byte following the tag and length bytes. */ -- 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@redhat.com> --- 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
Merge the pre-checks with the 'switch' statement which is operating on the same values to simplify further refactoring. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index ddd79fa8bc..ba05014e40 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -438,23 +438,27 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re fieldKeyword = g_strndup((char *)buf, 2); fieldFormat = virPCIVPDResourceGetFieldValueFormat(fieldKeyword); - /* Handle special cases first */ - if (!readOnly && fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) { - VIR_INFO("Unexpected RV keyword in the read-write section."); - return false; - } else if (readOnly && fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR) { - VIR_INFO("Unexpected RW keyword in the read-only section."); - return false; - } - /* Determine how many bytes to read per field value type. */ switch (fieldFormat) { case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT: - case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR: case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY: bytesToRead = fieldDataLen; break; + + case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR: + if (readOnly) { + VIR_INFO("Unexpected RW keyword in the read-only section."); + return false; + } + + bytesToRead = fieldDataLen; + break; + case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD: + if (!readOnly) { + VIR_INFO("Unexpected RV keyword in the read-write section."); + return false; + } /* Only need one byte to be read and accounted towards * the checksum calculation. */ bytesToRead = 1; -- 2.43.0
The 'fieldFormat' variable is guaranteed to have only the proper enum values by virPCIVPDResourceGetFieldValueFormat. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
*switch Jano On a Tuesday in 2024, Peter Krempa wrote:
The 'fieldFormat' variable is guaranteed to have only the proper enum values by virPCIVPDResourceGetFieldValueFormat.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 3 --- 1 file changed, 3 deletions(-)
Use a 'switch' statement instead of a bunch of if/elseif statements. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 66 +++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 29 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index 25c4c2c5ec..60e520c46f 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -485,36 +485,43 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re /* Advance the position to the first byte of the next field. */ fieldPos += fieldDataLen; - if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT) { - /* Trim whitespace around a retrieved value and set it to be a field's value. Cases - * where unnecessary whitespace was present around a field value have been encountered - * in the wild. - */ - fieldValue = g_strstrip(g_strndup((char *)buf, fieldDataLen)); - if (!virPCIVPDResourceIsValidTextValue(fieldValue)) { - /* Skip fields with invalid values - this is safe assuming field length is - * correctly specified. */ - VIR_DEBUG("A value for field %s contains invalid characters", fieldKeyword); + switch (fieldFormat) { + case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_TEXT: + /* Trim whitespace around a retrieved value and set it to be a field's value. Cases + * where unnecessary whitespace was present around a field value have been encountered + * in the wild. + */ + fieldValue = g_strstrip(g_strndup((char *)buf, fieldDataLen)); + if (!virPCIVPDResourceIsValidTextValue(fieldValue)) { + /* Skip fields with invalid values - this is safe assuming field length is + * correctly specified. */ + VIR_DEBUG("A value for field %s contains invalid characters", fieldKeyword); + continue; + } + break; + + case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_BINARY: + fieldValue = g_malloc(fieldDataLen); + memcpy(fieldValue, buf, fieldDataLen); + break; + + case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR: + /* Skip the read-write space since it is used for indication only. */ + hasRW = true; + goto done; + + case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD: + if (*csum) { + /* All bytes up to and including the checksum byte should add up to 0. */ + VIR_INFO("Checksum validation has failed"); + return false; + } + hasChecksum = true; + goto done; + + case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST: + /* Skip unknown fields */ continue; - } - } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD) { - if (*csum) { - /* All bytes up to and including the checksum byte should add up to 0. */ - VIR_INFO("Checksum validation has failed"); - return false; - } - hasChecksum = true; - break; - } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR) { - /* Skip the read-write space since it is used for indication only. */ - hasRW = true; - break; - } else if (fieldFormat == VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_LAST) { - /* Skip unknown fields */ - continue; - } else { - fieldValue = g_malloc(fieldDataLen); - memcpy(fieldValue, buf, fieldDataLen); } if (readOnly) { @@ -528,6 +535,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re virPCIVPDResourceUpdateKeyword(res, readOnly, fieldKeyword, fieldValue); } + 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); -- 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@redhat.com> --- 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
On a Tuesday in 2024, Peter Krempa wrote:
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
*aggregated Jano
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@redhat.com> --- src/util/virpcivpd.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The code abused 'VIR_INFO' as an attempt at error reporting. Rework the code to return the usual 0/-1 and raise proper errors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpcivpd.c | 67 +++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/src/util/virpcivpd.c b/src/util/virpcivpd.c index be19f7b747..4a440c2aea 100644 --- a/src/util/virpcivpd.c +++ b/src/util/virpcivpd.c @@ -404,12 +404,15 @@ virPCIVPDReadVPDBytes(int vpdFileFd, * @csum: A pointer to a 1-byte checksum. * @res: A pointer to virPCIVPDResource. * - * 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 if the field was parsed sucessfully; -1 on error */ -static bool -virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t resDataLen, - bool readOnly, uint8_t *csum, virPCIVPDResource *res) +static int +virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, + uint16_t resPos, + uint16_t resDataLen, + bool readOnly, + uint8_t *csum, + virPCIVPDResource *res) { /* A buffer of up to one resource record field size (plus a zero byte) is needed. */ g_autofree uint8_t *buf = g_malloc0(PCI_VPD_MAX_FIELD_SIZE + 1); @@ -427,7 +430,7 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re /* Keyword resources consist of keywords (2 ASCII bytes per the spec) and 1-byte length. */ if (virPCIVPDReadVPDBytes(vpdFileFd, buf, 3, fieldPos, csum) < 0) - return false; + return -1; fieldDataLen = buf[2]; /* Change the position to the field's data portion skipping the keyword and length bytes. */ @@ -444,8 +447,9 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RDWR: if (readOnly) { - VIR_INFO("Unexpected RW keyword in the read-only section."); - return false; + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: unexpected RW keyword in read-only section")); + return -1; } bytesToRead = fieldDataLen; @@ -453,8 +457,9 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD: if (!readOnly) { - VIR_INFO("Unexpected RV keyword in the read-write section."); - return false; + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: unexpected RV keyword in read-write section")); + return -1; } /* Only need one byte to be read and accounted towards * the checksum calculation. */ @@ -472,12 +477,13 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re if (resPos + resDataLen < fieldPos + fieldDataLen) { /* In this case the field cannot simply be skipped since the position of the * next field is determined based on the length of a previous field. */ - VIR_INFO("A field data length violates the resource length boundary."); - return false; + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: data field length invalid")); + return -1; } if (virPCIVPDReadVPDBytes(vpdFileFd, buf, bytesToRead, fieldPos, csum) < 0) - return false; + return -1; /* Advance the position to the first byte of the next field. */ fieldPos += fieldDataLen; @@ -492,7 +498,6 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re if (!virPCIVPDResourceIsValidTextValue(fieldValue)) { /* Skip fields with invalid values - this is safe assuming field length is * correctly specified. */ - VIR_DEBUG("A value for field %s contains invalid characters", fieldKeyword); continue; } break; @@ -512,8 +517,9 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re case VIR_PCI_VPD_RESOURCE_FIELD_VALUE_FORMAT_RESVD: if (*csum) { /* All bytes up to and including the checksum byte should add up to 0. */ - VIR_INFO("Checksum validation has failed"); - return false; + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: invalid checksum")); + return -1; } hasChecksum = true; goto done; @@ -540,16 +546,18 @@ virPCIVPDParseVPDLargeResourceFields(int vpdFileFd, uint16_t resPos, uint16_t re * they were not the last fields in the section. */ if ((fieldPos < resPos + resDataLen)) { /* unparsed data still present */ - VIR_DEBUG("PCI VPD data parsing failed"); - return false; + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: parsing ended prematurely")); + return -1; } if (readOnly && !hasChecksum) { - VIR_DEBUG("VPD-R does not contain the mandatory checksum"); - return false; + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to read the PCI VPD data: missing mandatory checksum")); + return -1; } - return true; + return 0; } @@ -606,7 +614,6 @@ virPCIVPDParse(int vpdFileFd) uint8_t csum = 0; uint8_t headerBuf[2]; - bool isWellFormed = false; uint16_t resPos = 0, resDataLen; uint8_t tag = 0; bool endResReached = false, hasReadOnly = false; @@ -659,20 +666,21 @@ virPCIVPDParse(int vpdFileFd) &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: - isWellFormed = virPCIVPDParseVPDLargeResourceFields(vpdFileFd, resPos, - resDataLen, true, &csum, res); + if (virPCIVPDParseVPDLargeResourceFields(vpdFileFd, resPos, + resDataLen, true, &csum, res) < 0) + return NULL; /* Encountered the VPD-R tag. The resource record parsing also validates * the presence of the required checksum in the RV field. */ hasReadOnly = true; break; /* Large resource type which is also a VPD-W: 0x80 | 0x11 == 0x91 */ case PCI_VPD_LARGE_RESOURCE_FLAG | PCI_VPD_READ_WRITE_LARGE_RESOURCE_FLAG: - isWellFormed = virPCIVPDParseVPDLargeResourceFields(vpdFileFd, resPos, resDataLen, - false, &csum, res); + if (virPCIVPDParseVPDLargeResourceFields(vpdFileFd, resPos, resDataLen, + false, &csum, res) < 0) + return NULL; break; default: /* While we cannot parse unknown resource types, they can still be skipped @@ -682,11 +690,6 @@ virPCIVPDParse(int vpdFileFd) continue; } - if (!isWellFormed) { - VIR_DEBUG("Encountered an invalid VPD"); - return NULL; - } - /* Continue processing other resource records. */ resPos += resDataLen; } -- 2.43.0
Remove the wannabe error reporting via 'VIR_DEBUG/VIR_INFO' in favor of proper errors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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
s/reporting/logging/ The error is reset in patch 23/31 so nothing gets 'raised' or 'reported' anyway. Jano On a Tuesday in 2024, Peter Krempa wrote:
Remove the wannabe error reporting via 'VIR_DEBUG/VIR_INFO' in favor of proper errors.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On a Tuesday in 2024, Peter Krempa wrote:
The first part of the series fixes the XML formatter of nodedevs to not generate invalid XML if a PCI device custom field contains '>' which would be printed unescaped
The rest fixes questionable and broken error reporting from the pci vpd device code which actually parses the above data.
https://gitlab.com/pipo.sk/libvirt/-/pipelines/1157912774
Peter Krempa (31): virPCIVPDResourceIsValidTextValue: Adjust comment to reflect actual code util: pcivpd: Refactor virPCIVPDResourceIsValidTextValue virNodeDeviceCapVPDFormatCustom*: Escape unsanitized strings virNodeDeviceCapVPDFormat: Properly escape system-originated strings schema: nodedev: Adjust allowed characters in 'vpdFieldValueFormat' tests: Test the previously mishandled PCI VPD characters Don't overwrite error message from 'virXPathNodeSet' tests: virpcivpdtest: Remove 'testVirPCIVPDReadVPDBytes' case util: virpcivpd: Unexport 'virPCIVPDReadVPDBytes' util: pcivpd: Unexport virPCIVPDParseVPDLargeResourceFields tests: virpcivpd: Remove 'testVirPCIVPDParseVPDStringResource' case util: virpcivpd: Unexport 'virPCIVPDParseVPDLargeResourceString' virPCIVPDResourceGetKeywordPrefix: Fix logging util: virpcivpd: Remove return value from virPCIVPDResourceCustomUpsertValue conf: virNodeDeviceCapVPDParse*: Remove pointless NULL checks virpcivpdtest: testPCIVPDResourceBasic: Remove tests for uninitialized 'ro'/'rw' section util: virPCIVPDResourceUpdateKeyword: Remove impossible checks conf: node_device: Refactor 'virNodeDeviceCapVPDParseCustomFields' to fix error reporting virNodeDeviceCapVPDParseXML: Fix error reporting util: virpcivpd: Remove return value from virPCIVPDResourceUpdateKeyword virPCIDeviceHasVPD: Refactor "debug" messages virPCIDeviceGetVPD: Fix multiple error handling bugs virPCIDeviceGetVPD: Handle errors in callers virPCIVPDReadVPDBytes: Refactor error handling virPCIVPDParseVPDLargeResourceString: Properly report errors virPCIVPDParseVPDLargeResourceFields: Merge logic conditions virPCIVPDParseVPDLargeResourceFields: Remove impossible 'default' swithch case virPCIVPDParseVPDLargeResourceFields: Refactor processing of read data virPCIVPDParseVPDLargeResourceFields: Refactor return logic virPCIVPDParseVPDLargeResourceFields: Report proper errors virPCIVPDParse: Do reasonable error reporting
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa