[libvirt PATCH 00/10] Refactor more XML parsing boilerplate code, part IX
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html Tim Wiederhake (10): virDomainObjParseXML: Use virXMLProp* virDomainObjParseXML: Use g_autoptr virNetworkDHCPLeaseTimeDef: Make expiry unsigned long long virNetworkDHCPLeaseTimeDefParseXML: Use virXMLProp* virPCIEDeviceInfoLinkParseXML: Use virXMLProp* virPCIEDeviceInfoLinkParseXML: Remove unused parameter ctxt virNodeDevCapsDefParseXML: Use virXMLProp* conf: node_device: Register autoptr cleanup function for virNodeDevCapsDef virNodeDevCapsDefParseXML: Use g_autoptr virNodeDeviceDefParseXML: Use virXMLProp* src/conf/domain_conf.c | 64 ++++++++++--------------- src/conf/network_conf.c | 32 +++++-------- src/conf/network_conf.h | 2 +- src/conf/node_device_conf.c | 94 ++++++++----------------------------- src/conf/node_device_conf.h | 2 + src/network/bridge_driver.c | 2 +- 6 files changed, 62 insertions(+), 134 deletions(-) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e8632e4d73..0d396cbdda 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20354,7 +20354,7 @@ virDomainObjParseXML(xmlDocPtr xml, virDomainObj *obj; size_t i; int n; - int state; + virDomainState state; int reason = 0; void *parseOpaque = NULL; g_autofree char *tmp = NULL; @@ -20377,17 +20377,9 @@ virDomainObjParseXML(xmlDocPtr xml, if (!obj->def) goto error; - if (!(tmp = virXMLPropString(ctxt->node, "state"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing domain state")); + if (virXMLPropEnum(ctxt->node, "state", virDomainStateTypeFromString, + VIR_XML_PROP_REQUIRED, &state) < 0) goto error; - } - if ((state = virDomainStateTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid domain state '%s'"), tmp); - goto error; - } - VIR_FREE(tmp); if ((tmp = virXMLPropString(ctxt->node, "reason"))) { if ((reason = virDomainStateReasonFromString(state, tmp)) < 0) { @@ -20409,18 +20401,16 @@ virDomainObjParseXML(xmlDocPtr xml, if ((n = virXPathNodeSet("./taint", ctxt, &taintNodes)) < 0) goto error; for (i = 0; i < n; i++) { - char *str = virXMLPropString(taintNodes[i], "flag"); - if (str) { - int flag = virDomainTaintTypeFromString(str); - if (flag < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown taint flag %s"), str); - VIR_FREE(str); - goto error; - } - VIR_FREE(str); - virDomainObjTaint(obj, flag); - } + int rc; + virDomainTaintFlags taint; + + if ((rc = virXMLPropEnum(taintNodes[i], "flag", + virDomainTaintTypeFromString, + VIR_XML_PROP_NONE, &taint)) < 0) + goto error; + + if (rc == 1) + virDomainObjTaint(obj, taint); } if ((n = virXPathNodeSet("./deprecation", ctxt, &depNodes)) < 0) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/domain_conf.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0d396cbdda..7044701fac 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -20351,7 +20351,7 @@ virDomainObjParseXML(xmlDocPtr xml, long val; xmlNodePtr config; xmlNodePtr oldnode; - virDomainObj *obj; + g_autoptr(virDomainObj) obj = NULL; size_t i; int n; virDomainState state; @@ -20367,7 +20367,7 @@ virDomainObjParseXML(xmlDocPtr xml, if (!(config = virXPathNode("./domain", ctxt))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no domain config")); - goto error; + return NULL; } oldnode = ctxt->node; @@ -20375,17 +20375,17 @@ virDomainObjParseXML(xmlDocPtr xml, obj->def = virDomainDefParseXML(xml, ctxt, xmlopt, flags); ctxt->node = oldnode; if (!obj->def) - goto error; + return NULL; if (virXMLPropEnum(ctxt->node, "state", virDomainStateTypeFromString, VIR_XML_PROP_REQUIRED, &state) < 0) - goto error; + return NULL; if ((tmp = virXMLPropString(ctxt->node, "reason"))) { if ((reason = virDomainStateReasonFromString(state, tmp)) < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("invalid domain state reason '%s'"), tmp); - goto error; + return NULL; } } @@ -20394,12 +20394,12 @@ virDomainObjParseXML(xmlDocPtr xml, if (virXPathLong("string(./@pid)", ctxt, &val) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("invalid pid")); - goto error; + return NULL; } obj->pid = (pid_t)val; if ((n = virXPathNodeSet("./taint", ctxt, &taintNodes)) < 0) - goto error; + return NULL; for (i = 0; i < n; i++) { int rc; virDomainTaintFlags taint; @@ -20407,14 +20407,14 @@ virDomainObjParseXML(xmlDocPtr xml, if ((rc = virXMLPropEnum(taintNodes[i], "flag", virDomainTaintTypeFromString, VIR_XML_PROP_NONE, &taint)) < 0) - goto error; + return NULL; if (rc == 1) virDomainObjTaint(obj, taint); } if ((n = virXPathNodeSet("./deprecation", ctxt, &depNodes)) < 0) - goto error; + return NULL; for (i = 0; i < n; i++) { g_autofree char *str = virXMLNodeContentString(depNodes[i]); virDomainObjDeprecation(obj, str); @@ -20422,24 +20422,20 @@ virDomainObjParseXML(xmlDocPtr xml, if (xmlopt->privateData.parse && xmlopt->privateData.parse(ctxt, obj, &xmlopt->config) < 0) - goto error; + return NULL; if (xmlopt->privateData.getParseOpaque) parseOpaque = xmlopt->privateData.getParseOpaque(obj); /* callback to fill driver specific domain aspects */ if (virDomainDefPostParse(obj->def, flags, xmlopt, parseOpaque) < 0) - goto error; + return NULL; /* validate configuration */ if (virDomainDefValidate(obj->def, flags, xmlopt, parseOpaque) < 0) - goto error; - - return obj; + return NULL; - error: - virObjectUnref(obj); - return NULL; + return g_steal_pointer(&obj); } -- 2.26.3
The width of `unsigned long` differs on 32 bit and 64 bit architectures. There is no compelling reason why the maximum DHCP lease time should depend on the architecture. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/network_conf.c | 8 ++++---- src/conf/network_conf.h | 2 +- src/network/bridge_driver.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d6eafa3f57..c3c335135b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -412,13 +412,13 @@ virNetworkDHCPLeaseTimeDefParseXML(virNetworkDHCPLeaseTimeDef **lease, virNetworkDHCPLeaseTimeDef *new_lease = NULL; g_autofree char *expirystr = NULL; g_autofree char *unitstr = NULL; - unsigned long expiry; + unsigned long long expiry; int unit = VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES; if (!(expirystr = virXMLPropString(node, "expiry"))) return 0; - if (virStrToLong_ul(expirystr, NULL, 10, &expiry) < 0) { + if (virStrToLong_ull(expirystr, NULL, 10, &expiry) < 0) { virReportError(VIR_ERR_XML_ERROR, _("failed to parse expiry value '%s'"), expirystr); return -1; @@ -2312,7 +2312,7 @@ virNetworkIPDefFormat(virBuffer *buf, if (!lease->expiry) { virBufferAddLit(buf, "<lease expiry='0'/>\n"); } else { - virBufferAsprintf(buf, "<lease expiry='%lu' unit='%s'/>\n", + virBufferAsprintf(buf, "<lease expiry='%llu' unit='%s'/>\n", lease->expiry, virNetworkDHCPLeaseTimeUnitTypeToString(lease->unit)); } @@ -2344,7 +2344,7 @@ virNetworkIPDefFormat(virBuffer *buf, if (!lease->expiry) { virBufferAddLit(buf, "<lease expiry='0'/>\n"); } else { - virBufferAsprintf(buf, "<lease expiry='%lu' unit='%s'/>\n", + virBufferAsprintf(buf, "<lease expiry='%llu' unit='%s'/>\n", lease->expiry, virNetworkDHCPLeaseTimeUnitTypeToString(lease->unit)); } diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index a7e6b7a2a6..6199f3f588 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -105,7 +105,7 @@ VIR_ENUM_DECL(virNetworkForwardDriverName); typedef struct _virNetworkDHCPLeaseTimeDef virNetworkDHCPLeaseTimeDef; struct _virNetworkDHCPLeaseTimeDef { - unsigned long expiry; + unsigned long long expiry; virNetworkDHCPLeaseTimeUnitType unit; }; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index ee3f9dab0a..a711b34c48 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -988,7 +988,7 @@ networkBuildDnsmasqLeaseTime(virNetworkDHCPLeaseTimeDef *lease) } else { unit = virNetworkDHCPLeaseTimeUnitTypeToString(lease->unit); /* We get only first compatible char from string: 's', 'm' or 'h' */ - virBufferAsprintf(&buf, "%lu%c", lease->expiry, unit[0]); + virBufferAsprintf(&buf, "%llu%c", lease->expiry, unit[0]); } return virBufferContentAndReset(&buf); -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/network_conf.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c3c335135b..a9eadff29c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -410,27 +410,21 @@ virNetworkDHCPLeaseTimeDefParseXML(virNetworkDHCPLeaseTimeDef **lease, xmlNodePtr node) { virNetworkDHCPLeaseTimeDef *new_lease = NULL; - g_autofree char *expirystr = NULL; - g_autofree char *unitstr = NULL; unsigned long long expiry; - int unit = VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES; + virNetworkDHCPLeaseTimeUnitType unit; + int rc; - if (!(expirystr = virXMLPropString(node, "expiry"))) + if ((rc = virXMLPropULongLong(node, "expiry", 0, VIR_XML_PROP_NONE, &expiry)) < 0) + return -1; + + if (rc == 0) return 0; - if (virStrToLong_ull(expirystr, NULL, 10, &expiry) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("failed to parse expiry value '%s'"), expirystr); + if (virXMLPropEnumDefault(node, "unit", + virNetworkDHCPLeaseTimeUnitTypeFromString, + VIR_XML_PROP_NONE, &unit, + VIR_NETWORK_DHCP_LEASETIME_UNIT_MINUTES) < 0) return -1; - } - - if ((unitstr = virXMLPropString(node, "unit"))) { - if ((unit = virNetworkDHCPLeaseTimeUnitTypeFromString(unitstr)) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid unit: %s"), unitstr); - return -1; - } - } /* infinite */ if (expiry > 0) { -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 34 ++++++---------------------------- 1 file changed, 6 insertions(+), 28 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index faa36c1944..ef09ed0a6f 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1601,38 +1601,16 @@ virPCIEDeviceInfoLinkParseXML(xmlXPathContextPtr ctxt, virPCIELink *lnk) { VIR_XPATH_NODE_AUTORESTORE(ctxt) - int speed; - g_autofree char *speedStr = NULL; - g_autofree char *portStr = NULL; - ctxt->node = linkNode; - - if (virXPathUInt("number(./@width)", ctxt, &lnk->width) < 0) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("mandatory attribute 'width' is missing or malformed")); + if (virXMLPropUInt(linkNode, "width", 0, VIR_XML_PROP_REQUIRED, &lnk->width) < 0) return -1; - } - if ((speedStr = virXPathString("string(./@speed)", ctxt))) { - if ((speed = virPCIELinkSpeedTypeFromString(speedStr)) < 0) { - virReportError(VIR_ERR_XML_DETAIL, - _("malformed 'speed' attribute: %s"), - speedStr); - return -1; - } - lnk->speed = speed; - } + if (virXMLPropEnum(linkNode, "speed", virPCIELinkSpeedTypeFromString, + VIR_XML_PROP_NONE, &lnk->speed) < 0) + return -1; - if ((portStr = virXPathString("string(./@port)", ctxt))) { - if (virStrToLong_i(portStr, NULL, 10, &lnk->port) < 0) { - virReportError(VIR_ERR_XML_DETAIL, - _("malformed 'port' attribute: %s"), - portStr); - return -1; - } - } else { - lnk->port = -1; - } + if (virXMLPropInt(linkNode, "port", 10, VIR_XML_PROP_NONE, &lnk->port, -1) < 0) + return -1; return 0; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index ef09ed0a6f..34c8aa988e 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1596,12 +1596,9 @@ virNodeDevCapPCIDevIommuGroupParseXML(xmlXPathContextPtr ctxt, static int -virPCIEDeviceInfoLinkParseXML(xmlXPathContextPtr ctxt, - xmlNodePtr linkNode, +virPCIEDeviceInfoLinkParseXML(xmlNodePtr linkNode, virPCIELink *lnk) { - VIR_XPATH_NODE_AUTORESTORE(ctxt) - if (virXMLPropUInt(linkNode, "width", 0, VIR_XML_PROP_REQUIRED, &lnk->width) < 0) return -1; @@ -1629,16 +1626,14 @@ virPCIEDeviceInfoParseXML(xmlXPathContextPtr ctxt, if ((lnk = virXPathNode("./link[@validity='cap']", ctxt))) { pci_express->link_cap = g_new0(virPCIELink, 1); - if (virPCIEDeviceInfoLinkParseXML(ctxt, lnk, - pci_express->link_cap) < 0) + if (virPCIEDeviceInfoLinkParseXML(lnk, pci_express->link_cap) < 0) return -1; } if ((lnk = virXPathNode("./link[@validity='sta']", ctxt))) { pci_express->link_sta = g_new0(virPCIELink, 1); - if (virPCIEDeviceInfoLinkParseXML(ctxt, lnk, - pci_express->link_sta) < 0) + if (virPCIEDeviceInfoLinkParseXML(lnk, pci_express->link_sta) < 0) return -1; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 34c8aa988e..b3d5bc1515 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1972,24 +1972,13 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, const char *virt_type) { virNodeDevCapsDef *caps; - g_autofree char *tmp = NULL; - int val, ret = -1; + int ret = -1; caps = g_new0(virNodeDevCapsDef, 1); - tmp = virXMLPropString(node, "type"); - if (!tmp) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing capability type")); + if (virXMLPropEnum(node, "type", virNodeDevCapTypeFromString, + VIR_XML_PROP_REQUIRED, &caps->data.type) < 0) goto error; - } - - if ((val = virNodeDevCapTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown capability type '%s'"), tmp); - goto error; - } - caps->data.type = val; switch (caps->data.type) { case VIR_NODE_DEV_CAP_SYSTEM: -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h index 85790ad96c..a60562e4fe 100644 --- a/src/conf/node_device_conf.h +++ b/src/conf/node_device_conf.h @@ -377,6 +377,8 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDeviceDef, virNodeDeviceDefFree); void virNodeDevCapsDefFree(virNodeDevCapsDef *caps); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNodeDevCapsDef, virNodeDevCapsDefFree); + #define VIR_CONNECT_LIST_NODE_DEVICES_FILTERS_CAP \ (VIR_CONNECT_LIST_NODE_DEVICES_CAP_SYSTEM | \ VIR_CONNECT_LIST_NODE_DEVICES_CAP_PCI_DEV | \ -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index b3d5bc1515..04014f75dc 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -1971,14 +1971,12 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, int create, const char *virt_type) { - virNodeDevCapsDef *caps; + g_autoptr(virNodeDevCapsDef) caps = g_new0(virNodeDevCapsDef, 1); int ret = -1; - caps = g_new0(virNodeDevCapsDef, 1); - if (virXMLPropEnum(node, "type", virNodeDevCapTypeFromString, VIR_XML_PROP_REQUIRED, &caps->data.type) < 0) - goto error; + return NULL; switch (caps->data.type) { case VIR_NODE_DEV_CAP_SYSTEM: @@ -2050,12 +2048,9 @@ virNodeDevCapsDefParseXML(xmlXPathContextPtr ctxt, } if (ret < 0) - goto error; - return caps; + return NULL; - error: - virNodeDevCapsDefFree(caps); - return NULL; + return g_steal_pointer(&caps); } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 04014f75dc..4477a8d9d2 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2089,24 +2089,13 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, for (i = 0, m = 0; i < n; i++) { xmlNodePtr node = nodes[i]; - g_autofree char *tmp = virXMLPropString(node, "type"); - int val; + virNodeDevDevnodeType val; - if (!tmp) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("missing devnode type")); + if (virXMLPropEnum(node, "type", virNodeDevDevnodeTypeFromString, + VIR_XML_PROP_REQUIRED, &val) < 0) goto error; - } - - val = virNodeDevDevnodeTypeFromString(tmp); - - if (val < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown devnode type '%s'"), tmp); - goto error; - } - switch ((virNodeDevDevnodeType)val) { + switch (val) { case VIR_NODE_DEV_DEVNODE_DEV: if (!(def->devnode = virXMLNodeContentString(node))) goto error; -- 2.26.3
On 5/10/21 2:48 PM, Tim Wiederhake wrote:
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html
Tim Wiederhake (10): virDomainObjParseXML: Use virXMLProp* virDomainObjParseXML: Use g_autoptr virNetworkDHCPLeaseTimeDef: Make expiry unsigned long long virNetworkDHCPLeaseTimeDefParseXML: Use virXMLProp* virPCIEDeviceInfoLinkParseXML: Use virXMLProp* virPCIEDeviceInfoLinkParseXML: Remove unused parameter ctxt virNodeDevCapsDefParseXML: Use virXMLProp* conf: node_device: Register autoptr cleanup function for virNodeDevCapsDef virNodeDevCapsDefParseXML: Use g_autoptr virNodeDeviceDefParseXML: Use virXMLProp*
src/conf/domain_conf.c | 64 ++++++++++--------------- src/conf/network_conf.c | 32 +++++-------- src/conf/network_conf.h | 2 +- src/conf/node_device_conf.c | 94 ++++++++----------------------------- src/conf/node_device_conf.h | 2 + src/network/bridge_driver.c | 2 +- 6 files changed, 62 insertions(+), 134 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
participants (2)
-
Michal Prívozník -
Tim Wiederhake