Re: [libvirt] [PATCH] Use correct pci addresses during interface-detach[v2]

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