[libvirt PATCH v2 0/6] Refactor more XML parsing boilerplate code, part X
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html Changes since V1: * Split up VIR_FREE'd and reused ´g_autofree xmlNodePtr *´ variables. Tim Wiederhake (6): virNodeDeviceDefParseXML: Use g_auto* virDomainNumatuneNodeParseXML: Use virXMLProp* virDomainNumatuneNodeParseXML: Use g_autofree virDomainNumaDefNodeDistanceParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use g_autofree src/conf/node_device_conf.c | 44 ++- src/conf/numa_conf.c | 305 ++++++------------ .../hugepages-memaccess-invalid.err | 2 +- 3 files changed, 110 insertions(+), 241 deletions(-) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 44 +++++++++++++++---------------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4477a8d9d2..861f43f6c4 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2059,21 +2059,20 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, int create, const char *virt_type) { - virNodeDeviceDef *def; + g_autoptr(virNodeDeviceDef) def = g_new0(virNodeDeviceDef, 1); virNodeDevCapsDef **next_cap; - xmlNodePtr *nodes = NULL; + g_autofree xmlNodePtr *devnode = NULL; + g_autofree xmlNodePtr *capability = NULL; int n, m; size_t i; - def = g_new0(virNodeDeviceDef, 1); - /* Extract device name */ if (create == EXISTING_DEVICE) { def->name = virXPathString("string(./name[1])", ctxt); if (!def->name) { virReportError(VIR_ERR_NO_NAME, NULL); - goto error; + return NULL; } } else { def->name = g_strdup("new device"); @@ -2082,27 +2081,27 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, def->sysfs_path = virXPathString("string(./path[1])", ctxt); /* Parse devnodes */ - if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0) - goto error; + if ((n = virXPathNodeSet("./devnode", ctxt, &devnode)) < 0) + return NULL; def->devlinks = g_new0(char *, n + 1); for (i = 0, m = 0; i < n; i++) { - xmlNodePtr node = nodes[i]; + xmlNodePtr node = devnode[i]; virNodeDevDevnodeType val; if (virXMLPropEnum(node, "type", virNodeDevDevnodeTypeFromString, VIR_XML_PROP_REQUIRED, &val) < 0) - goto error; + return NULL; switch (val) { case VIR_NODE_DEV_DEVNODE_DEV: if (!(def->devnode = virXMLNodeContentString(node))) - goto error; + return NULL; break; case VIR_NODE_DEV_DEVNODE_LINK: if (!(def->devlinks[m++] = virXMLNodeContentString(node))) - goto error; + return NULL; break; case VIR_NODE_DEV_DEVNODE_LAST: break; @@ -2118,7 +2117,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, _("when providing parent wwnn='%s', the " "wwpn must also be provided"), def->parent_wwnn); - goto error; + return NULL; } if (!def->parent_wwnn && def->parent_wwpn) { @@ -2126,42 +2125,35 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, _("when providing parent wwpn='%s', the " "wwnn must also be provided"), def->parent_wwpn); - goto error; + return NULL; } def->parent_fabric_wwn = virXPathString("string(./parent[1]/@fabric_wwn)", ctxt); /* Parse device capabilities */ - VIR_FREE(nodes); - if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) - goto error; + if ((n = virXPathNodeSet("./capability", ctxt, &capability)) < 0) + return NULL; if (n == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("no device capabilities for '%s'"), def->name); - goto error; + return NULL; } next_cap = &def->caps; for (i = 0; i < n; i++) { *next_cap = virNodeDevCapsDefParseXML(ctxt, def, - nodes[i], + capability[i], create, virt_type); if (!*next_cap) - goto error; + return NULL; next_cap = &(*next_cap)->next; } - VIR_FREE(nodes); - - return def; - error: - virNodeDeviceDefFree(def); - VIR_FREE(nodes); - return NULL; + return g_steal_pointer(&def); } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/numa_conf.c | 45 +++++++++++++------------------------------- 1 file changed, 13 insertions(+), 32 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 932af4a185..bae59ac7b8 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -186,25 +186,13 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, } for (i = 0; i < n; i++) { - int mode = 0; unsigned int cellid = 0; virDomainNumaNode *mem_node = NULL; xmlNodePtr cur_node = nodes[i]; - tmp = virXMLPropString(cur_node, "cellid"); - if (!tmp) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing required cellid attribute " - "in memnode element")); + if (virXMLPropUInt(cur_node, "cellid", 10, VIR_XML_PROP_REQUIRED, + &cellid) < 0) goto cleanup; - } - if (virStrToLong_uip(tmp, NULL, 10, &cellid) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid cellid attribute in memnode element: %s"), - tmp); - goto cleanup; - } - VIR_FREE(tmp); if (cellid >= numa->nmem_nodes) { virReportError(VIR_ERR_XML_ERROR, "%s", @@ -222,25 +210,18 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, goto cleanup; } - tmp = virXMLPropString(cur_node, "mode"); - if (!tmp) { - mem_node->mode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; - } else { - if ((mode = virDomainNumatuneMemModeTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid mode attribute in memnode element")); - goto cleanup; - } + if (virXMLPropEnumDefault(cur_node, "mode", + virDomainNumatuneMemModeTypeFromString, + VIR_XML_PROP_NONE, &mem_node->mode, + VIR_DOMAIN_NUMATUNE_MEM_STRICT) < 0) + goto cleanup; - if (numa->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && - mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("'restrictive' mode is required in memnode element " - "when mode is 'restrictive' in memory element")); - goto cleanup; - } - VIR_FREE(tmp); - mem_node->mode = mode; + if (numa->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && + mem_node->mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("'restrictive' mode is required in memnode element " + "when mode is 'restrictive' in memory element")); + goto cleanup; } tmp = virXMLPropString(cur_node, "nodeset"); -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/numa_conf.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index bae59ac7b8..15bfda3aa1 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -155,16 +155,14 @@ static int virDomainNumatuneNodeParseXML(virDomainNuma *numa, xmlXPathContextPtr ctxt) { - char *tmp = NULL; int n = 0; - int ret = -1; size_t i = 0; - xmlNodePtr *nodes = NULL; + g_autofree xmlNodePtr *nodes = NULL; if ((n = virXPathNodeSet("./numatune/memnode", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot extract memnode nodes")); - goto cleanup; + return -1; } if (!n) @@ -175,30 +173,31 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Per-node binding is not compatible with " "automatic NUMA placement.")); - goto cleanup; + return -1; } if (!numa->nmem_nodes) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Element 'memnode' is invalid without " "any guest NUMA cells")); - goto cleanup; + return -1; } for (i = 0; i < n; i++) { unsigned int cellid = 0; virDomainNumaNode *mem_node = NULL; xmlNodePtr cur_node = nodes[i]; + g_autofree char *tmp = NULL; if (virXMLPropUInt(cur_node, "cellid", 10, VIR_XML_PROP_REQUIRED, &cellid) < 0) - goto cleanup; + return -1; if (cellid >= numa->nmem_nodes) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Argument 'cellid' in memnode element must " "correspond to existing guest's NUMA cell")); - goto cleanup; + return -1; } mem_node = &numa->mem_nodes[cellid]; @@ -207,21 +206,21 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, virReportError(VIR_ERR_XML_ERROR, _("Multiple memnode elements with cellid %u"), cellid); - goto cleanup; + return -1; } if (virXMLPropEnumDefault(cur_node, "mode", virDomainNumatuneMemModeTypeFromString, VIR_XML_PROP_NONE, &mem_node->mode, VIR_DOMAIN_NUMATUNE_MEM_STRICT) < 0) - goto cleanup; + return -1; if (numa->memory.mode == VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE && mem_node->mode != VIR_DOMAIN_NUMATUNE_MEM_RESTRICTIVE) { virReportError(VIR_ERR_XML_ERROR, "%s", _("'restrictive' mode is required in memnode element " "when mode is 'restrictive' in memory element")); - goto cleanup; + return -1; } tmp = virXMLPropString(cur_node, "nodeset"); @@ -229,24 +228,19 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing required nodeset attribute " "in memnode element")); - goto cleanup; + return -1; } if (virBitmapParse(tmp, &mem_node->nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto cleanup; + return -1; if (virBitmapIsAllClear(mem_node->nodeset)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid value of 'nodeset': %s"), tmp); - goto cleanup; + return -1; } - VIR_FREE(tmp); } - ret = 0; - cleanup: - VIR_FREE(nodes); - VIR_FREE(tmp); - return ret; + return 0; } int -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/numa_conf.c | 42 ++++-------------------------------------- 1 file changed, 4 insertions(+), 38 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 15bfda3aa1..60cd7767ef 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -742,7 +742,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, { int ret = -1; int sibling; - char *tmp = NULL; xmlNodePtr *nodes = NULL; size_t i, ndistances = def->nmem_nodes; @@ -764,24 +763,9 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, virDomainNumaDistance *rdist; unsigned int sibling_id, sibling_value; - /* siblings are in order of parsing or explicitly numbered */ - if (!(tmp = virXMLPropString(nodes[i], "id"))) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing 'id' attribute in NUMA " - "distances under 'cell id %d'"), - cur_cell); - goto cleanup; - } - - /* The "id" needs to be applicable */ - if (virStrToLong_uip(tmp, NULL, 10, &sibling_id) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid 'id' attribute in NUMA " - "distances for sibling: '%s'"), - tmp); + if (virXMLPropUInt(nodes[i], "id", 10, VIR_XML_PROP_REQUIRED, + &sibling_id) < 0) goto cleanup; - } - VIR_FREE(tmp); /* The "id" needs to be within numa/cell range */ if (sibling_id >= ndistances) { @@ -792,26 +776,9 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, goto cleanup; } - /* We need a locality value. Check and correct - * distance to local and distance to remote node. - */ - if (!(tmp = virXMLPropString(nodes[i], "value"))) { - virReportError(VIR_ERR_XML_ERROR, - _("Missing 'value' attribute in NUMA distances " - "under 'cell id %d' for 'sibling id %d'"), - cur_cell, sibling_id); - goto cleanup; - } - - /* The "value" needs to be applicable */ - if (virStrToLong_uip(tmp, NULL, 10, &sibling_value) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("'value %s' is invalid for " - "'sibling id %d' under NUMA 'cell id %d'"), - tmp, sibling_id, cur_cell); + if (virXMLPropUInt(nodes[i], "value", 10, VIR_XML_PROP_REQUIRED, + &sibling_value) < 0) goto cleanup; - } - VIR_FREE(tmp); /* Assure LOCAL_DISTANCE <= "value" <= UNREACHABLE * and correct LOCAL_DISTANCE setting if such applies. @@ -866,7 +833,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, def->mem_nodes[i].ndistances = 0; } VIR_FREE(nodes); - VIR_FREE(tmp); return ret; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/numa_conf.c | 128 +++++------------- .../hugepages-memaccess-invalid.err | 2 +- 2 files changed, 35 insertions(+), 95 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 60cd7767ef..9fe4998951 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -954,26 +954,23 @@ virDomainNumaDefParseXML(virDomainNuma *def, for (i = 0; i < n; i++) { VIR_XPATH_NODE_AUTORESTORE(ctxt) int rc; - unsigned int cur_cell = i; + unsigned int cur_cell; - /* cells are in order of parsing or explicitly numbered */ - if ((tmp = virXMLPropString(nodes[i], "id"))) { - if (virStrToLong_uip(tmp, NULL, 10, &cur_cell) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid 'id' attribute in NUMA cell: '%s'"), - tmp); - goto cleanup; - } + if ((rc = virXMLPropUInt(nodes[i], "id", 10, VIR_XML_PROP_NONE, + &cur_cell)) < 0) + goto cleanup; - if (cur_cell >= n) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Exactly one 'cell' element per guest " - "NUMA cell allowed, non-contiguous ranges or " - "ranges not starting from 0 are not allowed")); - goto cleanup; - } + if (rc == 0) + cur_cell = i; + + /* cells are in order of parsing or explicitly numbered */ + if (cur_cell >= n) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Exactly one 'cell' element per guest " + "NUMA cell allowed, non-contiguous ranges or " + "ranges not starting from 0 are not allowed")); + goto cleanup; } - VIR_FREE(tmp); if (def->mem_nodes[cur_cell].mem) { virReportError(VIR_ERR_XML_ERROR, @@ -1013,29 +1010,15 @@ virDomainNumaDefParseXML(virDomainNuma *def, &def->mem_nodes[cur_cell].mem, true, false) < 0) goto cleanup; - if ((tmp = virXMLPropString(nodes[i], "memAccess"))) { - if ((rc = virDomainMemoryAccessTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid 'memAccess' attribute value '%s'"), - tmp); - goto cleanup; - } - - def->mem_nodes[cur_cell].memAccess = rc; - VIR_FREE(tmp); - } - - if ((tmp = virXMLPropString(nodes[i], "discard"))) { - if ((rc = virTristateBoolTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid 'discard' attribute value '%s'"), - tmp); - goto cleanup; - } + if (virXMLPropEnum(nodes[i], "memAccess", + virDomainMemoryAccessTypeFromString, + VIR_XML_PROP_NONZERO, + &def->mem_nodes[cur_cell].memAccess) < 0) + goto cleanup; - def->mem_nodes[cur_cell].discard = rc; - VIR_FREE(tmp); - } + if (virXMLPropTristateBool(nodes[i], "discard", VIR_XML_PROP_NONE, + &def->mem_nodes[cur_cell].discard) < 0) + goto cleanup; /* Parse NUMA distances info */ if (virDomainNumaDefNodeDistanceParseXML(def, ctxt, cur_cell) < 0) @@ -1057,24 +1040,15 @@ virDomainNumaDefParseXML(virDomainNuma *def, unsigned int initiator; unsigned int target; unsigned int cache = 0; - int accessType; + virDomainMemoryLatency accessType; unsigned long long value; if (virXMLNodeNameEqual(nodes[i], "latency")) { type = VIR_DOMAIN_NUMA_INTERCONNECT_TYPE_LATENCY; - if (!(tmp = virXMLPropString(nodes[i], "value"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'value' attribute in NUMA interconnects")); - goto cleanup; - } - - if (virStrToLong_ullp(tmp, NULL, 10, &value) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid 'value' attribute in NUMA interconnects")); + if (virXMLPropULongLong(nodes[i], "value", 10, + VIR_XML_PROP_REQUIRED, &value) < 0) goto cleanup; - } - VIR_FREE(tmp); } else if (virXMLNodeNameEqual(nodes[i], "bandwidth")) { VIR_XPATH_NODE_AUTORESTORE(ctxt) type = VIR_DOMAIN_NUMA_INTERCONNECT_TYPE_BANDWIDTH; @@ -1088,56 +1062,22 @@ virDomainNumaDefParseXML(virDomainNuma *def, continue; } - if (!(tmp = virXMLPropString(nodes[i], "initiator"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'initiator' attribute in NUMA interconnects")); + if (virXMLPropUInt(nodes[i], "initiator", 10, VIR_XML_PROP_REQUIRED, + &initiator) < 0) goto cleanup; - } - - if (virStrToLong_uip(tmp, NULL, 10, &initiator) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid 'initiator' attribute in NUMA interconnects")); - goto cleanup; - } - VIR_FREE(tmp); - - if (!(tmp = virXMLPropString(nodes[i], "target"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'target' attribute in NUMA interconnects")); - goto cleanup; - } - if (virStrToLong_uip(tmp, NULL, 10, &target) < 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid 'target' attribute in NUMA interconnects")); + if (virXMLPropUInt(nodes[i], "target", 10, VIR_XML_PROP_REQUIRED, + &target) < 0) goto cleanup; - } - VIR_FREE(tmp); - - /* cache attribute is optional */ - if ((tmp = virXMLPropString(nodes[i], "cache"))) { - if (virStrToLong_uip(tmp, NULL, 10, &cache) < 0 || - cache == 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid 'cache' attribute in NUMA interconnects")); - goto cleanup; - } - } - VIR_FREE(tmp); - - if (!(tmp = virXMLPropString(nodes[i], "type"))) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Missing 'type' attribute in NUMA interconnects")); + if (virXMLPropUInt(nodes[i], "cache", 10, VIR_XML_PROP_NONE, + &cache) < 0) goto cleanup; - } - if ((accessType = virDomainMemoryLatencyTypeFromString(tmp)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("Invalid 'type' attribute in NUMA interconnects")); + if (virXMLPropEnum(nodes[i], "type", virDomainMemoryLatencyTypeFromString, + VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, + &accessType) < 0) goto cleanup; - } - VIR_FREE(tmp); def->interconnects[i] = (virDomainNumaInterconnect) {type, initiator, target, cache, accessType, value}; diff --git a/tests/qemuxml2argvdata/hugepages-memaccess-invalid.err b/tests/qemuxml2argvdata/hugepages-memaccess-invalid.err index 75ddd3e282..a1f07fb04d 100644 --- a/tests/qemuxml2argvdata/hugepages-memaccess-invalid.err +++ b/tests/qemuxml2argvdata/hugepages-memaccess-invalid.err @@ -1 +1 @@ -unsupported configuration: Invalid 'memAccess' attribute value 'invalid' +XML error: Invalid value for attribute 'memAccess' in element 'cell': 'invalid'. -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/numa_conf.c | 84 +++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 44 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 9fe4998951..525bc28962 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -932,20 +932,20 @@ int virDomainNumaDefParseXML(virDomainNuma *def, xmlXPathContextPtr ctxt) { - xmlNodePtr *nodes = NULL; - char *tmp = NULL; + g_autofree xmlNodePtr *cell = NULL; + g_autofree xmlNodePtr *interconnect = NULL; + int n; size_t i, j; - int ret = -1; /* check if NUMA definition is present */ if (!virXPathNode("./cpu/numa[1]", ctxt)) return 0; - if ((n = virXPathNodeSet("./cpu/numa[1]/cell", ctxt, &nodes)) <= 0) { + if ((n = virXPathNodeSet("./cpu/numa[1]/cell", ctxt, &cell)) <= 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("NUMA topology defined without NUMA cells")); - goto cleanup; + return -1; } def->mem_nodes = g_new0(struct _virDomainNumaNode, n); @@ -953,12 +953,13 @@ virDomainNumaDefParseXML(virDomainNuma *def, for (i = 0; i < n; i++) { VIR_XPATH_NODE_AUTORESTORE(ctxt) + g_autofree char *tmp = NULL; int rc; unsigned int cur_cell; - if ((rc = virXMLPropUInt(nodes[i], "id", 10, VIR_XML_PROP_NONE, + if ((rc = virXMLPropUInt(cell[i], "id", 10, VIR_XML_PROP_NONE, &cur_cell)) < 0) - goto cleanup; + return -1; if (rc == 0) cur_cell = i; @@ -969,25 +970,24 @@ virDomainNumaDefParseXML(virDomainNuma *def, _("Exactly one 'cell' element per guest " "NUMA cell allowed, non-contiguous ranges or " "ranges not starting from 0 are not allowed")); - goto cleanup; + return -1; } if (def->mem_nodes[cur_cell].mem) { virReportError(VIR_ERR_XML_ERROR, _("Duplicate NUMA cell info for cell id '%u'"), cur_cell); - goto cleanup; + return -1; } - if ((tmp = virXMLPropString(nodes[i], "cpus"))) { + if ((tmp = virXMLPropString(cell[i], "cpus"))) { g_autoptr(virBitmap) cpumask = NULL; if (virBitmapParse(tmp, &cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto cleanup; + return -1; if (!virBitmapIsAllClear(cpumask)) def->mem_nodes[cur_cell].cpumask = g_steal_pointer(&cpumask); - VIR_FREE(tmp); } for (j = 0; j < n; j++) { @@ -1001,38 +1001,38 @@ virDomainNumaDefParseXML(virDomainNuma *def, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("NUMA cells %u and %zu have overlapping vCPU ids"), cur_cell, j); - goto cleanup; + return -1; } } - ctxt->node = nodes[i]; + ctxt->node = cell[i]; if (virDomainParseMemory("./@memory", "./@unit", ctxt, &def->mem_nodes[cur_cell].mem, true, false) < 0) - goto cleanup; + return -1; - if (virXMLPropEnum(nodes[i], "memAccess", + if (virXMLPropEnum(cell[i], "memAccess", virDomainMemoryAccessTypeFromString, VIR_XML_PROP_NONZERO, &def->mem_nodes[cur_cell].memAccess) < 0) - goto cleanup; + return -1; - if (virXMLPropTristateBool(nodes[i], "discard", VIR_XML_PROP_NONE, + if (virXMLPropTristateBool(cell[i], "discard", VIR_XML_PROP_NONE, &def->mem_nodes[cur_cell].discard) < 0) - goto cleanup; + return -1; /* Parse NUMA distances info */ if (virDomainNumaDefNodeDistanceParseXML(def, ctxt, cur_cell) < 0) - goto cleanup; + return -1; /* Parse cache info */ if (virDomainNumaDefNodeCacheParseXML(def, ctxt, cur_cell) < 0) - goto cleanup; + return -1; } - VIR_FREE(nodes); if ((n = virXPathNodeSet("./cpu/numa[1]/interconnects[1]/latency|" - "./cpu/numa[1]/interconnects[1]/bandwidth", ctxt, &nodes)) < 0) - goto cleanup; + "./cpu/numa[1]/interconnects[1]/bandwidth", ctxt, + &interconnect)) < 0) + return -1; def->interconnects = g_new0(virDomainNumaInterconnect, n); for (i = 0; i < n; i++) { @@ -1043,53 +1043,49 @@ virDomainNumaDefParseXML(virDomainNuma *def, virDomainMemoryLatency accessType; unsigned long long value; - if (virXMLNodeNameEqual(nodes[i], "latency")) { + if (virXMLNodeNameEqual(interconnect[i], "latency")) { type = VIR_DOMAIN_NUMA_INTERCONNECT_TYPE_LATENCY; - if (virXMLPropULongLong(nodes[i], "value", 10, + if (virXMLPropULongLong(interconnect[i], "value", 10, VIR_XML_PROP_REQUIRED, &value) < 0) - goto cleanup; - } else if (virXMLNodeNameEqual(nodes[i], "bandwidth")) { + return -1; + } else if (virXMLNodeNameEqual(interconnect[i], "bandwidth")) { VIR_XPATH_NODE_AUTORESTORE(ctxt) type = VIR_DOMAIN_NUMA_INTERCONNECT_TYPE_BANDWIDTH; - ctxt->node = nodes[i]; + ctxt->node = interconnect[i]; if (virDomainParseMemory("./@value", "./@unit", ctxt, &value, true, false) < 0) - goto cleanup; + return -1; } else { /* Ignore yet unknown child elements. */ continue; } - if (virXMLPropUInt(nodes[i], "initiator", 10, VIR_XML_PROP_REQUIRED, + if (virXMLPropUInt(interconnect[i], "initiator", 10, VIR_XML_PROP_REQUIRED, &initiator) < 0) - goto cleanup; + return -1; - if (virXMLPropUInt(nodes[i], "target", 10, VIR_XML_PROP_REQUIRED, + if (virXMLPropUInt(interconnect[i], "target", 10, VIR_XML_PROP_REQUIRED, &target) < 0) - goto cleanup; + return -1; - if (virXMLPropUInt(nodes[i], "cache", 10, VIR_XML_PROP_NONE, + if (virXMLPropUInt(interconnect[i], "cache", 10, VIR_XML_PROP_NONE, &cache) < 0) - goto cleanup; + return -1; - if (virXMLPropEnum(nodes[i], "type", virDomainMemoryLatencyTypeFromString, + if (virXMLPropEnum(interconnect[i], "type", + virDomainMemoryLatencyTypeFromString, VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, &accessType) < 0) - goto cleanup; + return -1; def->interconnects[i] = (virDomainNumaInterconnect) {type, initiator, target, cache, accessType, value}; def->ninterconnects++; } - ret = 0; - - cleanup: - VIR_FREE(nodes); - VIR_FREE(tmp); - return ret; + return 0; } -- 2.26.3
On 5/13/21 11:31 AM, Tim Wiederhake wrote:
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html
Changes since V1: * Split up VIR_FREE'd and reused ´g_autofree xmlNodePtr *´ variables.
Tim Wiederhake (6): virNodeDeviceDefParseXML: Use g_auto* virDomainNumatuneNodeParseXML: Use virXMLProp* virDomainNumatuneNodeParseXML: Use g_autofree virDomainNumaDefNodeDistanceParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use g_autofree
src/conf/node_device_conf.c | 44 ++- src/conf/numa_conf.c | 305 ++++++------------ .../hugepages-memaccess-invalid.err | 2 +- 3 files changed, 110 insertions(+), 241 deletions(-)
Reviewed-by: Laine Stump <laine@redhat.com> I'll push them as soon as simple smoke test build has completed
participants (2)
-
Laine Stump -
Tim Wiederhake