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@xxxxxxxxxxxxxxxxxx
> ---
> 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
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list