11:02 a.m.
On 03/27/13 13:53, Martin Kletzander wrote:
On 03/21/2013 05:42 PM, Peter Krempa wrote:
> Use the established approach to improve this function too.
> ---
> tools/virsh-domain.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
> tools/virsh.pod | 15 +++++++++++----
> 2 files changed, 50 insertions(+), 17 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 6741837..df72c78 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -9353,13 +9353,25 @@ static const vshCmdOptDef opts_detach_interface[] = {
> .help = N_("MAC address")
> },
> {.name = "persistent",
> - .type = VSH_OT_ALIAS,
> - .help = "config"
> + .type = VSH_OT_BOOL,
> + .help = N_("make live change persistent")
> },
> {.name = "config",
> .type = VSH_OT_BOOL,
> .help = N_("affect next boot")
> },
> + {.name = "live",
> + .type = VSH_OT_BOOL,
> + .help = N_("affect running domain")
> + },
> + {.name = "current",
> + .type = VSH_OT_BOOL,
> + .help = N_("affect current domain")
> + },
> + {.name = "force",
> + .type = VSH_OT_BOOL,
> + .help = N_("force device update")
> + },
Adding non-used --force paramter, copy-paste error?
> {.name = NULL}
> };
>
> @@ -9373,12 +9385,27 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> xmlNodePtr cur = NULL, matchNode = NULL;
> xmlBufferPtr xml_buf = NULL;
> const char *mac =NULL, *type = NULL;
> - char *doc;
> + char *doc = NULL;
> char buf[64];
> int i = 0, diff_mac;
> int ret;
> int functionReturn = false;
> - unsigned int flags;
> + 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, live);
Same as in previous patch, remove this line.
> + 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 = vshCommandOptDomain(ctl, cmd, NULL)))
> return false;
> @@ -9389,13 +9416,14 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
> goto cleanup;
>
> - doc = virDomainGetXMLDesc(dom, 0);
> - if (!doc)
> + if (persistent &&
> + virDomainIsActive(dom) == 1)
> + flags |= VIR_DOMAIN_AFFECT_LIVE;
> +
> + if (!(doc = virDomainGetXMLDesc(dom, 0)))
> goto cleanup;
>
> - xml = virXMLParseStringCtxt(doc, _("(domain_definition)"),
&ctxt);
> - VIR_FREE(doc);
> - if (!xml) {
> + if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"),
&ctxt))) {
> vshError(ctl, "%s", _("Failed to get interface
information"));
> goto cleanup;
> }
> @@ -9460,10 +9488,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> goto cleanup;
> }
>
> - if (vshCommandOptBool(cmd, "config")) {
> - flags = VIR_DOMAIN_AFFECT_CONFIG;
> - if (virDomainIsActive(dom) == 1)
> - flags |= VIR_DOMAIN_AFFECT_LIVE;
> + if (flags != 0) {
> ret = virDomainDetachDeviceFlags(dom,
> (char *)xmlBufferContent(xml_buf),
> flags);
This if seems pointless, because we usually call the *Flags() function
with flags=0, but this particular function is (for example in qemu
driver) calling the *Flags() function with AFFECT_LIVE. The current
virsh code also adds the AFFECT_LIVE automatically, as you can see...
> @@ -9479,6 +9504,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
> }
>
> cleanup:
> + VIR_FREE(doc);
If this fixes a leak, it could be mentioned in the commit message.
> virDomainFree(dom);
> xmlXPathFreeObject(obj);
> xmlXPathFreeContext(ctxt);
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index a9e8c65..ebbe201 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1876,16 +1876,23 @@ If I<--config> is specified, alter persistent
configuration, effect observed
> on next boot, for compatibility purposes, I<--persistent> is alias of
> I<--config>.
>
> -=item B<detach-interface> I<domain> I<type> [I<--mac mac>]
[I<--config>]
> +=item B<detach-interface> I<domain> I<type> [I<--mac mac>]
> +[[[I<--live>] [I<--config>] | [I<--current>]] |
[I<--persistent>]]
>
> Detach a network interface from a domain.
> I<type> can be either I<network> to indicate a physical network device
or
> I<bridge> to indicate a bridge to a device. It is recommended to use the
> I<mac> option to distinguish between the interfaces if more than one are
> present on the domain.
> -If I<--config> is specified, alter persistent configuration, effect observed
> -on next boot, for compatibility purposes, I<--persistent> is alias of
> -I<--config>.
> +
> +If I<--live> is specified, affect a running domain.
> +If I<--config> is specified, affect the next startup of a persistent domain.
> +If I<--current> is specified, affect the current domain state.
> +Both I<--live> and I<--config> flags may be given, but
I<--current> is
> +exclusive. Not specifying any flag is the same as specifying I<--current>.
I changed the last sentence to: When no flag is specified legacy API is
used whose behavior depends on the hypervisor driver.
Peter