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