Hi Nitesh,
The subject is weird, how about:
virsh: properly handle detach-interface --live --config
On 04/07/2016 05:13 AM, Nitesh Konkar wrote:
The virsh attach virsh detach interface command fails when both live
and config
are set and when the interface gets attached to different pci slots
on live and config xml respectively.
When we attach an interface with both --live and --config,
the first time they get the same PCI slots, but the second time
onwards it differs and hence the virsh detach-interface --live
--config command fails. This patch makes sure that when both
--live --config are set , qemuDomainDetachDeviceFlags is called
twice, once with config xml and once with live xml.
---
Change log:
v4:
* Code unchanged, updated with commit message,change log
and steps to reproduce the issue..
v3:
* Created function vshDomainDetachInterface and called
it once/twice from cmdDetachInterface depending on
number of flags set (live/config/both). Passed the
correct xml(live/persistent) to it.
v2:
* Changes only in cmdDetachInterface to pass the right domain xml
depending on the flag set (live/config/both).
v1:
* Changes only in qemuDomainDetachDeviceFlags to set the right value
in dev and dev_copy.
Steps to see the issue:
virsh attach-interface --domain DomainName --type network --source default --mac
52:54:00:4b:76:5f --live --config
virsh detach-interface --domain DomainName --type network --mac 52:54:00:4b:76:5f --live
--config
virsh attach-interface --domain DomainName --type network --source default --mac
52:54:00:4b:76:5f --live --config
virsh detach-interface --domain DomainName --type network --mac 52:54:00:4b:76:5f --live
--config
tools/virsh-domain.c | 104 +++++++++++++++++++++++++++++++--------------------
1 file changed, 64 insertions(+), 40 deletions(-)
The patch has trailing whitespace. Make sure to run 'make syntax-check' to
pick up these types of issues.
tools/virsh-domain.c:11201:static bool
tools/virsh-domain.c:11290: if (ret == 0)
tools/virsh-domain.c:11323:
tools/virsh-domain.c:11332: }
tools/virsh-domain.c:11341: flags &= (~VIR_DOMAIN_AFFECT_CONFIG);
tools/virsh-domain.c:11342:
tools/virsh-domain.c:11353: VIR_FREE(doc_live);
maint.mk: found trailing blank(s)
maint.mk:749: recipe for target 'sc_trailing_blank' failed
make: *** [sc_trailing_blank] Error 1
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 62acecb..a6abaf5 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10801,40 +10801,21 @@ static const vshCmdOptDef opts_detach_interface[] = {
{.name = NULL}
};
-static bool
-cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
+static bool
+vshDomainDetachInterface(char *doc, unsigned int flags, virDomainPtr dom, vshControl
*ctl, const vshCmd *cmd)
Name this virshDomainDetachInterface, matches similar functions like virshFindDisk
{
- virDomainPtr dom = NULL;
xmlDocPtr xml = NULL;
xmlXPathObjectPtr obj = NULL;
xmlXPathContextPtr ctxt = NULL;
xmlNodePtr cur = NULL, matchNode = NULL;
- char *detach_xml = NULL;
const char *mac = NULL, *type = NULL;
- char *doc = NULL;
+ char *detach_xml = NULL;
+ bool current = vshCommandOptBool(cmd, "current");
char buf[64];
int diff_mac;
size_t i;
int ret;
bool functionReturn = false;
- unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
- bool current = vshCommandOptBool(cmd, "current");
- bool config = vshCommandOptBool(cmd, "config");
- bool live = vshCommandOptBool(cmd, "live");
- bool persistent = vshCommandOptBool(cmd, "persistent");
-
- VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
-
- VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
- VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
-
- if (config || persistent)
- flags |= VIR_DOMAIN_AFFECT_CONFIG;
- if (live)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
-
- if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
- return false;
if (vshCommandOptStringReq(ctl, cmd, "type", &type) < 0)
goto cleanup;
@@ -10842,15 +10823,6 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
goto cleanup;
- if (persistent &&
- virDomainIsActive(dom) == 1)
- flags |= VIR_DOMAIN_AFFECT_LIVE;
-
- if (flags & VIR_DOMAIN_AFFECT_CONFIG)
- doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE);
- else
- doc = virDomainGetXMLDesc(dom, 0);
-
if (!doc)
goto cleanup;
@@ -10918,23 +10890,75 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
else
ret = virDomainDetachDevice(dom, detach_xml);
- if (ret != 0) {
- vshError(ctl, "%s", _("Failed to detach interface"));
- } else {
- vshPrint(ctl, "%s", _("Interface detached
successfully\n"));
+ if (ret == 0)
functionReturn = true;
- }
cleanup:
- VIR_FREE(doc);
VIR_FREE(detach_xml);
- virDomainFree(dom);
+ xmlFreeDoc(xml);
xmlXPathFreeObject(obj);
xmlXPathFreeContext(ctxt);
- xmlFreeDoc(xml);
return functionReturn;
}
+static bool
+cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
+{
+ virDomainPtr dom = NULL;
+ unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+ char *doc_live = NULL, *doc_config = NULL;
+ bool current = vshCommandOptBool(cmd, "current");
+ bool config = vshCommandOptBool(cmd, "config");
+ bool live = vshCommandOptBool(cmd, "live");
+ bool persistent = vshCommandOptBool(cmd, "persistent");
+ bool functionReturn = false;
+
+ VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
+
+ VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
+ VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
+
+ if (config || persistent)
+ flags |= VIR_DOMAIN_AFFECT_CONFIG;
+
+ if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+ return false;
+
+ if (persistent &&
+ virDomainIsActive(dom) == 1)
+ flags |= VIR_DOMAIN_AFFECT_LIVE;
+
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
+ doc_config = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE);
+ if (!(functionReturn = vshDomainDetachInterface(doc_config, flags, dom, ctl,
cmd)))
+ goto cleanup;
+ }
+
Indentation is wrong here
+ if (live)
+ flags |= VIR_DOMAIN_AFFECT_LIVE;
+
+ if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+ doc_live = virDomainGetXMLDesc(dom, 0);
+
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG)
+ flags &= (~VIR_DOMAIN_AFFECT_CONFIG);
+
Indentation is off here
+ functionReturn = vshDomainDetachInterface(doc_live, flags,
dom, ctl, cmd);
+ }
+
+ cleanup:
+ if (functionReturn == false) {
+ vshError(ctl, "%s", _("Failed to detach interface"));
+ } else {
+ vshPrint(ctl, "%s", _("Interface detached
successfully\n"));
+ functionReturn = true;
+ }
+ VIR_FREE(doc_live);
+ VIR_FREE(doc_config);
+ virDomainFree(dom);
+ return functionReturn;
Indentation is off here
+}
+
typedef enum {
VIRSH_FIND_DISK_NORMAL,
VIRSH_FIND_DISK_CHANGEABLE,
Can you make this two patches? First patch separates out
virshDomainDetachInterface, then the second patch makes the actual change to
call it twice.
(actually, if you're motivated, it would be nice to also break out the device
lookup logic to virshFindInterface similar to what we have with virshFindDisk,
but that's not a requirement)
Thanks,
Cole