On 19.02.2016 12:53, 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.
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
Okay, I can see the problem but I find the solution a bit hackish.
Signed-off-by:nitkon12@linux.vnet.ibm.com
---
tools/virsh-domain.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 62acecb..43c8436 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -10815,7 +10815,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
char buf[64];
int diff_mac;
size_t i;
- int ret;
+ int ret, flag_live_config_both = 0;
This new flag makes me confused. I know what you're trying to achieve
with it, but the name could be better. How about configDetached?
Moreover, it's a boolean not an int.
bool functionReturn = false;
unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
bool current = vshCommandOptBool(cmd, "current");
@@ -10830,7 +10830,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
if (config || persistent)
flags |= VIR_DOMAIN_AFFECT_CONFIG;
- if (live)
+ if (!(config || persistent) && live) // Only Live
flags |= VIR_DOMAIN_AFFECT_LIVE;
I don't think I follow. But maybe I'm misguided by --persistent.
Historically it just an alias for --config. But not in this case. I
wonder why.
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
@@ -10851,6 +10851,8 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
else
doc = virDomainGetXMLDesc(dom, 0);
+ forlivexml:
+
I'd rather avoid introducing this kind of label. How about putting the
important code (interface lookup in domain XML) into a separate function
and call it twice if needed? That way you can also avoid using
@flag_live_config_both.
if (!doc)
goto cleanup;
@@ -10921,6 +10923,14 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
if (ret != 0) {
vshError(ctl, "%s", _("Failed to detach interface"));
} else {
+ if ((config || persistent) && live && flag_live_config_both ==
0) {//executed only when live config both in cmd.
+ doc = virDomainGetXMLDesc(dom, 0);
+ flag_live_config_both=1; //Need to execute this code only once
+ flags |= VIR_DOMAIN_AFFECT_LIVE; //set live flag
+ flags &= (~VIR_DOMAIN_AFFECT_CONFIG ); // need to unset config flag as
config xml detach is already done.
+ mac=NULL;
+ goto forlivexml;
+ }
vshPrint(ctl, "%s", _("Interface detached
successfully\n"));
functionReturn = true;
}
Then, this patch does not comply with our formatting rules. Run 'make
syntax-check' to see all the problems.
Looking forward to v3.
Michal