The man page states that with --config the next boot is affected. This can be understood as if _only_ the next bood was affected. This isn't

true if the machine is running. This patch adds the full --live, --config, --current infrastructure and tweaks stuff to correctly support the obsolete --persistent flag. --- tools/virsh-domain.c | 41 ++++++++++++++++++++++++++++++----------- tools/virsh.pod | 21 ++++++++++++++------- 2 files changed, 44 insertions(+), 18 deletions(-)

@@ -9267,7 +9275,22 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) const char *from = NULL; char *buffer = NULL; bool ret = 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);

From my POV, I'd leave --persistent --live --config allowed. The change

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>.