8:16 a.m.
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
8:16 a.m.
New subject: [PATCH 01/13] virsh: Add --print-xml option for 'detach-interface'
Signed-off-by: Peter Krempa <pkrempa(a)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
8:27 a.m.
New subject: [PATCH 01/13] virsh: Add --print-xml option for 'detach-interface'
On a Friday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)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(a)redhat.com>
Jano
8:16 a.m.
New subject: [PATCH 02/13] virshDomainDetachInterface: Use virXPathNodeSet instead of xmlXpathEval
Refactor the XPath lookup to use the internal helper.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 03/13] virsh: Add --print-xml option for 'domif-setlink'
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 04/13] virsh: cmdDomIfSetLink: Use virXPathNodeSet instead of xmlXpathEval
Refactor the XPath lookup to use the internal helper.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 05/13] virsh: Refactor cleanup in 'cmdVolClone'
Automatically free 'newxml' and remove the 'cleanup' label and
'ret'
variable.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 06/13] virsh: Add --print-xml flag for 'vol-clone' command
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 07/13] virsh: virshMakeCloneXML: Use virXPathNode instead of xmlXPathEval
Refactor the code to use the XPath helpers instead of open-coding them.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 08/13] virsh: cmdDetachDisk: Refactor cleanup
Use automatic pointer freeing for the 'disk_node' variable and remove
the 'cleanup' label and 'functionReturn' variable.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 09/13] virsh: cmdChangeMedia: Refactor cleanup
Use automatic pointer freeing for the 'disk_node' variable and remove
the 'cleanup' label and 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 10/13] virshFindDisk: Use virXPathNodeSet instead of xmlXPathEval
Don't open-code the XPath lookup.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:16 a.m.
New subject: [PATCH 11/13] virshFindDisk: Sanitize removable media check
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(a)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
8:16 a.m.
New subject: [PATCH 12/13] util: xml: Introduce virXMLNodeGetSubelement
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(a)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
8:16 a.m.
New subject: [PATCH 13/13] virshFindDisk: Sanitize use of 'tmp' variable
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(a)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
9:41 a.m.
New subject: [PATCH 13/13] virshFindDisk: Sanitize use of 'tmp' variable
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(a)redhat.com>
---
tools/virsh-domain.c | 48 +++++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 23 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:41 a.m.
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(a)redhat.com>
Jano
753
days inactive
753
days old
16 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa