Hi Nitesh,
The subject is weird, how about:
virsh: properly handle detach-interface --live --config
On 04/07/2016 05:13 AM, 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.
> ---
> Change log:
> v4:
> * Code unchanged, updated with commit message,change log
> and steps to reproduce the issue..
>
> v3:
> * Created function vshDomainDetachInterface and called
> it once/twice from cmdDetachInterface depending on
> number of flags set (live/config/both). Passed the
> correct xml(live/persistent) to it.
>
> v2:
> * Changes only in cmdDetachInterface to pass the right domain xml
> depending on the flag set (live/config/both).
>
> v1:
> * Changes only in qemuDomainDetachDeviceFlags to set the right value
> in dev and dev_copy.
>
> 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
>
> tools/virsh-domain.c | 104 +++++++++++++++++++++++++++++++--------------------
> 1 file changed, 64 insertions(+), 40 deletions(-)
>
The patch has trailing whitespace. Make sure to run 'make syntax-check' to
pick up these types of issues.
tools/virsh-domain.c:11201:static bool
tools/virsh-domain.c:11290: if (ret == 0)
tools/virsh-domain.c:11323:
tools/virsh-domain.c:11332: }
tools/virsh-domain.c:11341: flags &= (~VIR_DOMAIN_AFFECT_CONFIG);
tools/virsh-domain.c:11342:
tools/virsh-domain.c:11353: VIR_FREE(doc_live);
maint.mk: found trailing blank(s)
maint.mk:749: recipe for target 'sc_trailing_blank' failed
make: *** [sc_trailing_blank] Error 1
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 62acecb..a6abaf5 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -10801,40 +10801,21 @@ static const vshCmdOptDef opts_detach_interface[] = {
> {.name = NULL}
> };
>
> -static bool
> -cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> +static bool
> +vshDomainDetachInterface(char *doc, unsigned int flags, virDomainPtr dom, vshControl *ctl, const vshCmd *cmd)
Name this virshDomainDetachInterface, matches similar functions like virshFindDisk
> {
> - virDomainPtr dom = NULL;
> xmlDocPtr xml = NULL;
> xmlXPathObjectPtr obj = NULL;
> xmlXPathContextPtr ctxt = NULL;
> xmlNodePtr cur = NULL, matchNode = NULL;
> - char *detach_xml = NULL;
> const char *mac = NULL, *type = NULL;
> - char *doc = NULL;
> + char *detach_xml = NULL;
> + bool current = vshCommandOptBool(cmd, "current");
> char buf[64];
> int diff_mac;
> size_t i;
> int ret;
> bool functionReturn = false;
> - unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> - bool current = vshCommandOptBool(cmd, "current");
> - bool config = vshCommandOptBool(cmd, "config");
> - bool live = vshCommandOptBool(cmd, "live");
> - bool persistent = vshCommandOptBool(cmd, "persistent");
> -
> - VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
> -
> - VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> - VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> -
> - if (config || persistent)
> - flags |= VIR_DOMAIN_AFFECT_CONFIG;
> - if (live)
> - flags |= VIR_DOMAIN_AFFECT_LIVE;
> -
> - if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> - return false;
>
> if (vshCommandOptStringReq(ctl, cmd, "type", &type) < 0)
> goto cleanup;
> @@ -10842,15 +10823,6 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
> goto cleanup;
>
> - if (persistent &&
> - virDomainIsActive(dom) == 1)
> - flags |= VIR_DOMAIN_AFFECT_LIVE;
> -
> - if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> - doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE);
> - else
> - doc = virDomainGetXMLDesc(dom, 0);
> -
> if (!doc)
> goto cleanup;
>
> @@ -10918,23 +10890,75 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> else
> ret = virDomainDetachDevice(dom, detach_xml);
>
> - if (ret != 0) {
> - vshError(ctl, "%s", _("Failed to detach interface"));
> - } else {
> - vshPrint(ctl, "%s", _("Interface detached successfully\n"));
> + if (ret == 0)
> functionReturn = true;
> - }
>
> cleanup:
> - VIR_FREE(doc);
> VIR_FREE(detach_xml);
> - virDomainFree(dom);
> + xmlFreeDoc(xml);
> xmlXPathFreeObject(obj);
> xmlXPathFreeContext(ctxt);
> - xmlFreeDoc(xml);
> return functionReturn;
> }
>
> +static bool
> +cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> +{
> + virDomainPtr dom = NULL;
> + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
> + char *doc_live = NULL, *doc_config = NULL;
> + bool current = vshCommandOptBool(cmd, "current");
> + bool config = vshCommandOptBool(cmd, "config");
> + bool live = vshCommandOptBool(cmd, "live");
> + bool persistent = vshCommandOptBool(cmd, "persistent");
> + bool functionReturn = false;
> +
> + VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
> +
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> + VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> +
> + if (config || persistent)
> + flags |= VIR_DOMAIN_AFFECT_CONFIG;
> +
> + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> + return false;
> +
> + if (persistent &&
> + virDomainIsActive(dom) == 1)
> + flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> + doc_config = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE);
> + if (!(functionReturn = vshDomainDetachInterface(doc_config, flags, dom, ctl, cmd)))
> + goto cleanup;
> + }
> +
Indentation is wrong here
> + if (live)
> + flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> + doc_live = virDomainGetXMLDesc(dom, 0);
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> + flags &= (~VIR_DOMAIN_AFFECT_CONFIG);
> +
Indentation is off here
> + functionReturn = vshDomainDetachInterface(doc_live, flags, dom, ctl, cmd);
> + }
> +
> + cleanup:
> + if (functionReturn == false) {
> + vshError(ctl, "%s", _("Failed to detach interface"));
> + } else {
> + vshPrint(ctl, "%s", _("Interface detached successfully\n"));
> + functionReturn = true;
> + }
> + VIR_FREE(doc_live);
> + VIR_FREE(doc_config);
> + virDomainFree(dom);
> + return functionReturn;
Indentation is off here
> +}
> +
> typedef enum {
> VIRSH_FIND_DISK_NORMAL,
> VIRSH_FIND_DISK_CHANGEABLE,
>
Can you make this two patches? First patch separates out
virshDomainDetachInterface, then the second patch makes the actual change to
call it twice.
(actually, if you're motivated, it would be nice to also break out the device
lookup logic to virshFindInterface similar to what we have with virshFindDisk,
but that's not a requirement)
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list