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