[libvirt PATCH 00/10] Refactor more XML parsing boilerplate code, part X
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html Tim Wiederhake (10): virNodeDeviceDefParseXML: Use g_auto* virDomainNumatuneNodeParseXML: Use virXMLProp* virDomainNumatuneNodeParseXML: Use g_autofree virDomainNumaDefNodeDistanceParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use g_autofree virStorageAdapterParseXMLFCHost: Use virXMLProp* virStoragePoolDefParseSource: Use virXMLProp* virStoragePoolDefParseSource: Use VIR_XPATH_NODE_AUTORESTORE virStoragePoolDefParseXML: Use virXMLProp* src/conf/node_device_conf.c | 34 +-- src/conf/numa_conf.c | 285 +++++------------- src/conf/storage_adapter_conf.c | 17 +- src/conf/storage_conf.c | 83 ++--- .../hugepages-memaccess-invalid.err | 2 +- 5 files changed, 125 insertions(+), 296 deletions(-) -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4477a8d9d2..5ac046f768 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2059,21 +2059,19 @@ 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 *nodes = 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"); @@ -2083,7 +2081,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, /* Parse devnodes */ if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0) - goto error; + return NULL; def->devlinks = g_new0(char *, n + 1); @@ -2093,16 +2091,16 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, 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 +2116,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,7 +2124,7 @@ 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); @@ -2134,13 +2132,13 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, /* Parse device capabilities */ VIR_FREE(nodes); if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) - goto error; + 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; @@ -2150,18 +2148,12 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, 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
On 5/11/21 11:01 AM, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/node_device_conf.c | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c index 4477a8d9d2..5ac046f768 100644 --- a/src/conf/node_device_conf.c +++ b/src/conf/node_device_conf.c @@ -2059,21 +2059,19 @@ 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 *nodes = 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"); @@ -2083,7 +2081,7 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
/* Parse devnodes */ if ((n = virXPathNodeSet("./devnode", ctxt, &nodes)) < 0) - goto error; + return NULL;
def->devlinks = g_new0(char *, n + 1);
@@ -2093,16 +2091,16 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt,
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 +2116,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,7 +2124,7 @@ 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); @@ -2134,13 +2132,13 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, /* Parse device capabilities */ VIR_FREE(nodes);
The presence of a VIR_FREE() of a pointer that was declared g_autofree points out that it's being re-used, which we had decided is a bad idea (unless more discussion has happened on the topic that I missed). You should instead define two separate g_autofree pointers, one for each use.
if ((n = virXPathNodeSet("./capability", ctxt, &nodes)) < 0) - goto error; + 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; @@ -2150,18 +2148,12 @@ virNodeDeviceDefParseXML(xmlXPathContextPtr ctxt, 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); }
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 | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index bae59ac7b8..531bdc6eba 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -155,16 +155,15 @@ static int virDomainNumatuneNodeParseXML(virDomainNuma *numa, xmlXPathContextPtr ctxt) { - char *tmp = NULL; + g_autofree 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,14 +174,14 @@ 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++) { @@ -192,13 +191,13 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa, 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,20 @@ 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
On 5/11/21 11:01 AM, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/numa_conf.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index bae59ac7b8..531bdc6eba 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -155,16 +155,15 @@ static int virDomainNumatuneNodeParseXML(virDomainNuma *numa, xmlXPathContextPtr ctxt) { - char *tmp = NULL; + g_autofree char *tmp = NULL;
tmp should be defined inside the for loop so that it's autofreed after each iteration.
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,14 +174,14 @@ 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++) { @@ -192,13 +191,13 @@ virDomainNumatuneNodeParseXML(virDomainNuma *numa,
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,20 @@ 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
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 531bdc6eba..e631bfa341 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -743,7 +743,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, { int ret = -1; int sibling; - char *tmp = NULL; xmlNodePtr *nodes = NULL; size_t i, ndistances = def->nmem_nodes; @@ -765,24 +764,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) { @@ -793,26 +777,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. @@ -867,7 +834,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, def->mem_nodes[i].ndistances = 0; } VIR_FREE(nodes); - VIR_FREE(tmp); return ret; } -- 2.26.3
On 5/11/21 11:01 AM, Tim Wiederhake wrote:
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 531bdc6eba..e631bfa341 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -743,7 +743,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, { int ret = -1; int sibling; - char *tmp = NULL; xmlNodePtr *nodes = NULL;
(Not about *this* patch, but rather the absence of a related patch) The other "Use virXMLProp*" patches have a companion patch that makes "nodes" a g_autofree, but you haven't included that patch for this function.
size_t i, ndistances = def->nmem_nodes;
@@ -765,24 +764,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) { @@ -793,26 +777,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. @@ -867,7 +834,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, def->mem_nodes[i].ndistances = 0; } VIR_FREE(nodes); - VIR_FREE(tmp);
return ret; }
On Tue, 2021-05-11 at 12:15 -0400, Laine Stump wrote:
On 5/11/21 11:01 AM, Tim Wiederhake wrote:
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 531bdc6eba..e631bfa341 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -743,7 +743,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, { int ret = -1; int sibling; - char *tmp = NULL; xmlNodePtr *nodes = NULL;
(Not about *this* patch, but rather the absence of a related patch) The other "Use virXMLProp*" patches have a companion patch that makes "nodes" a g_autofree, but you haven't included that patch for this function.
g-autofree-ing this function is not trivial (see label "cleanup"), like it was with other function. The topic of this series-of-series is replacing virXMLPropString + virStrTo* combos with virXMLProp{enum,int,tristate,etc}, which is why at this time, this function stays un-g-autofree-d. I will propably have a look at it at some point in the future though. Cheers, Tim
size_t i, ndistances = def->nmem_nodes;
@@ -765,24 +764,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) { @@ -793,26 +777,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. @@ -867,7 +834,6 @@ virDomainNumaDefNodeDistanceParseXML(virDomainNuma *def, def->mem_nodes[i].ndistances = 0; } VIR_FREE(nodes); - VIR_FREE(tmp);
return ret; }
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 e631bfa341..27aac87a8d 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -955,26 +955,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, @@ -1014,29 +1011,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) @@ -1058,24 +1041,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; @@ -1089,56 +1063,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 | 49 +++++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c index 27aac87a8d..abb39a36ac 100644 --- a/src/conf/numa_conf.c +++ b/src/conf/numa_conf.c @@ -933,11 +933,9 @@ int virDomainNumaDefParseXML(virDomainNuma *def, xmlXPathContextPtr ctxt) { - xmlNodePtr *nodes = NULL; - char *tmp = NULL; + g_autofree xmlNodePtr *nodes = NULL; int n; size_t i, j; - int ret = -1; /* check if NUMA definition is present */ if (!virXPathNode("./cpu/numa[1]", ctxt)) @@ -946,7 +944,7 @@ virDomainNumaDefParseXML(virDomainNuma *def, if ((n = virXPathNodeSet("./cpu/numa[1]/cell", ctxt, &nodes)) <= 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); @@ -954,12 +952,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, &cur_cell)) < 0) - goto cleanup; + return -1; if (rc == 0) cur_cell = i; @@ -970,25 +969,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"))) { 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++) { @@ -1002,38 +1000,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]; if (virDomainParseMemory("./@memory", "./@unit", ctxt, &def->mem_nodes[cur_cell].mem, true, false) < 0) - goto cleanup; + return -1; if (virXMLPropEnum(nodes[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, &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; + return -1; def->interconnects = g_new0(virDomainNumaInterconnect, n); for (i = 0; i < n; i++) { @@ -1049,7 +1047,7 @@ virDomainNumaDefParseXML(virDomainNuma *def, if (virXMLPropULongLong(nodes[i], "value", 10, VIR_XML_PROP_REQUIRED, &value) < 0) - goto cleanup; + return -1; } else if (virXMLNodeNameEqual(nodes[i], "bandwidth")) { VIR_XPATH_NODE_AUTORESTORE(ctxt) type = VIR_DOMAIN_NUMA_INTERCONNECT_TYPE_BANDWIDTH; @@ -1057,7 +1055,7 @@ virDomainNumaDefParseXML(virDomainNuma *def, ctxt->node = nodes[i]; if (virDomainParseMemory("./@value", "./@unit", ctxt, &value, true, false) < 0) - goto cleanup; + return -1; } else { /* Ignore yet unknown child elements. */ continue; @@ -1065,32 +1063,27 @@ virDomainNumaDefParseXML(virDomainNuma *def, if (virXMLPropUInt(nodes[i], "initiator", 10, VIR_XML_PROP_REQUIRED, &initiator) < 0) - goto cleanup; + return -1; if (virXMLPropUInt(nodes[i], "target", 10, VIR_XML_PROP_REQUIRED, &target) < 0) - goto cleanup; + return -1; if (virXMLPropUInt(nodes[i], "cache", 10, VIR_XML_PROP_NONE, &cache) < 0) - goto cleanup; + return -1; if (virXMLPropEnum(nodes[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/11/21 11:01 AM, Tim Wiederhake wrote:
}
VIR_FREE(nodes); if ((n = virXPathNodeSet("./cpu/numa[1]/interconnects[1]/latency|" "./cpu/numa[1]/interconnects[1]/bandwidth", ctxt, &nodes)) <
Again, the VIR_FREE(nodes) points out that you're re-using a g_autofree pointer, and should instead use two separate pointers.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/storage_adapter_conf.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c index 142489f6cd..6b5a58e1e7 100644 --- a/src/conf/storage_adapter_conf.c +++ b/src/conf/storage_adapter_conf.c @@ -64,28 +64,17 @@ static int virStorageAdapterParseXMLFCHost(xmlNodePtr node, virStorageAdapterFCHost *fchost) { - char *managed = NULL; + if (virXMLPropTristateBool(node, "managed", VIR_XML_PROP_NONE, + &fchost->managed) < 0) + return -1; fchost->parent = virXMLPropString(node, "parent"); - if ((managed = virXMLPropString(node, "managed"))) { - int value; - if ((value = virTristateBoolTypeFromString(managed)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown fc_host managed setting '%s'"), - managed); - VIR_FREE(managed); - return -1; - } - fchost->managed = value; - } - fchost->parent_wwnn = virXMLPropString(node, "parent_wwnn"); fchost->parent_wwpn = virXMLPropString(node, "parent_wwpn"); fchost->parent_fabric_wwn = virXMLPropString(node, "parent_fabric_wwn"); fchost->wwpn = virXMLPropString(node, "wwpn"); fchost->wwnn = virXMLPropString(node, "wwnn"); - VIR_FREE(managed); return 0; } -- 2.26.3
On 5/11/21 11:01 AM, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/conf/storage_adapter_conf.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/src/conf/storage_adapter_conf.c b/src/conf/storage_adapter_conf.c index 142489f6cd..6b5a58e1e7 100644 --- a/src/conf/storage_adapter_conf.c +++ b/src/conf/storage_adapter_conf.c @@ -64,28 +64,17 @@ static int virStorageAdapterParseXMLFCHost(xmlNodePtr node, virStorageAdapterFCHost *fchost) { - char *managed = NULL; + if (virXMLPropTristateBool(node, "managed", VIR_XML_PROP_NONE, + &fchost->managed) < 0) + return -1;
fchost->parent = virXMLPropString(node, "parent"); - if ((managed = virXMLPropString(node, "managed"))) { - int value; - if ((value = virTristateBoolTypeFromString(managed)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown fc_host managed setting '%s'"), - managed); - VIR_FREE(managed); - return -1; - } - fchost->managed = value; - } - fchost->parent_wwnn = virXMLPropString(node, "parent_wwnn"); fchost->parent_wwpn = virXMLPropString(node, "parent_wwpn"); fchost->parent_fabric_wwn = virXMLPropString(node, "parent_fabric_wwn"); fchost->wwpn = virXMLPropString(node, "wwpn"); fchost->wwnn = virXMLPropString(node, "wwnn");
- VIR_FREE(managed); return 0; }
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/storage_conf.c | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 328650bd6a..435a029b4e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -531,7 +531,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStoragePoolOptions *options; int n; g_autoptr(virStorageAuthDef) authdef = NULL; - g_autofree char *port = NULL; g_autofree char *ver = NULL; g_autofree xmlNodePtr *nodeset = NULL; g_autofree char *sourcedir = NULL; @@ -580,16 +579,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } - port = virXMLPropString(nodeset[i], "port"); - if (port) { - if (virStrToLong_i(port, NULL, 10, &source->hosts[i].port) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid port number: %s"), - port); - goto cleanup; - } - } - VIR_FREE(port); + if (virXMLPropInt(nodeset[i], "port", 10, VIR_XML_PROP_NONE, + &source->hosts[i].port, 0) < 0) + goto cleanup; } } @@ -602,7 +594,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; for (i = 0; i < nsource; i++) { - g_autofree char *partsep = NULL; virStoragePoolSourceDevice dev = { .path = NULL }; dev.path = virXMLPropString(nodeset[i], "path"); @@ -612,17 +603,11 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; } - partsep = virXMLPropString(nodeset[i], "part_separator"); - if (partsep) { - int value = virTristateBoolTypeFromString(partsep); - if (value <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid part_separator setting '%s'"), - partsep); - virStoragePoolSourceDeviceClear(&dev); - goto cleanup; - } - dev.part_separator = value; + if (virXMLPropTristateBool(nodeset[i], "part_separator", + VIR_XML_PROP_NONE, + &dev.part_separator) < 0) { + virStoragePoolSourceDeviceClear(&dev); + goto cleanup; } if (VIR_APPEND_ELEMENT(source->devices, source->ndevice, dev) < 0) { -- 2.26.3
On 5/11/21 11:01 AM, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/conf/storage_conf.c | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 328650bd6a..435a029b4e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -531,7 +531,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, virStoragePoolOptions *options; int n; g_autoptr(virStorageAuthDef) authdef = NULL; - g_autofree char *port = NULL; g_autofree char *ver = NULL; g_autofree xmlNodePtr *nodeset = NULL; g_autofree char *sourcedir = NULL; @@ -580,16 +579,9 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; }
- port = virXMLPropString(nodeset[i], "port"); - if (port) { - if (virStrToLong_i(port, NULL, 10, &source->hosts[i].port) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid port number: %s"), - port); - goto cleanup; - } - } - VIR_FREE(port); + if (virXMLPropInt(nodeset[i], "port", 10, VIR_XML_PROP_NONE, + &source->hosts[i].port, 0) < 0) + goto cleanup; } }
@@ -602,7 +594,6 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup;
for (i = 0; i < nsource; i++) { - g_autofree char *partsep = NULL; virStoragePoolSourceDevice dev = { .path = NULL }; dev.path = virXMLPropString(nodeset[i], "path");
@@ -612,17 +603,11 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, goto cleanup; }
- partsep = virXMLPropString(nodeset[i], "part_separator"); - if (partsep) { - int value = virTristateBoolTypeFromString(partsep); - if (value <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid part_separator setting '%s'"), - partsep); - virStoragePoolSourceDeviceClear(&dev); - goto cleanup; - } - dev.part_separator = value; + if (virXMLPropTristateBool(nodeset[i], "part_separator", + VIR_XML_PROP_NONE, + &dev.part_separator) < 0) { + virStoragePoolSourceDeviceClear(&dev); + goto cleanup; }
if (VIR_APPEND_ELEMENT(source->devices, source->ndevice, dev) < 0) {
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/storage_conf.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 435a029b4e..10b46ac368 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -523,8 +523,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, int pool_type, xmlNodePtr node) { - int ret = -1; - xmlNodePtr relnode, authnode; + xmlNodePtr authnode; xmlNodePtr adapternode; int nsource; size_t i; @@ -534,18 +533,18 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, g_autofree char *ver = NULL; g_autofree xmlNodePtr *nodeset = NULL; g_autofree char *sourcedir = NULL; + VIR_XPATH_NODE_AUTORESTORE(ctxt) - relnode = ctxt->node; ctxt->node = node; if ((options = virStoragePoolOptionsForPoolType(pool_type)) == NULL) - goto cleanup; + return -1; source->name = virXPathString("string(./name)", ctxt); if (pool_type == VIR_STORAGE_POOL_RBD && source->name == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("element 'name' is mandatory for RBD pool")); - goto cleanup; + return -1; } if (options->formatFromString) { @@ -560,12 +559,12 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if (source->format < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown pool format type %s"), format); - goto cleanup; + return -1; } } if ((n = virXPathNodeSet("./host", ctxt, &nodeset)) < 0) - goto cleanup; + return -1; if (n) { source->hosts = g_new0(virStoragePoolSourceHost, n); @@ -576,12 +575,12 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if (!source->hosts[i].name) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool host name")); - goto cleanup; + return -1; } if (virXMLPropInt(nodeset[i], "port", 10, VIR_XML_PROP_NONE, &source->hosts[i].port, 0) < 0) - goto cleanup; + return -1; } } @@ -591,7 +590,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, nsource = virXPathNodeSet("./device", ctxt, &nodeset); if (nsource < 0) - goto cleanup; + return -1; for (i = 0; i < nsource; i++) { virStoragePoolSourceDevice dev = { .path = NULL }; @@ -600,19 +599,19 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if (dev.path == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool source device path")); - goto cleanup; + return -1; } if (virXMLPropTristateBool(nodeset[i], "part_separator", VIR_XML_PROP_NONE, &dev.part_separator) < 0) { virStoragePoolSourceDeviceClear(&dev); - goto cleanup; + return -1; } if (VIR_APPEND_ELEMENT(source->devices, source->ndevice, dev) < 0) { virStoragePoolSourceDeviceClear(&dev); - goto cleanup; + return -1; } } @@ -626,17 +625,17 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, if ((adapternode = virXPathNode("./adapter", ctxt))) { if (virStorageAdapterParseXML(&source->adapter, adapternode, ctxt) < 0) - goto cleanup; + return -1; } if ((authnode = virXPathNode("./auth", ctxt))) { if (!(authdef = virStorageAuthDefParse(authnode, ctxt))) - goto cleanup; + return -1; if (authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("storage pool missing auth type")); - goto cleanup; + return -1; } source->auth = g_steal_pointer(&authdef); @@ -650,24 +649,20 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, _("storage pool protocol ver unsupported for " "pool type '%s'"), virStoragePoolFormatFileSystemNetTypeToString(source->format)); - goto cleanup; + return -1; } if (virStrToLong_uip(ver, NULL, 0, &source->protocolVer) < 0) { virReportError(VIR_ERR_XML_ERROR, _("storage pool protocol ver '%s' is malformed"), ver); - goto cleanup; + return -1; } } source->vendor = virXPathString("string(./vendor/@name)", ctxt); source->product = virXPathString("string(./product/@name)", ctxt); - ret = 0; - cleanup: - ctxt->node = relnode; - - return ret; + return 0; } -- 2.26.3
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/storage_conf.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 10b46ac368..e481cac75c 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -841,24 +841,17 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) virStoragePoolOptions *options; xmlNodePtr source_node; g_autoptr(virStoragePoolDef) def = NULL; - g_autofree char *type = NULL; + virStoragePoolType type; g_autofree char *uuid = NULL; g_autofree char *target_path = NULL; def = g_new0(virStoragePoolDef, 1); - type = virXPathString("string(./@type)", ctxt); - if (type == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("storage pool missing type attribute")); + if (virXMLPropEnum(ctxt->node, "type", virStoragePoolTypeFromString, + VIR_XML_PROP_REQUIRED, &type) < 0) return NULL; - } - if ((def->type = virStoragePoolTypeFromString(type)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage pool type %s"), type); - return NULL; - } + def->type = type; if ((options = virStoragePoolOptionsForPoolType(def->type)) == NULL) return NULL; -- 2.26.3
On 5/11/21 11:01 AM, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Laine Stump <laine@redhat.com>
--- src/conf/storage_conf.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 10b46ac368..e481cac75c 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -841,24 +841,17 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) virStoragePoolOptions *options; xmlNodePtr source_node; g_autoptr(virStoragePoolDef) def = NULL; - g_autofree char *type = NULL; + virStoragePoolType type; g_autofree char *uuid = NULL; g_autofree char *target_path = NULL;
def = g_new0(virStoragePoolDef, 1);
- type = virXPathString("string(./@type)", ctxt); - if (type == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("storage pool missing type attribute")); + if (virXMLPropEnum(ctxt->node, "type", virStoragePoolTypeFromString, + VIR_XML_PROP_REQUIRED, &type) < 0) return NULL; - }
- if ((def->type = virStoragePoolTypeFromString(type)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown storage pool type %s"), type); - return NULL; - } + def->type = type;
if ((options = virStoragePoolOptionsForPoolType(def->type)) == NULL) return NULL;
On 5/11/21 5:01 PM, Tim Wiederhake wrote:
For background, see https://listman.redhat.com/archives/libvir-list/2021-April/msg00668.html
Tim Wiederhake (10): virNodeDeviceDefParseXML: Use g_auto* virDomainNumatuneNodeParseXML: Use virXMLProp* virDomainNumatuneNodeParseXML: Use g_autofree virDomainNumaDefNodeDistanceParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use virXMLProp* virDomainNumaDefParseXML: Use g_autofree virStorageAdapterParseXMLFCHost: Use virXMLProp* virStoragePoolDefParseSource: Use virXMLProp* virStoragePoolDefParseSource: Use VIR_XPATH_NODE_AUTORESTORE virStoragePoolDefParseXML: Use virXMLProp*
src/conf/node_device_conf.c | 34 +-- src/conf/numa_conf.c | 285 +++++------------- src/conf/storage_adapter_conf.c | 17 +- src/conf/storage_conf.c | 83 ++--- .../hugepages-memaccess-invalid.err | 2 +- 5 files changed, 125 insertions(+), 296 deletions(-)
I've pushed reviewed patches (07-10/10). Michal
participants (3)
-
Laine Stump -
Michal Prívozník -
Tim Wiederhake