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

[Nitesh Konkar]

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 <mprivozn at redhat.com>
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:nitkon12 at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160307/61844744/attachment-0001.htm>