[PATCH 00/13] virsh: Don't reimplement virXPathNodeSet
This series converts open-coded virXPathNodeSet to the proper helper use and also adds --print-xml option to commands using xmlXpathEval which didn't have it. Peter Krempa (13): virsh: Add --print-xml option for 'detach-interface' virshDomainDetachInterface: Use virXPathNodeSet instead of xmlXpathEval virsh: Add --print-xml option for 'domif-setlink' virsh: cmdDomIfSetLink: Use virXPathNodeSet instead of xmlXpathEval virsh: Refactor cleanup in 'cmdVolClone' virsh: Add --print-xml flag for 'vol-clone' command virsh: virshMakeCloneXML: Use virXPathNode instead of xmlXPathEval virsh: cmdDetachDisk: Refactor cleanup virsh: cmdChangeMedia: Refactor cleanup virshFindDisk: Use virXPathNodeSet instead of xmlXPathEval virshFindDisk: Sanitize removable media check util: xml: Introduce virXMLNodeGetSubelement virshFindDisk: Sanitize use of 'tmp' variable docs/manpages/virsh.rst | 15 +- src/libvirt_private.syms | 1 + src/util/virxml.c | 23 +++ src/util/virxml.h | 5 + tools/virsh-domain.c | 298 ++++++++++++++++++--------------------- tools/virsh-volume.c | 61 ++++---- 6 files changed, 207 insertions(+), 196 deletions(-) -- 2.38.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 5 ++++- tools/virsh-domain.c | 21 ++++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 1e8a43bc55..09fc1f67ad 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5090,7 +5090,7 @@ detach-interface :: detach-interface domain type [--mac mac] - [[[--live] [--config] | [--current]] | [--persistent]] + [[[--live] [--config] | [--current]] | [--persistent]] [--print-xml] Detach a network interface from a domain. *type* can be either *network* to indicate a physical network device or @@ -5112,6 +5112,9 @@ an offline domain, and like *--live* *--config* for a running domain. Note that older versions of virsh used *--config* as an alias for *--persistent*. +If *--print-xml* is specified, then the XML used to detach the interface +is printed instead. + Please see documentation for ``detach-device`` for known quirks. diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2d22547cc6..9574a6eab6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12454,6 +12454,10 @@ static const vshCmdOptDef opts_detach_interface[] = { VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = "print-xml", + .type = VSH_OT_BOOL, + .help = N_("print XML document rather than detach the interface") + }, {.name = NULL} }; @@ -12464,7 +12468,8 @@ virshDomainDetachInterface(char *doc, vshControl *ctl, bool current, const char *type, - const char *mac) + const char *mac, + bool printxml) { g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathObject) obj = NULL; @@ -12533,6 +12538,11 @@ virshDomainDetachInterface(char *doc, return false; } + if (printxml) { + vshPrint(ctl, "%s", detach_xml); + return true; + } + if (flags != 0 || current) return virDomainDetachDeviceFlags(dom, detach_xml, flags) == 0; return virDomainDetachDevice(dom, detach_xml) == 0; @@ -12552,6 +12562,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool persistent = vshCommandOptBool(cmd, "persistent"); + bool printxml = vshCommandOptBool(cmd, "print-xml"); VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current); @@ -12574,7 +12585,8 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; if (!(ret = virshDomainDetachInterface(doc_config, flags | VIR_DOMAIN_AFFECT_CONFIG, - dom, ctl, current, type, mac))) + dom, ctl, current, type, mac, + printxml))) goto cleanup; } @@ -12590,9 +12602,12 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; ret = virshDomainDetachInterface(doc_live, flags, - dom, ctl, current, type, mac); + dom, ctl, current, type, mac, printxml); } + if (printxml) + return true; + cleanup: if (!ret) { vshError(ctl, "%s", _("Failed to detach interface")); -- 2.38.1
On a Friday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 5 ++++- tools/virsh-domain.c | 21 ++++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 1e8a43bc55..09fc1f67ad 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -5090,7 +5090,7 @@ detach-interface ::
detach-interface domain type [--mac mac] - [[[--live] [--config] | [--current]] | [--persistent]] + [[[--live] [--config] | [--current]] | [--persistent]] [--print-xml]
Detach a network interface from a domain. *type* can be either *network* to indicate a physical network device or @@ -5112,6 +5112,9 @@ an offline domain, and like *--live* *--config* for a running domain. Note that older versions of virsh used *--config* as an alias for *--persistent*.
+If *--print-xml* is specified, then the XML used to detach the interface +is printed instead. + Please see documentation for ``detach-device`` for known quirks.
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2d22547cc6..9574a6eab6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12454,6 +12454,10 @@ static const vshCmdOptDef opts_detach_interface[] = { VIRSH_COMMON_OPT_DOMAIN_CONFIG, VIRSH_COMMON_OPT_DOMAIN_LIVE, VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = "print-xml", + .type = VSH_OT_BOOL, + .help = N_("print XML document rather than detach the interface") + }, {.name = NULL} };
@@ -12464,7 +12468,8 @@ virshDomainDetachInterface(char *doc, vshControl *ctl, bool current, const char *type, - const char *mac) + const char *mac, + bool printxml) { g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathObject) obj = NULL; @@ -12533,6 +12538,11 @@ virshDomainDetachInterface(char *doc, return false; }
+ if (printxml) { + vshPrint(ctl, "%s", detach_xml); + return true; + } + if (flags != 0 || current) return virDomainDetachDeviceFlags(dom, detach_xml, flags) == 0; return virDomainDetachDevice(dom, detach_xml) == 0; @@ -12552,6 +12562,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); bool persistent = vshCommandOptBool(cmd, "persistent"); + bool printxml = vshCommandOptBool(cmd, "print-xml");
VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
@@ -12574,7 +12585,8 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup; if (!(ret = virshDomainDetachInterface(doc_config, flags | VIR_DOMAIN_AFFECT_CONFIG, - dom, ctl, current, type, mac))) + dom, ctl, current, type, mac, + printxml))) goto cleanup; }
@@ -12590,9 +12602,12 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) goto cleanup;
ret = virshDomainDetachInterface(doc_live, flags, - dom, ctl, current, type, mac); + dom, ctl, current, type, mac, printxml); }
+ if (printxml) + return true;
virshDomainDetachInterface can still fail. s/true/ret/ Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Refactor the XPath lookup to use the internal helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 52 ++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9574a6eab6..87de3a708a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12472,12 +12472,12 @@ virshDomainDetachInterface(char *doc, bool printxml) { g_autoptr(xmlDoc) xml = NULL; - g_autoptr(xmlXPathObject) obj = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - xmlNodePtr cur = NULL, matchNode = NULL; g_autofree char *detach_xml = NULL; - char buf[64]; - int diff_mac = -1; + g_autofree char *xpath = g_strdup_printf("/domain/devices/interface[@type='%s']", type); + g_autofree xmlNodePtr *nodes = NULL; + ssize_t nnodes; + xmlNodePtr matchNode = NULL; size_t i; if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) { @@ -12485,34 +12485,20 @@ virshDomainDetachInterface(char *doc, return false; } - g_snprintf(buf, sizeof(buf), "/domain/devices/interface[@type='%s']", type); - obj = xmlXPathEval(BAD_CAST buf, ctxt); - if (obj == NULL || obj->type != XPATH_NODESET || - obj->nodesetval == NULL || obj->nodesetval->nodeNr == 0) { + if ((nnodes = virXPathNodeSet(xpath, ctxt, &nodes)) <= 0) { vshError(ctl, _("No interface found whose type is %s"), type); return false; } - if (!mac && obj->nodesetval->nodeNr > 1) { - vshError(ctl, _("Domain has %d interfaces. Please specify which one " - "to detach using --mac"), obj->nodesetval->nodeNr); - return false; - } + if (mac) { + for (i = 0; i < nnodes; i++) { + g_autofree char *tmp_mac = NULL; - if (!mac) { - matchNode = obj->nodesetval->nodeTab[0]; - goto hit; - } + ctxt->node = nodes[i]; - /* multiple possibilities, so search for matching mac */ - for (i = 0; i < obj->nodesetval->nodeNr; i++) { - cur = obj->nodesetval->nodeTab[i]->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "mac")) { - g_autofree char *tmp_mac = virXMLPropString(cur, "address"); - diff_mac = virMacAddrCompare(tmp_mac, mac); - if (!diff_mac) { + if ((tmp_mac = virXPathString("string(./mac/@address)", ctxt))) { + + if (virMacAddrCompare(tmp_mac, mac) == 0) { if (matchNode) { /* this is the 2nd match, so it's ambiguous */ vshError(ctl, _("Domain has multiple interfaces matching " @@ -12521,18 +12507,26 @@ virshDomainDetachInterface(char *doc, mac); return false; } - matchNode = obj->nodesetval->nodeTab[i]; + + matchNode = nodes[i]; } } - cur = cur->next; } + } else { + if (nnodes > 1) { + vshError(ctl, _("Domain has %zd interfaces. Please specify which one to detach using --mac"), + nnodes); + return false; + } + + matchNode = nodes[0]; } + if (!matchNode) { vshError(ctl, _("No interface with MAC address %s was found"), mac); return false; } - hit: if (!(detach_xml = virXMLNodeToString(xml, matchNode))) { vshSaveLibvirtError(); return false; -- 2.38.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 5 ++++- tools/virsh-domain.c | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 09fc1f67ad..14ed9cd5a0 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1890,7 +1890,7 @@ domif-setlink :: - domif-setlink domain interface-device state [--config] + domif-setlink domain interface-device state [--config] [--print-xml] Modify link state of the domain's virtual interface. Possible values for state are "up" and "down". If *--config* is specified, only the persistent @@ -1898,6 +1898,9 @@ configuration of the domain is modified, for compatibility purposes, *--persistent* is alias of *--config*. *interface-device* can be the interface's target name or the MAC address. +If *--print-xml* is specified, then the XML used to update the interface is +printed instead. + domifaddr --------- diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 87de3a708a..9baab2672a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3120,6 +3120,10 @@ static const vshCmdOptDef opts_domif_setlink[] = { .help = "config" }, VIRSH_COMMON_OPT_DOMAIN_CONFIG, + {.name = "print-xml", + .type = VSH_OT_BOOL, + .help = N_("print XML document rather than set the interface link state") + }, {.name = NULL} }; @@ -3238,6 +3242,11 @@ cmdDomIfSetLink(vshControl *ctl, const vshCmd *cmd) return false; } + if (vshCommandOptBool(cmd, "print-xml")) { + vshPrint(ctl, "%s", xml_buf); + return true; + } + if (virDomainUpdateDeviceFlags(dom, xml_buf, flags) < 0) { vshError(ctl, _("Failed to update interface link state")); return false; -- 2.38.1
Refactor the XPath lookup to use the internal helper. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 93 ++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 55 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9baab2672a..2fb1751f25 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3133,18 +3133,17 @@ cmdDomIfSetLink(vshControl *ctl, const vshCmd *cmd) g_autoptr(virshDomain) dom = NULL; const char *iface; const char *state; - virMacAddr macaddr; - const char *element; - const char *attr; - bool config; unsigned int flags = 0; unsigned int xmlflags = 0; size_t i; g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - g_autoptr(xmlXPathObject) obj = NULL; - xmlNodePtr cur = NULL; g_autofree char *xml_buf = NULL; + g_autofree xmlNodePtr *nodes = NULL; + ssize_t nnodes; + xmlNodePtr ifaceNode = NULL; + xmlNodePtr linkNode = NULL; + xmlAttrPtr stateAttr; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; @@ -3153,14 +3152,12 @@ cmdDomIfSetLink(vshControl *ctl, const vshCmd *cmd) vshCommandOptStringReq(ctl, cmd, "state", &state) < 0) return false; - config = vshCommandOptBool(cmd, "config"); - if (STRNEQ(state, "up") && STRNEQ(state, "down")) { vshError(ctl, _("invalid link state '%s'"), state); return false; } - if (config) { + if (vshCommandOptBool(cmd, "config")) { flags = VIR_DOMAIN_AFFECT_CONFIG; xmlflags |= VIR_DOMAIN_XML_INACTIVE; } else { @@ -3173,70 +3170,56 @@ cmdDomIfSetLink(vshControl *ctl, const vshCmd *cmd) if (virshDomainGetXMLFromDom(ctl, dom, xmlflags, &xml, &ctxt) < 0) return false; - obj = xmlXPathEval(BAD_CAST "/domain/devices/interface", ctxt); - if (obj == NULL || obj->type != XPATH_NODESET || - obj->nodesetval == NULL || obj->nodesetval->nodeNr == 0) { + if ((nnodes = virXPathNodeSet("/domain/devices/interface", ctxt, &nodes)) <= 0) { vshError(ctl, _("Failed to extract interface information or no interfaces found")); return false; } - if (virMacAddrParse(iface, &macaddr) == 0) { - element = "mac"; - attr = "address"; - } else { - element = "target"; - attr = "dev"; - } + for (i = 0; i < nnodes; i++) { + g_autofree char *macaddr = NULL; + g_autofree char *target = NULL; - /* find interface with matching mac addr */ - for (i = 0; i < obj->nodesetval->nodeNr; i++) { - cur = obj->nodesetval->nodeTab[i]->children; + ctxt->node = nodes[i]; - while (cur) { - if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, element)) { - g_autofree char *value = virXMLPropString(cur, attr); + if ((macaddr = virXPathString("string(./mac/@address)", ctxt)) && + STRCASEEQ(macaddr, iface)) { + ifaceNode = nodes[i]; + break; + } - if (STRCASEEQ(value, iface)) - goto hit; - } - cur = cur->next; + if ((target = virXPathString("string(./target/@dev)", ctxt)) && + STRCASEEQ(target, iface)) { + ifaceNode = nodes[i]; + break; } } - vshError(ctl, _("interface (%s: %s) not found"), element, iface); - return false; - - hit: - /* find and modify/add link state node */ - /* try to find <link> element */ - cur = obj->nodesetval->nodeTab[i]->children; + if (!ifaceNode) { + vshError(ctl, _("interface '%s' not found"), iface); + return false; + } - while (cur) { - if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "link")) { - /* found, just modify the property */ - xmlSetProp(cur, BAD_CAST "state", BAD_CAST state); + ctxt->node = ifaceNode; - break; + /* try to find <link> element or create new one */ + if (!(linkNode = virXPathNode("./link", ctxt))) { + if (!(linkNode = xmlNewChild(ifaceNode, NULL, BAD_CAST "link", NULL))) { + vshError(ctl, _("failed to create XML node")); + return false; } - cur = cur->next; } - if (!cur) { - /* element <link> not found, add one */ - cur = xmlNewChild(obj->nodesetval->nodeTab[i], - NULL, - BAD_CAST "link", - NULL); - if (!cur) - return false; + if (xmlHasProp(linkNode, BAD_CAST "link")) + stateAttr = xmlSetProp(linkNode, BAD_CAST "state", BAD_CAST state); + else + stateAttr = xmlNewProp(linkNode, BAD_CAST "state", BAD_CAST state); - if (xmlNewProp(cur, BAD_CAST "state", BAD_CAST state) == NULL) - return false; + if (!stateAttr) { + vshError(ctl, _("Failed to create or modify the state XML attribute")); + return false; } - if (!(xml_buf = virXMLNodeToString(xml, obj->nodesetval->nodeTab[i]))) { + if (!(xml_buf = virXMLNodeToString(xml, ifaceNode))) { vshSaveLibvirtError(); vshError(ctl, _("Failed to create XML")); return false; -- 2.38.1
Automatically free 'newxml' and remove the 'cleanup' label and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-volume.c | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 4f23481180..b7a1ddb9e5 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -570,12 +570,11 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd) g_autoptr(virshStorageVol) newvol = NULL; const char *name = NULL; g_autofree char *origxml = NULL; - xmlChar *newxml = NULL; - bool ret = false; + g_autofree xmlChar *newxml = NULL; unsigned int flags = 0; if (!(origvol = virshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) - goto cleanup; + return false; if (vshCommandOptBool(cmd, "prealloc-metadata")) flags |= VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA; @@ -586,38 +585,30 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd) origpool = virStoragePoolLookupByVolume(origvol); if (!origpool) { vshError(ctl, "%s", _("failed to get parent pool")); - goto cleanup; + return false; } if (vshCommandOptStringReq(ctl, cmd, "newname", &name) < 0) - goto cleanup; + return false; - origxml = virStorageVolGetXMLDesc(origvol, 0); - if (!origxml) - goto cleanup; + if (!(origxml = virStorageVolGetXMLDesc(origvol, 0))) + return false; - newxml = virshMakeCloneXML(origxml, name); - if (!newxml) { + if (!(newxml = virshMakeCloneXML(origxml, name))) { vshError(ctl, "%s", _("Failed to allocate XML buffer")); - goto cleanup; + return false; } - newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, origvol, flags); - - if (newvol != NULL) { - vshPrintExtra(ctl, _("Vol %s cloned from %s\n"), - virStorageVolGetName(newvol), virStorageVolGetName(origvol)); - } else { + if (!(newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, + origvol, flags))) { vshError(ctl, _("Failed to clone vol from %s"), virStorageVolGetName(origvol)); - goto cleanup; + return false; } - ret = true; - - cleanup: - xmlFree(newxml); - return ret; + vshPrintExtra(ctl, _("Vol %s cloned from %s\n"), + virStorageVolGetName(newvol), virStorageVolGetName(origvol)); + return true; } /* -- 2.38.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 5 ++++- tools/virsh-volume.c | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 14ed9cd5a0..c85bc8151d 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -6719,7 +6719,7 @@ vol-clone :: vol-clone vol-name-or-key-or-path name - [--pool pool-or-uuid] [--prealloc-metadata] [--reflink] + [--pool pool-or-uuid] [--prealloc-metadata] [--reflink] [--print-xml] Clone an existing volume within the parent pool. Less powerful, but easier to type, version of ``vol-create-from``. @@ -6743,6 +6743,9 @@ When *--reflink* is specified, perform a COW lightweight copy, where the data blocks are copied only when modified. If this is not possible, the copy fails. +If *--print-xml* is specified, then the XML used to clone the volume is +printed instead. + vol-delete ---------- diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index b7a1ddb9e5..adfea570be 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -559,6 +559,10 @@ static const vshCmdOptDef opts_vol_clone[] = { .type = VSH_OT_BOOL, .help = N_("use btrfs COW lightweight copy") }, + {.name = "print-xml", + .type = VSH_OT_BOOL, + .help = N_("print XML document rather than clone the volume") + }, {.name = NULL} }; @@ -599,6 +603,11 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd) return false; } + if (vshCommandOptBool(cmd, "print-xml")) { + vshPrint(ctl, "%s", newxml); + return true; + } + if (!(newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, origvol, flags))) { vshError(ctl, _("Failed to clone vol from %s"), -- 2.38.1
Refactor the code to use the XPath helpers instead of open-coding them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-volume.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index adfea570be..2a7809140e 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -505,28 +505,22 @@ cmdVolCreateFrom(vshControl *ctl, const vshCmd *cmd) return true; } -static xmlChar * +static char * virshMakeCloneXML(const char *origxml, const char *newname) { g_autoptr(xmlDoc) doc = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - g_autoptr(xmlXPathObject) obj = NULL; - xmlChar *newxml = NULL; - int size; + xmlNodePtr node; - doc = virXMLParseStringCtxt(origxml, _("(volume_definition)"), &ctxt); - if (!doc) + if (!(doc = virXMLParseStringCtxt(origxml, _("(volume_definition)"), &ctxt))) return NULL; - obj = xmlXPathEval(BAD_CAST "/volume/name", ctxt); - if (obj == NULL || obj->nodesetval == NULL || - obj->nodesetval->nodeTab == NULL) + if (!(node = virXPathNode("/volume/name", ctxt))) return NULL; - xmlNodeSetContent(obj->nodesetval->nodeTab[0], (const xmlChar *)newname); - xmlDocDumpMemory(doc, &newxml, &size); + xmlNodeSetContent(node, (const xmlChar *)newname); - return newxml; + return virXMLNodeToString(doc, doc->children); } /* @@ -574,7 +568,7 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd) g_autoptr(virshStorageVol) newvol = NULL; const char *name = NULL; g_autofree char *origxml = NULL; - g_autofree xmlChar *newxml = NULL; + g_autofree char *newxml = NULL; unsigned int flags = 0; if (!(origvol = virshCommandOptVol(ctl, cmd, "vol", "pool", NULL))) @@ -608,8 +602,7 @@ cmdVolClone(vshControl *ctl, const vshCmd *cmd) return true; } - if (!(newvol = virStorageVolCreateXMLFrom(origpool, (char *) newxml, - origvol, flags))) { + if (!(newvol = virStorageVolCreateXMLFrom(origpool, newxml, origvol, flags))) { vshError(ctl, _("Failed to clone vol from %s"), virStorageVolGetName(origvol)); return false; -- 2.38.1
Use automatic pointer freeing for the 'disk_node' variable and remove the 'cleanup' label and 'functionReturn' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2fb1751f25..56490124a0 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12894,8 +12894,7 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) const char *target = NULL; g_autofree char *doc = NULL; int ret; - bool functionReturn = false; - xmlNodePtr disk_node = NULL; + g_autoptr(xmlNode) disk_node = NULL; bool current = vshCommandOptBool(cmd, "current"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); @@ -12916,7 +12915,7 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) return false; if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0) - goto cleanup; + return false; if (flags == VIR_DOMAIN_AFFECT_CONFIG) doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE); @@ -12924,24 +12923,23 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) doc = virDomainGetXMLDesc(dom, 0); if (!doc) - goto cleanup; + return false; if (persistent && virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE; if (!(disk_node = virshFindDisk(doc, target, VIRSH_FIND_DISK_NORMAL))) - goto cleanup; + return false; if (!(disk_xml = virXMLNodeToString(NULL, disk_node))) { vshSaveLibvirtError(); - goto cleanup; + return false; } if (vshCommandOptBool(cmd, "print-xml")) { vshPrint(ctl, "%s", disk_xml); - functionReturn = true; - goto cleanup; + return true; } if (flags != 0 || current) @@ -12951,15 +12949,11 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) if (ret != 0) { vshError(ctl, "%s", _("Failed to detach disk")); - goto cleanup; + return false; } vshPrintExtra(ctl, "%s", _("Disk detached successfully\n")); - functionReturn = true; - - cleanup: - xmlFreeNode(disk_node); - return functionReturn; + return true; } /* -- 2.38.1
Use automatic pointer freeing for the 'disk_node' variable and remove the 'cleanup' label and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 56490124a0..c4cf7aa95e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -13089,9 +13089,8 @@ cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) const char *source = NULL; const char *path = NULL; g_autofree char *doc = NULL; - xmlNodePtr disk_node = NULL; + g_autoptr(xmlNode) disk_node = NULL; g_autofree char *disk_xml = NULL; - bool ret = false; virshUpdateDiskXMLType update_type; const char *action = NULL; const char *success_msg = NULL; @@ -13152,38 +13151,34 @@ cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) return false; if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) - goto cleanup; + return false; if (flags & VIR_DOMAIN_AFFECT_CONFIG) doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE); else doc = virDomainGetXMLDesc(dom, 0); if (!doc) - goto cleanup; + return false; if (!(disk_node = virshFindDisk(doc, path, VIRSH_FIND_DISK_CHANGEABLE))) - goto cleanup; + return false; if (!(disk_xml = virshUpdateDiskXML(disk_node, source, block, path, update_type))) - goto cleanup; + return false; if (vshCommandOptBool(cmd, "print-xml")) { vshPrint(ctl, "%s", disk_xml); - } else { - if (virDomainUpdateDeviceFlags(dom, disk_xml, flags) != 0) { - vshError(ctl, _("Failed to complete action %s on media"), action); - goto cleanup; - } - - vshPrint(ctl, "%s", success_msg); + return true; } - ret = true; + if (virDomainUpdateDeviceFlags(dom, disk_xml, flags) != 0) { + vshError(ctl, _("Failed to complete action %s on media"), action); + return false; + } - cleanup: - xmlFreeNode(disk_node); - return ret; + vshPrint(ctl, "%s", success_msg); + return true; } static const vshCmdInfo info_domfstrim[] = { -- 2.38.1
Don't open-code the XPath lookup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c4cf7aa95e..1beaacd5e9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12639,8 +12639,9 @@ virshFindDisk(const char *doc, int type) { g_autoptr(xmlDoc) xml = NULL; - g_autoptr(xmlXPathObject) obj = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; + g_autofree xmlNodePtr *nodes = NULL; + ssize_t nnodes; xmlNodePtr cur = NULL; size_t i; @@ -12650,21 +12651,17 @@ virshFindDisk(const char *doc, return NULL; } - obj = xmlXPathEval(BAD_CAST "/domain/devices/disk", ctxt); - if (obj == NULL || - obj->type != XPATH_NODESET || - obj->nodesetval == NULL || - obj->nodesetval->nodeNr == 0) { + if ((nnodes = virXPathNodeSet("/domain/devices/disk", ctxt, &nodes)) <= 0) { vshError(NULL, "%s", _("Failed to get disk information")); return NULL; } /* search disk using @path */ - for (i = 0; i < obj->nodesetval->nodeNr; i++) { + for (i = 0; i < nnodes; i++) { bool is_supported = true; if (type == VIRSH_FIND_DISK_CHANGEABLE) { - xmlNodePtr n = obj->nodesetval->nodeTab[i]; + xmlNodePtr n = nodes[i]; is_supported = false; /* Check if the disk is CDROM or floppy disk */ @@ -12680,7 +12677,7 @@ virshFindDisk(const char *doc, continue; } - cur = obj->nodesetval->nodeTab[i]->children; + cur = nodes[i]->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { g_autofree char *tmp = NULL; @@ -12696,7 +12693,7 @@ virshFindDisk(const char *doc, } if (STREQ_NULLABLE(tmp, path)) { - xmlNodePtr ret = xmlCopyNode(obj->nodesetval->nodeTab[i], 1); + xmlNodePtr ret = xmlCopyNode(nodes[i], 1); /* drop backing store since they are not needed here */ virshDiskDropBackingStore(ret); return ret; -- 2.38.1
The XPath lookup guarantees that the top level element is always 'disk' so there's no need to check that it actually is. We can also remove the two unnecessary temporary variables. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1beaacd5e9..8bd058a33a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12658,22 +12658,13 @@ virshFindDisk(const char *doc, /* search disk using @path */ for (i = 0; i < nnodes; i++) { - bool is_supported = true; - if (type == VIRSH_FIND_DISK_CHANGEABLE) { - xmlNodePtr n = nodes[i]; - is_supported = false; + g_autofree char *device = virXMLPropString(nodes[i], "device"); /* Check if the disk is CDROM or floppy disk */ - if (virXMLNodeNameEqual(n, "disk")) { - g_autofree char *device_value = virXMLPropString(n, "device"); - - if (STREQ(device_value, "cdrom") || - STREQ(device_value, "floppy")) - is_supported = true; - } - - if (!is_supported) + if (device && + STRNEQ(device, "cdrom") && + STRNEQ(device, "floppy")) continue; } -- 2.38.1
Introduce a simple helper fetching a sub-element node by name. This is meant as a simple replacement for either open-coded versions of this or use of XPath for this trivial lookup. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virxml.c | 23 +++++++++++++++++++++++ src/util/virxml.h | 5 +++++ 3 files changed, 29 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ebd7bc61a8..209ec14544 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3671,6 +3671,7 @@ virXMLFormatElementInternal; virXMLFormatMetadata; virXMLNewNode; virXMLNodeContentString; +virXMLNodeGetSubelement; virXMLNodeNameEqual; virXMLNodeSanitizeNamespaces; virXMLNodeToString; diff --git a/src/util/virxml.c b/src/util/virxml.c index 870ba194b0..7d9d4bb234 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -849,6 +849,29 @@ virXPathBoolean(const char *xpath, } +/** + * virXMLNodeGetSubelement: + * @node: node to get subelement of + * @name: name of subelement to fetch + * + * Find and return a sub-element node of @node named @name. + */ +xmlNodePtr +virXMLNodeGetSubelement(xmlNodePtr node, + const char *name) +{ + xmlNodePtr n; + + for (n = node->children; n; n = n->next) { + if (n->type == XML_ELEMENT_NODE && + virXMLNodeNameEqual(n, name)) + return n; + } + + return NULL; +} + + /** * virXPathNode: * @xpath: the XPath string to evaluate diff --git a/src/util/virxml.h b/src/util/virxml.h index f19cbe59ae..d5b998263c 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -72,6 +72,11 @@ int virXPathLongLong(const char *xpath, xmlXPathContextPtr ctxt, long long *value); + +xmlNodePtr +virXMLNodeGetSubelement(xmlNodePtr node, + const char *name); + xmlNodePtr virXPathNode(const char *xpath, xmlXPathContextPtr ctxt); -- 2.38.1
The return value of virXMLPropString was assigned into 'tmp' multiple times and to prevent static analyzers moaning about a potential leak a short-circuited if+logic or was used. Replace the code by having a helper variable for each possibility and also replace the for-loop to iterate elements. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 48 +++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8bd058a33a..4d8690d9db 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12642,7 +12642,6 @@ virshFindDisk(const char *doc, g_autoptr(xmlXPathContext) ctxt = NULL; g_autofree xmlNodePtr *nodes = NULL; ssize_t nnodes; - xmlNodePtr cur = NULL; size_t i; xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt); @@ -12658,6 +12657,13 @@ virshFindDisk(const char *doc, /* search disk using @path */ for (i = 0; i < nnodes; i++) { + xmlNodePtr sourceNode; + g_autofree char *sourceFile = NULL; + g_autofree char *sourceDev = NULL; + g_autofree char *sourceDir = NULL; + g_autofree char *sourceName = NULL; + g_autofree char *targetDev = NULL; + if (type == VIRSH_FIND_DISK_CHANGEABLE) { g_autofree char *device = virXMLPropString(nodes[i], "device"); @@ -12668,29 +12674,25 @@ virshFindDisk(const char *doc, continue; } - cur = nodes[i]->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - g_autofree char *tmp = NULL; - - if (virXMLNodeNameEqual(cur, "source")) { - if ((tmp = virXMLPropString(cur, "file")) || - (tmp = virXMLPropString(cur, "dev")) || - (tmp = virXMLPropString(cur, "dir")) || - (tmp = virXMLPropString(cur, "name"))) { - } - } else if (virXMLNodeNameEqual(cur, "target")) { - tmp = virXMLPropString(cur, "dev"); - } + if ((sourceNode = virXMLNodeGetSubelement(nodes[i], "source"))) { + sourceFile = virXMLPropString(sourceNode, "file"); + sourceDev = virXMLPropString(sourceNode, "dev"); + sourceDir = virXMLPropString(sourceNode, "dir"); + sourceName = virXMLPropString(sourceNode, "name"); + } - if (STREQ_NULLABLE(tmp, path)) { - xmlNodePtr ret = xmlCopyNode(nodes[i], 1); - /* drop backing store since they are not needed here */ - virshDiskDropBackingStore(ret); - return ret; - } - } - cur = cur->next; + ctxt->node = nodes[i]; + targetDev = virXPathString("string(./target/@dev)", ctxt); + + if (STREQ_NULLABLE(targetDev, path) || + STREQ_NULLABLE(sourceFile, path) || + STREQ_NULLABLE(sourceDev, path) || + STREQ_NULLABLE(sourceDir, path) || + STREQ_NULLABLE(sourceName, path)) { + xmlNodePtr ret = xmlCopyNode(nodes[i], 1); + /* drop backing store since they are not needed here */ + virshDiskDropBackingStore(ret); + return ret; } } -- 2.38.1
On a Friday in 2022, Peter Krempa wrote:
The return value of virXMLPropString was assigned into 'tmp' multiple times and to prevent static analyzers moaning about a potential leak a short-circuited if+logic or was used.
where did the + come from?
Replace the code by having a helper variable for each possibility and also replace the for-loop to iterate elements.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 48 +++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 23 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On a Friday in 2022, Peter Krempa wrote:
This series converts open-coded virXPathNodeSet to the proper helper use and also adds --print-xml option to commands using xmlXpathEval which didn't have it.
Peter Krempa (13): virsh: Add --print-xml option for 'detach-interface' virshDomainDetachInterface: Use virXPathNodeSet instead of xmlXpathEval virsh: Add --print-xml option for 'domif-setlink' virsh: cmdDomIfSetLink: Use virXPathNodeSet instead of xmlXpathEval virsh: Refactor cleanup in 'cmdVolClone' virsh: Add --print-xml flag for 'vol-clone' command virsh: virshMakeCloneXML: Use virXPathNode instead of xmlXPathEval virsh: cmdDetachDisk: Refactor cleanup virsh: cmdChangeMedia: Refactor cleanup virshFindDisk: Use virXPathNodeSet instead of xmlXPathEval virshFindDisk: Sanitize removable media check util: xml: Introduce virXMLNodeGetSubelement virshFindDisk: Sanitize use of 'tmp' variable
docs/manpages/virsh.rst | 15 +- src/libvirt_private.syms | 1 + src/util/virxml.c | 23 +++ src/util/virxml.h | 5 + tools/virsh-domain.c | 298 ++++++++++++++++++--------------------- tools/virsh-volume.c | 61 ++++---- 6 files changed, 207 insertions(+), 196 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa