Hello Michal, Thanks for the feedback. I have sent a follow up v3 version of my patch.
Warm Regards, Nitesh Konkar. On Mon, Feb 22, 2016 at 6:02 PM, Michal Privoznik <[email protected]> wrote: > 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:[email protected] > > --- > > 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 > >
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
