[PATCH 0/5] Refactor few old-style XML node lookup loops
Peter Krempa (5): virNetworkDNSHostDefParseXML: Refactor parsing virsh: domain: Refactor XML handling for disk changes virDomainFeaturesCapabilitiesDefParse: Use virXMLNodeGetSubelementList virDomainFeaturesKVMDefParse: Use virXMLNodeGetSubelementList virDomainFeaturesXENDefParse: Use virXMLNodeGetSubelementList src/conf/domain_conf.c | 50 +++++++++++----------- src/conf/network_conf.c | 94 ++++++++++++++++++++--------------------- tools/virsh-domain.c | 63 +++------------------------ 3 files changed, 78 insertions(+), 129 deletions(-) -- 2.41.0
Use 'virXMLNodeGetSubelementList' instead of looping through XML nodes and modernize the code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/network_conf.c | 94 ++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 48 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index e4c8c5fd4d..9dd6fb4ce9 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -656,63 +656,61 @@ virNetworkDNSHostDefParseXML(const char *networkName, virNetworkDNSHostDef *def, bool partialOkay) { - xmlNodePtr cur; - g_autofree char *ip = NULL; - - if (!(ip = virXMLPropString(node, "ip")) && !partialOkay) { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing IP address in network '%1$s' DNS HOST record"), - networkName); - goto error; - } - - if (ip && (virSocketAddrParse(&def->ip, ip, AF_UNSPEC) < 0)) { - virReportError(VIR_ERR_XML_DETAIL, - _("Invalid IP address in network '%1$s' DNS HOST record"), - networkName); - goto error; - } + g_autofree xmlNodePtr *hostnameNodes = NULL; + size_t nhostnameNodes = virXMLNodeGetSubelementList(node, "hostname", &hostnameNodes); + size_t i; + g_auto(GStrv) hostnames = NULL; + g_autofree char *ip = virXMLPropString(node, "ip"); - cur = node->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "hostname")) { - if (cur->children != NULL) { - g_autofree char *name = virXMLNodeContentString(cur); + if (nhostnameNodes > 0) { + hostnames = g_new0(char *, nhostnameNodes + 1); - if (!name) - goto error; + for (i = 0; i < nhostnameNodes; i++) { + if (!(hostnames[i] = virXMLNodeContentString(hostnameNodes[i]))) + return -1; - if (!name[0]) { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing hostname in network '%1$s' DNS HOST record"), - networkName); - goto error; - } - VIR_APPEND_ELEMENT(def->names, def->nnames, name); - } + if (*hostnames[i] == '\0') { + virReportError(VIR_ERR_XML_DETAIL, + _("Missing hostname in network '%1$s' DNS HOST record"), + networkName); + return -1; + } + } + } else { + if (!partialOkay) { + virReportError(VIR_ERR_XML_DETAIL, + _("Missing hostname in network '%1$s' DNS HOST record"), + networkName); + return -1; } - cur = cur->next; - } - if (def->nnames == 0 && !partialOkay) { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing hostname in network '%1$s' DNS HOST record"), - networkName); - goto error; } - if (!VIR_SOCKET_ADDR_VALID(&def->ip) && def->nnames == 0) { - virReportError(VIR_ERR_XML_DETAIL, - _("Missing ip and hostname in network '%1$s' DNS HOST record"), - networkName); - goto error; + if (ip) { + if (virSocketAddrParse(&def->ip, ip, AF_UNSPEC) < 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("Invalid IP address in network '%1$s' DNS HOST record"), + networkName); + return -1; + } + } else { + if (!partialOkay) { + virReportError(VIR_ERR_XML_DETAIL, + _("Missing IP address in network '%1$s' DNS HOST record"), + networkName); + return -1; + } + + if (nhostnameNodes == 0) { + virReportError(VIR_ERR_XML_DETAIL, + _("Missing ip and hostname in network '%1$s' DNS HOST record"), + networkName); + return -1; + } } + def->names = g_steal_pointer(&hostnames); + def->nnames = nhostnameNodes; return 0; - - error: - virNetworkDNSHostDefClear(def); - return -1; } -- 2.41.0
Use virXMLNodeGetSubelement to find needed subelements. In virshUpdateDiskXML this commit removes the code which keeps XML formatting tidy, but that is not needed for the code to format proper XMLs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 63 +++++--------------------------------------- 1 file changed, 7 insertions(+), 56 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index e0776c991f..541a799aaf 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12641,19 +12641,13 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) static void virshDiskDropBackingStore(xmlNodePtr disk_node) { - xmlNodePtr tmp; + xmlNodePtr tmp = virXMLNodeGetSubelement(disk_node, "backingStore"); - for (tmp = disk_node->children; tmp; tmp = tmp->next) { - if (tmp->type != XML_ELEMENT_NODE) - continue; - - if (virXMLNodeNameEqual(tmp, "backingStore")) { - xmlUnlinkNode(tmp); - xmlFreeNode(tmp); + if (!tmp) + return; - return; - } - } + xmlUnlinkNode(tmp); + xmlFreeNode(tmp); } @@ -12753,10 +12747,7 @@ virshUpdateDiskXML(xmlNodePtr disk_node, const char *target, virshUpdateDiskXMLType type) { - xmlNodePtr tmp = NULL; xmlNodePtr source = NULL; - xmlNodePtr target_node = NULL; - xmlNodePtr text_node = NULL; g_autofree char *device_type = NULL; char *ret = NULL; g_autofree char *startupPolicy = NULL; @@ -12773,33 +12764,7 @@ virshUpdateDiskXML(xmlNodePtr disk_node, return NULL; } - /* find the current source subelement */ - for (tmp = disk_node->children; tmp; tmp = tmp->next) { - /* - * Save the last text node before the <target/>. The - * reasoning behind this is that the target node will be - * present in this case and also has a proper indentation. - */ - if (!target_node && tmp->type == XML_TEXT_NODE) - text_node = tmp; - - /* - * We need only element nodes from now on. - */ - if (tmp->type != XML_ELEMENT_NODE) - continue; - - if (!source && virXMLNodeNameEqual(tmp, "source")) - source = tmp; - else if (!target_node && virXMLNodeNameEqual(tmp, "target")) - target_node = tmp; - - /* - * We've found all we needed. - */ - if (source && target_node) - break; - } + source = virXMLNodeGetSubelement(disk_node, "source"); if (type == VIRSH_UPDATE_DISK_XML_EJECT) { if (!source) { @@ -12852,21 +12817,7 @@ virshUpdateDiskXML(xmlNodePtr disk_node, if (startupPolicy) xmlNewProp(source, BAD_CAST "startupPolicy", BAD_CAST startupPolicy); - /* - * So that the output XML looks nice in case anyone calls - * 'change-media' with '--print-xml', let's attach the source - * before target... - */ - xmlAddPrevSibling(target_node, source); - - /* - * ... and duplicate the text node doing the indentation just - * so it's more easily readable. And don't make it fatal. - */ - if ((tmp = xmlCopyNode(text_node, 0))) { - if (!xmlAddPrevSibling(target_node, tmp)) - xmlFreeNode(tmp); - } + xmlAddChild(disk_node, source); } if (!(ret = virXMLNodeToString(NULL, disk_node))) { -- 2.41.0
Rewrite the old-style parser to use virXMLNodeGetSubelementList Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index bb4f1fdb94..d510279472 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16462,7 +16462,10 @@ static int virDomainFeaturesCapabilitiesDefParse(virDomainDef *def, xmlNodePtr node) { + g_autofree xmlNodePtr *caps = NULL; + size_t ncaps = virXMLNodeGetSubelementList(node, NULL, &caps); virDomainCapabilitiesPolicy policy; + size_t i; if (virXMLPropEnumDefault(node, "policy", virDomainCapabilitiesPolicyTypeFromString, @@ -16472,25 +16475,23 @@ virDomainFeaturesCapabilitiesDefParse(virDomainDef *def, def->features[VIR_DOMAIN_FEATURE_CAPABILITIES] = policy; - node = xmlFirstElementChild(node); - while (node) { + for (i = 0; i < ncaps; i++) { virTristateSwitch state; - int val = virDomainProcessCapsFeatureTypeFromString((const char *)node->name); + int val = virDomainProcessCapsFeatureTypeFromString((const char *)caps[i]->name); if (val < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unexpected capability feature '%1$s'"), node->name); + _("unexpected capability feature '%1$s'"), caps[i]->name); return -1; } - if (virXMLPropTristateSwitch(node, "state", VIR_XML_PROP_NONE, &state) < 0) + if (virXMLPropTristateSwitch(caps[i], "state", VIR_XML_PROP_NONE, &state) < 0) return -1; if (state == VIR_TRISTATE_SWITCH_ABSENT) state = VIR_TRISTATE_SWITCH_ON; def->caps_features[val] = state; - node = xmlNextElementSibling(node); } return 0; -- 2.41.0
Rewrite the old-style parser to use virXMLNodeGetSubelementList Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d510279472..3a08034b9d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16355,24 +16355,24 @@ static int virDomainFeaturesKVMDefParse(virDomainDef *def, xmlNodePtr node) { - g_autofree virDomainFeatureKVM *kvm = NULL; - - kvm = g_new0(virDomainFeatureKVM, 1); + g_autofree virDomainFeatureKVM *kvm = g_new0(virDomainFeatureKVM, 1); + g_autofree xmlNodePtr *feats = NULL; + size_t nfeats = virXMLNodeGetSubelementList(node, NULL, &feats); + size_t i; - node = xmlFirstElementChild(node); - while (node) { + for (i = 0; i < nfeats; i++) { int feature; virTristateSwitch value; - feature = virDomainKVMTypeFromString((const char *)node->name); + feature = virDomainKVMTypeFromString((const char *)feats[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported KVM feature: %1$s"), - node->name); + feats[i]->name); return -1; } - if (virXMLPropTristateSwitch(node, "state", VIR_XML_PROP_REQUIRED, + if (virXMLPropTristateSwitch(feats[i], "state", VIR_XML_PROP_REQUIRED, &value) < 0) return -1; @@ -16382,7 +16382,7 @@ virDomainFeaturesKVMDefParse(virDomainDef *def, if (feature == VIR_DOMAIN_KVM_DIRTY_RING && value == VIR_TRISTATE_SWITCH_ON) { - if (virXMLPropUInt(node, "size", 0, VIR_XML_PROP_REQUIRED, + if (virXMLPropUInt(feats[i], "size", 0, VIR_XML_PROP_REQUIRED, &kvm->dirty_ring_size) < 0) { return -1; } @@ -16396,8 +16396,6 @@ virDomainFeaturesKVMDefParse(virDomainDef *def, return -1; } } - - node = xmlNextElementSibling(node); } def->features[VIR_DOMAIN_FEATURE_KVM] = VIR_TRISTATE_SWITCH_ON; -- 2.41.0
Rewrite the old-style parser to use virXMLNodeGetSubelementList Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3a08034b9d..02bba77768 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16409,22 +16409,25 @@ static int virDomainFeaturesXENDefParse(virDomainDef *def, xmlNodePtr node) { + g_autofree xmlNodePtr *feats = NULL; + size_t nfeats = virXMLNodeGetSubelementList(node, NULL, &feats); + size_t i; + def->features[VIR_DOMAIN_FEATURE_XEN] = VIR_TRISTATE_SWITCH_ON; - node = xmlFirstElementChild(node); - while (node) { + for (i = 0; i < nfeats; i++) { int feature; virTristateSwitch value; - feature = virDomainXenTypeFromString((const char *)node->name); + feature = virDomainXenTypeFromString((const char *)feats[i]->name); if (feature < 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported Xen feature: %1$s"), - node->name); + feats[i]->name); return -1; } - if (virXMLPropTristateSwitch(node, "state", + if (virXMLPropTristateSwitch(feats[i], "state", VIR_XML_PROP_REQUIRED, &value) < 0) return -1; @@ -16438,7 +16441,7 @@ virDomainFeaturesXENDefParse(virDomainDef *def, if (value != VIR_TRISTATE_SWITCH_ON) break; - if (virXMLPropEnum(node, "mode", + if (virXMLPropEnum(feats[i], "mode", virDomainXenPassthroughModeTypeFromString, VIR_XML_PROP_NONZERO, &def->xen_passthrough_mode) < 0) @@ -16448,8 +16451,6 @@ virDomainFeaturesXENDefParse(virDomainDef *def, case VIR_DOMAIN_XEN_LAST: break; } - - node = xmlNextElementSibling(node); } return 0; -- 2.41.0
On 8/28/23 15:57, Peter Krempa wrote:
Peter Krempa (5): virNetworkDNSHostDefParseXML: Refactor parsing virsh: domain: Refactor XML handling for disk changes virDomainFeaturesCapabilitiesDefParse: Use virXMLNodeGetSubelementList virDomainFeaturesKVMDefParse: Use virXMLNodeGetSubelementList virDomainFeaturesXENDefParse: Use virXMLNodeGetSubelementList
src/conf/domain_conf.c | 50 +++++++++++----------- src/conf/network_conf.c | 94 ++++++++++++++++++++--------------------- tools/virsh-domain.c | 63 +++------------------------ 3 files changed, 78 insertions(+), 129 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa