[PATCH 00/11] conf: Refactor virSysinfoParseXML and remove pointless XML node validation
Peter Krempa (11): conf: domain: Refactor cleanup in virSysinfoBIOSParseXML conf: domain: Remove pointless XML node name validation in virSysinfoBIOSParseXML conf: domain: Reformat XPath queries in virSysinfoSystemParseXML conf: domain: Refactor cleanup in virSysinfoSystemParseXML conf: domain: Remove pointless XML node name validation in virSysinfoSystemParseXML conf: domain: Reformat XPath queries in virSysinfoChassisParseXML conf: domain: Refactor cleanup in virSysinfoChassisParseXML conf: domain: Remove pointless XML node name validation in virSysinfoChassisParseXML conf: domain: Parse 'type' attribute via virXMLPropEnum in virSysinfoParseXML conf: domain: Refactor cleanup in virSysinfoParseXML conf: domain: Remove pointless XML node name validation in virSysinfoParseXML src/conf/domain_conf.c | 139 ++++++++++------------------------------- src/util/virsysinfo.h | 3 + 2 files changed, 36 insertions(+), 106 deletions(-) -- 2.37.3
Register automatic cleanup for virSysinfoBIOSDef and use it to refactor the cleanup code paths in virSysinfoBIOSParseXML. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 19 ++++++------------- src/util/virsysinfo.h | 1 + 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7984a15c46..15db12876e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12136,19 +12136,16 @@ virSysinfoBIOSParseXML(xmlNodePtr node, virSysinfoBIOSDef **bios) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - int ret = -1; - virSysinfoBIOSDef *def; + g_autoptr(virSysinfoBIOSDef) def = g_new0(virSysinfoBIOSDef, 1); ctxt->node = node; if (!virXMLNodeNameEqual(node, "bios")) { virReportError(VIR_ERR_XML_ERROR, "%s", _("XML does not contain expected 'bios' element")); - return ret; + return -1; } - def = g_new0(virSysinfoBIOSDef, 1); - def->vendor = virXPathString("string(entry[@name='vendor'])", ctxt); def->version = virXPathString("string(entry[@name='version'])", ctxt); def->date = virXPathString("string(entry[@name='date'])", ctxt); @@ -12173,20 +12170,16 @@ virSysinfoBIOSParseXML(xmlNodePtr node, (year < 0 || (year >= 100 && year < 1900))) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("Invalid BIOS 'date' format")); - goto cleanup; + return -1; } } if (!def->vendor && !def->version && - !def->date && !def->release) { - g_clear_pointer(&def, virSysinfoBIOSDefFree); - } + !def->date && !def->release) + return 0; *bios = g_steal_pointer(&def); - ret = 0; - cleanup: - virSysinfoBIOSDefFree(def); - return ret; + return 0; } static int diff --git a/src/util/virsysinfo.h b/src/util/virsysinfo.h index 97e0e18ddf..899193dc81 100644 --- a/src/util/virsysinfo.h +++ b/src/util/virsysinfo.h @@ -142,6 +142,7 @@ struct _virSysinfoDef { virSysinfoDef *virSysinfoRead(void); void virSysinfoBIOSDefFree(virSysinfoBIOSDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSysinfoBIOSDef, virSysinfoBIOSDefFree); void virSysinfoSystemDefFree(virSysinfoSystemDef *def); void virSysinfoBaseBoardDefClear(virSysinfoBaseBoardDef *def); void virSysinfoChassisDefFree(virSysinfoChassisDef *def); -- 2.37.3
The only caller passes 'node' argument originating from an XPath lookup for the 'bios' element, so there's no point in checking it once more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 15db12876e..eab2e98792 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12140,12 +12140,6 @@ virSysinfoBIOSParseXML(xmlNodePtr node, ctxt->node = node; - if (!virXMLNodeNameEqual(node, "bios")) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("XML does not contain expected 'bios' element")); - return -1; - } - def->vendor = virXPathString("string(entry[@name='vendor'])", ctxt); def->version = virXPathString("string(entry[@name='version'])", ctxt); def->date = virXPathString("string(entry[@name='date'])", ctxt); -- 2.37.3
Remove the unneeded linebreaks after assignment operator. Only one line exceeds 80 colums and just by 4 characters. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index eab2e98792..b0a978cc62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12198,14 +12198,10 @@ virSysinfoSystemParseXML(xmlNodePtr node, def = g_new0(virSysinfoSystemDef, 1); - def->manufacturer = - virXPathString("string(entry[@name='manufacturer'])", ctxt); - def->product = - virXPathString("string(entry[@name='product'])", ctxt); - def->version = - virXPathString("string(entry[@name='version'])", ctxt); - def->serial = - virXPathString("string(entry[@name='serial'])", ctxt); + def->manufacturer = virXPathString("string(entry[@name='manufacturer'])", ctxt); + def->product = virXPathString("string(entry[@name='product'])", ctxt); + def->version = virXPathString("string(entry[@name='version'])", ctxt); + def->serial = virXPathString("string(entry[@name='serial'])", ctxt); tmpUUID = virXPathString("string(entry[@name='uuid'])", ctxt); if (tmpUUID) { unsigned char uuidbuf[VIR_UUID_BUFLEN]; @@ -12232,10 +12228,8 @@ virSysinfoSystemParseXML(xmlNodePtr node, virUUIDFormat(uuidbuf, uuidstr); def->uuid = g_strdup(uuidstr); } - def->sku = - virXPathString("string(entry[@name='sku'])", ctxt); - def->family = - virXPathString("string(entry[@name='family'])", ctxt); + def->sku = virXPathString("string(entry[@name='sku'])", ctxt); + def->family = virXPathString("string(entry[@name='family'])", ctxt); if (!def->manufacturer && !def->product && !def->version && !def->serial && !def->uuid && !def->sku && !def->family) { -- 2.37.3
Register automatic cleanup for virSysinfoSystemDef and use it to refactor the cleanup code paths in virSysinfoSystemParseXML. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 24 ++++++++---------------- src/util/virsysinfo.h | 1 + 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b0a978cc62..8e9f415070 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12184,8 +12184,7 @@ virSysinfoSystemParseXML(xmlNodePtr node, bool uuid_generated) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - int ret = -1; - virSysinfoSystemDef *def; + g_autoptr(virSysinfoSystemDef) def = g_new0(virSysinfoSystemDef, 1); g_autofree char *tmpUUID = NULL; ctxt->node = node; @@ -12193,11 +12192,9 @@ virSysinfoSystemParseXML(xmlNodePtr node, if (!virXMLNodeNameEqual(node, "system")) { virReportError(VIR_ERR_XML_ERROR, "%s", _("XML does not contain expected 'system' element")); - return ret; + return -1; } - def = g_new0(virSysinfoSystemDef, 1); - def->manufacturer = virXPathString("string(entry[@name='manufacturer'])", ctxt); def->product = virXPathString("string(entry[@name='product'])", ctxt); def->version = virXPathString("string(entry[@name='version'])", ctxt); @@ -12209,15 +12206,14 @@ virSysinfoSystemParseXML(xmlNodePtr node, if (virUUIDParse(tmpUUID, uuidbuf) < 0) { virReportError(VIR_ERR_XML_DETAIL, "%s", _("malformed <sysinfo> uuid element")); - goto cleanup; + return -1; } if (uuid_generated) { memcpy(domUUID, uuidbuf, VIR_UUID_BUFLEN); } else if (memcmp(domUUID, uuidbuf, VIR_UUID_BUFLEN) != 0) { virReportError(VIR_ERR_XML_DETAIL, "%s", - _("UUID mismatch between <uuid> and " - "<sysinfo>")); - goto cleanup; + _("UUID mismatch between <uuid> and <sysinfo>")); + return -1; } /* Although we've validated the UUID as good, virUUIDParse() is * lax with respect to allowing extraneous "-" and " ", but the @@ -12232,15 +12228,11 @@ virSysinfoSystemParseXML(xmlNodePtr node, def->family = virXPathString("string(entry[@name='family'])", ctxt); if (!def->manufacturer && !def->product && !def->version && - !def->serial && !def->uuid && !def->sku && !def->family) { - g_clear_pointer(&def, virSysinfoSystemDefFree); - } + !def->serial && !def->uuid && !def->sku && !def->family) + return 0; *sysdef = g_steal_pointer(&def); - ret = 0; - cleanup: - virSysinfoSystemDefFree(def); - return ret; + return 0; } static int diff --git a/src/util/virsysinfo.h b/src/util/virsysinfo.h index 899193dc81..d9f15b06e2 100644 --- a/src/util/virsysinfo.h +++ b/src/util/virsysinfo.h @@ -144,6 +144,7 @@ virSysinfoDef *virSysinfoRead(void); void virSysinfoBIOSDefFree(virSysinfoBIOSDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSysinfoBIOSDef, virSysinfoBIOSDefFree); void virSysinfoSystemDefFree(virSysinfoSystemDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSysinfoSystemDef, virSysinfoSystemDefFree); void virSysinfoBaseBoardDefClear(virSysinfoBaseBoardDef *def); void virSysinfoChassisDefFree(virSysinfoChassisDef *def); void virSysinfoOEMStringsDefFree(virSysinfoOEMStringsDef *def); -- 2.37.3
The only caller passes 'node' argument originating from an XPath lookup for the 'system' element, so there's no point in checking it once more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8e9f415070..d9b77eddb4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12189,12 +12189,6 @@ virSysinfoSystemParseXML(xmlNodePtr node, ctxt->node = node; - if (!virXMLNodeNameEqual(node, "system")) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("XML does not contain expected 'system' element")); - return -1; - } - def->manufacturer = virXPathString("string(entry[@name='manufacturer'])", ctxt); def->product = virXPathString("string(entry[@name='product'])", ctxt); def->version = virXPathString("string(entry[@name='version'])", ctxt); -- 2.37.3
Remove the unneeded linebreaks after assignment operator. Only one line exceeds 80 colums and just by 4 characters. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d9b77eddb4..90478f2528 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12336,16 +12336,11 @@ virSysinfoChassisParseXML(xmlNodePtr node, def = g_new0(virSysinfoChassisDef, 1); - def->manufacturer = - virXPathString("string(entry[@name='manufacturer'])", ctxt); - def->version = - virXPathString("string(entry[@name='version'])", ctxt); - def->serial = - virXPathString("string(entry[@name='serial'])", ctxt); - def->asset = - virXPathString("string(entry[@name='asset'])", ctxt); - def->sku = - virXPathString("string(entry[@name='sku'])", ctxt); + def->manufacturer = virXPathString("string(entry[@name='manufacturer'])", ctxt); + def->version = virXPathString("string(entry[@name='version'])", ctxt); + def->serial = virXPathString("string(entry[@name='serial'])", ctxt); + def->asset = virXPathString("string(entry[@name='asset'])", ctxt); + def->sku = virXPathString("string(entry[@name='sku'])", ctxt); if (!def->manufacturer && !def->version && !def->serial && !def->asset && !def->sku) { -- 2.37.3
Register automatic cleanup for virSysinfoChassisDef and use it to refactor the cleanup code paths in virSysinfoChassisParseXML. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 16 +++++----------- src/util/virsysinfo.h | 1 + 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 90478f2528..e10b4d72f8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12323,19 +12323,16 @@ virSysinfoChassisParseXML(xmlNodePtr node, virSysinfoChassisDef **chassisdef) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - int ret = -1; - virSysinfoChassisDef *def; + g_autoptr(virSysinfoChassisDef) def = g_new0(virSysinfoChassisDef, 1); ctxt->node = node; if (!xmlStrEqual(node->name, BAD_CAST "chassis")) { virReportError(VIR_ERR_XML_ERROR, "%s", _("XML does not contain expected 'chassis' element")); - return ret; + return -1; } - def = g_new0(virSysinfoChassisDef, 1); - def->manufacturer = virXPathString("string(entry[@name='manufacturer'])", ctxt); def->version = virXPathString("string(entry[@name='version'])", ctxt); def->serial = virXPathString("string(entry[@name='serial'])", ctxt); @@ -12343,14 +12340,11 @@ virSysinfoChassisParseXML(xmlNodePtr node, def->sku = virXPathString("string(entry[@name='sku'])", ctxt); if (!def->manufacturer && !def->version && - !def->serial && !def->asset && !def->sku) { - g_clear_pointer(&def, virSysinfoChassisDefFree); - } + !def->serial && !def->asset && !def->sku) + return 0; *chassisdef = g_steal_pointer(&def); - ret = 0; - virSysinfoChassisDefFree(def); - return ret; + return 0; } diff --git a/src/util/virsysinfo.h b/src/util/virsysinfo.h index d9f15b06e2..5fa91d9611 100644 --- a/src/util/virsysinfo.h +++ b/src/util/virsysinfo.h @@ -147,6 +147,7 @@ void virSysinfoSystemDefFree(virSysinfoSystemDef *def); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSysinfoSystemDef, virSysinfoSystemDefFree); void virSysinfoBaseBoardDefClear(virSysinfoBaseBoardDef *def); void virSysinfoChassisDefFree(virSysinfoChassisDef *def); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSysinfoChassisDef, virSysinfoChassisDefFree); void virSysinfoOEMStringsDefFree(virSysinfoOEMStringsDef *def); void virSysinfoDefFree(virSysinfoDef *def); -- 2.37.3
The only caller passes 'node' argument originating from an XPath lookup for the 'chassis' element, so there's no point in checking it once more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e10b4d72f8..6379cfda7c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12327,12 +12327,6 @@ virSysinfoChassisParseXML(xmlNodePtr node, ctxt->node = node; - if (!xmlStrEqual(node->name, BAD_CAST "chassis")) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("XML does not contain expected 'chassis' element")); - return -1; - } - def->manufacturer = virXPathString("string(entry[@name='manufacturer'])", ctxt); def->version = virXPathString("string(entry[@name='version'])", ctxt); def->serial = virXPathString("string(entry[@name='serial'])", ctxt); -- 2.37.3
Rewrite the code to use the simple helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6379cfda7c..b9494500e2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12451,8 +12451,6 @@ virSysinfoParseXML(xmlNodePtr node, { VIR_XPATH_NODE_AUTORESTORE(ctxt) virSysinfoDef *def; - g_autofree char *typeStr = NULL; - int type; ctxt->node = node; @@ -12464,18 +12462,9 @@ virSysinfoParseXML(xmlNodePtr node, def = g_new0(virSysinfoDef, 1); - typeStr = virXMLPropString(node, "type"); - if (typeStr == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("sysinfo must contain a type attribute")); - goto error; - } - if ((type = virSysinfoTypeFromString(typeStr)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown sysinfo type '%s'"), typeStr); + if (virXMLPropEnum(node, "type", virSysinfoTypeFromString, + VIR_XML_PROP_REQUIRED, &def->type) < 0) goto error; - } - def->type = type; switch (def->type) { case VIR_SYSINFO_SMBIOS: -- 2.37.3
Use automatic pointer freeing to remove the 'error' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b9494500e2..71997e586a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12450,7 +12450,7 @@ virSysinfoParseXML(xmlNodePtr node, bool uuid_generated) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - virSysinfoDef *def; + g_autoptr(virSysinfoDef) def = g_new0(virSysinfoDef, 1); ctxt->node = node; @@ -12460,32 +12460,26 @@ virSysinfoParseXML(xmlNodePtr node, return NULL; } - def = g_new0(virSysinfoDef, 1); - if (virXMLPropEnum(node, "type", virSysinfoTypeFromString, VIR_XML_PROP_REQUIRED, &def->type) < 0) - goto error; + return NULL; switch (def->type) { case VIR_SYSINFO_SMBIOS: if (virSysinfoParseSMBIOSDef(def, ctxt, domUUID, uuid_generated) < 0) - goto error; + return NULL; break; case VIR_SYSINFO_FWCFG: if (virSysinfoParseFWCfgDef(def, node, ctxt) < 0) - goto error; + return NULL; break; case VIR_SYSINFO_LAST: break; } - return def; - - error: - virSysinfoDefFree(def); - return NULL; + return g_steal_pointer(&def); } unsigned int -- 2.37.3
The only caller passes 'node' argument originating from an XPath lookup for the 'sysinfo' element, so there's no point in checking it once more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 71997e586a..65f8ad20c9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12454,12 +12454,6 @@ virSysinfoParseXML(xmlNodePtr node, ctxt->node = node; - if (!virXMLNodeNameEqual(node, "sysinfo")) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("XML does not contain expected 'sysinfo' element")); - return NULL; - } - if (virXMLPropEnum(node, "type", virSysinfoTypeFromString, VIR_XML_PROP_REQUIRED, &def->type) < 0) return NULL; -- 2.37.3
On a Tuesday in 2022, Peter Krempa wrote:
Peter Krempa (11): conf: domain: Refactor cleanup in virSysinfoBIOSParseXML conf: domain: Remove pointless XML node name validation in virSysinfoBIOSParseXML conf: domain: Reformat XPath queries in virSysinfoSystemParseXML conf: domain: Refactor cleanup in virSysinfoSystemParseXML conf: domain: Remove pointless XML node name validation in virSysinfoSystemParseXML conf: domain: Reformat XPath queries in virSysinfoChassisParseXML conf: domain: Refactor cleanup in virSysinfoChassisParseXML conf: domain: Remove pointless XML node name validation in virSysinfoChassisParseXML conf: domain: Parse 'type' attribute via virXMLPropEnum in virSysinfoParseXML conf: domain: Refactor cleanup in virSysinfoParseXML conf: domain: Remove pointless XML node name validation in virSysinfoParseXML
src/conf/domain_conf.c | 139 ++++++++++------------------------------- src/util/virsysinfo.h | 3 + 2 files changed, 36 insertions(+), 106 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa