11:07 a.m.
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
11:07 a.m.
New subject: [PATCH 01/31] virPCIVPDResourceIsValidTextValue: Adjust comment to reflect actual code
The function does not reject '&', '<', '>' contrary to
what it actually
states. Move and adjust the comment.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:07 a.m.
New subject: [PATCH 02/31] util: pcivpd: Refactor virPCIVPDResourceIsValidTextValue
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(a)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
11:07 a.m.
New subject: [PATCH 03/31] virNodeDeviceCapVPDFormatCustom*: Escape unsanitized strings
The custom field data is taken from PCI device data which can contain
any printable characters, and thus must be escaped when putting into
XML.
Originally, based on the comment and XML schema which was fixed in
previous commits the idea seemed to be that the parser would validate
that only characters which don't break the XML would be present but that
didn't seem to materialize.
Switch to proper escaping of the XML.
Fixes: 3954378d06a
Resolves: https://issues.redhat.com/browse/RHEL-22314
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:07 a.m.
New subject: [PATCH 04/31] virNodeDeviceCapVPDFormat: Properly escape system-originated strings
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(a)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
11:07 a.m.
New subject: [PATCH 05/31] schema: nodedev: Adjust allowed characters in 'vpdFieldValueFormat'
The check in 'virPCIVPDResourceIsValidTextValue' allows any printable
characters, thus the XML schema should do the same.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:07 a.m.
New subject: [PATCH 06/31] tests: Test the previously mishandled PCI VPD characters
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(a)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'>!(a)#$./&gt;&lt;</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
11:07 a.m.
New subject: [PATCH 07/31] Don't overwrite error message from 'virXPathNodeSet'
'virXPathNodeSet' returns -1 only when 'ctxt' or 'xpath' are NULL
or
when the 'xpath' string is invalid. Both are programmign errors. It
doesn't make sense for the code to overwrite the error message for
anything supposedly more relevant.
The majority of calls to 'virXPathNodeSet' already didn't do this, so
this patch fixes the rest to prevent it from spreading again.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
1:55 a.m.
New subject: [PATCH 07/31] Don't overwrite error message from
'virXPathNodeSet'
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(a)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(a)redhat.com>
Jano
11:07 a.m.
New subject: [PATCH 08/31] tests: virpcivpdtest: Remove 'testVirPCIVPDReadVPDBytes' case
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(a)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
11:07 a.m.
New subject: [PATCH 09/31] util: virpcivpd: Unexport 'virPCIVPDReadVPDBytes'
The function is no longer used outside of virpcivpd.c
Signed-off-by: Peter Krempa <pkrempa(a)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
11:07 a.m.
New subject: [PATCH 10/31] util: pcivpd: Unexport virPCIVPDParseVPDLargeResourceFields
The function is not used in other files.
Signed-off-by: Peter Krempa <pkrempa(a)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
11:07 a.m.
New subject: [PATCH 11/31] tests: virpcivpd: Remove 'testVirPCIVPDParseVPDStringResource' case
The test case excercises 'virPCIVPDParseVPDLargeResourceString' which is
also tested by other cases which parse the whole VPD block. Remove the
specific test case as it's not adding any additional value.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:07 a.m.
New subject: [PATCH 12/31] util: virpcivpd: Unexport 'virPCIVPDParseVPDLargeResourceString'
Signed-off-by: Peter Krempa <pkrempa(a)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
11:07 a.m.
New subject: [PATCH 13/31] virPCIVPDResourceGetKeywordPrefix: Fix logging
Use VIR_DEBUG instead of VIR_INFO as that's more appropriate and report
relevant information for debugging.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:07 a.m.
New subject: [PATCH 14/31] util: virpcivpd: Remove return value from virPCIVPDResourceCustomUpsertValue
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(a)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
11:07 a.m.
New subject: [PATCH 15/31] conf: virNodeDeviceCapVPDParse*: Remove pointless NULL checks
The function are never called with NULL argument so the checks can be
removed.
Signed-off-by: Peter Krempa <pkrempa(a)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
11:07 a.m.
New subject: [PATCH 16/31] virpcivpdtest: testPCIVPDResourceBasic: Remove tests for uninitialized 'ro'/'rw' section
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(a)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
11:07 a.m.
New subject: [PATCH 17/31] util: virPCIVPDResourceUpdateKeyword: Remove impossible checks
All callers satisfy these callers as they are just for programming
errors.
Signed-off-by: Peter Krempa <pkrempa(a)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
1:59 a.m.
New subject: [PATCH 17/31] util: virPCIVPDResourceUpdateKeyword: Remove
impossible checks
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(a)redhat.com>
---
src/util/virpcivpd.c | 19 -------------------
1 file changed, 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:07 a.m.
New subject: [PATCH 18/31] conf: node_device: Refactor 'virNodeDeviceCapVPDParseCustomFields' to fix error reporting
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(a)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
11:07 a.m.
New subject: [PATCH 19/31] virNodeDeviceCapVPDParseXML: Fix error reporting
Don't overwrite already reported errors and improve parsing of
attributes.
Signed-off-by: Peter Krempa <pkrempa(a)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
11:07 a.m.
New subject: [PATCH 20/31] util: virpcivpd: Remove return value from virPCIVPDResourceUpdateKeyword
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(a)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
2:06 a.m.
New subject: [PATCH 20/31] util: virpcivpd: Remove return value from
virPCIVPDResourceUpdateKeyword
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(a)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(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
2:27 a.m.
New subject: [PATCH 20/31] util: virpcivpd: Remove return value from
virPCIVPDResourceUpdateKeyword
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(a)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.
11:07 a.m.
New subject: [PATCH 21/31] virPCIDeviceHasVPD: Refactor "debug" messages
A checker function should not raise VIR_INFO or VIR_WARN messages
especially if they contain information useful only for debugging.
Turn the message into a VIR_DEBUG with universal meaning.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:08 a.m.
New subject: [PATCH 22/31] virPCIDeviceGetVPD: Fix multiple error handling bugs
- 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(a)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
11:08 a.m.
New subject: [PATCH 23/31] virPCIDeviceGetVPD: Handle errors in callers
Until now 'virPCIDeviceGetVPD' couldn't reallistically raise an error,
but that will change. Handle the errors by either resetting it if we'd
be ignoring it or forward it.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:08 a.m.
New subject: [PATCH 24/31] virPCIVPDReadVPDBytes: Refactor error handling
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(a)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
11:08 a.m.
New subject: [PATCH 25/31] virPCIVPDParseVPDLargeResourceString: Properly report errors
Replace VIR_INFO being used as form of error reporting with proper
virReportError and the usual return values.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
11:08 a.m.
New subject: [PATCH 26/31] virPCIVPDParseVPDLargeResourceFields: Merge logic conditions
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(a)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
11:08 a.m.
New subject: [PATCH 27/31] virPCIVPDParseVPDLargeResourceFields: Remove impossible 'default' swithch case
The 'fieldFormat' variable is guaranteed to have only the proper enum
values by virPCIVPDResourceGetFieldValueFormat.
Signed-off-by: Peter Krempa <pkrempa(a)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
2:21 a.m.
New subject: [PATCH 27/31] virPCIVPDParseVPDLargeResourceFields: Remove
impossible 'default' swithch case
*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(a)redhat.com>
---
src/util/virpcivpd.c | 3 ---
1 file changed, 3 deletions(-)
11:08 a.m.
New subject: [PATCH 28/31] virPCIVPDParseVPDLargeResourceFields: Refactor processing of read data
Use a 'switch' statement instead of a bunch of if/elseif statements.
Signed-off-by: Peter Krempa <pkrempa(a)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
11:08 a.m.
New subject: [PATCH 29/31] virPCIVPDParseVPDLargeResourceFields: Refactor return logic
Rewrite the conditions after exiting the parser so that they are easier
to understand. This partially decreases the granularity of "error"
messages as they are not strictly necessary albeit for debugging.
As it was already observed in this code the logic itself often does
something else than the comment claims, thus the code logic is
preserved.
Changes:
- any case when not all data was processed is agregated together and
gets a common "error" message
- absence of 'checksum' field is checked separately
- helper variables are removed as they are no longer needed
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
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
2:24 a.m.
New subject: [PATCH 29/31] virPCIVPDParseVPDLargeResourceFields: Refactor
return logic
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(a)redhat.com>
---
src/util/virpcivpd.c | 32 +++++++++++++-------------------
1 file changed, 13 insertions(+), 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
11:08 a.m.
New subject: [PATCH 30/31] virPCIVPDParseVPDLargeResourceFields: Report proper errors
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(a)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
11:08 a.m.
New subject: [PATCH 31/31] virPCIVPDParse: Do reasonable error reporting
Remove the wannabe error reporting via 'VIR_DEBUG/VIR_INFO' in favor of
proper errors.
Signed-off-by: Peter Krempa <pkrempa(a)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
3:26 a.m.
New subject: [PATCH 31/31] virPCIVPDParse: Do reasonable error reporting
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(a)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(a)lists.libvirt.org
To unsubscribe send an email to devel-leave(a)lists.libvirt.org
3:26 a.m.
New subject: [PATCH 00/31] pci vpd: Fix broken XML formatter and refactor
questionable error reporting
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(a)redhat.com>
Jano
295
days inactive
296
days old
39 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa