[libvirt] [PATCHv2 0/4] Add domain modification impact flags to hot-management commands
A respin of the series that changes some virsh commands to handle flags more sanely. This version fixes comments by Martin, Laine and Eric. This series omits already acked patches. Peter Krempa (4): virsh: Fix semantics of --config for "update-device" command virsh-domain: Add --live, --config, --current logic to cmdDetachInterface virsh-domain: Add --live, --config, --current logic to cmdDetachDevice virsh-domain: Add --live, --config, --current logic to cmdDetachDisk tools/virsh-domain.c | 182 +++++++++++++++++++++++++++++++++++++-------------- tools/virsh.pod | 81 +++++++++++++++++------ 2 files changed, 194 insertions(+), 69 deletions(-) -- 1.8.1.5
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next boot 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. --- Notes: Version 2: - note in the docs that semantics of the flags were fixed tools/virsh-domain.c | 40 +++++++++++++++++++++++++++++----------- tools/virsh.pod | 24 +++++++++++++++++------- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index fa5ab3d..2a228f5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9246,13 +9246,21 @@ static const vshCmdOptDef opts_update_device[] = { .help = N_("XML file") }, {.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") @@ -9267,7 +9275,21 @@ 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, 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; @@ -9275,19 +9297,15 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) goto cleanup; + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) { vshReportError(ctl); goto cleanup; } - if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } else { - flags = VIR_DOMAIN_AFFECT_LIVE; - } - if (vshCommandOptBool(cmd, "force")) flags |= VIR_DOMAIN_DEVICE_MODIFY_FORCE; diff --git a/tools/virsh.pod b/tools/virsh.pod index e7e82e3..04ea542 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1888,18 +1888,28 @@ 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<update-device> I<domain> I<file> [I<--config>] [I<--force>] +=item B<update-device> I<domain> I<file> [I<--force>] +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] Update the characteristics of a device associated with I<domain>, -based on the device definition in an XML I<file>. If the I<--config> -option is used, the changes will take affect the next time libvirt -starts the domain. For compatibility purposes, I<--persistent> is -alias of I<--config>. The I<--force> option can be used to force -device update, e.g., to eject a CD-ROM even if it is locked/mounted in -the domain. See the documentation at +based on the device definition in an XML I<file>. The I<--force> option +can be used to force device update, e.g., to eject a CD-ROM even if it is +locked/mounted in the domain. See the documentation at L<http://libvirt.org/formatdomain.html#elementsDevices> to learn about libvirt XML format for a device. +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>. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. + +Note that older versions of virsh used I<--config> as an alias for +I<--persistent>. + =item B<change-media> I<domain> I<path> [I<--eject>] [I<--insert>] [I<--update>] [I<source>] [I<--force>] [[I<--live>] [I<--config>] | [I<--current>]] -- 1.8.1.5
On 03/31/2013 03:22 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next boot 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. ---
Notes: Version 2: - note in the docs that semantics of the flags were fixed
tools/virsh-domain.c | 40 +++++++++++++++++++++++++++++----------- tools/virsh.pod | 24 +++++++++++++++++------- 2 files changed, 46 insertions(+), 18 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/31/2013 05:22 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next boot was affected. This isn't true if the machine is running.
You should probably change this comment to make it clear that you're changing the behavior of the option, rather than the documentation of what it does. After our discussion last week, I do agree that, although this is a change in behavior of an already-released command, it's acceptable because 1) it was different behavior from all other commands using --config, and 2) it was documented as behaving as all other commands.
This patch adds the full --live, --config, --current infrastructure and tweaks stuff to correctly support the obsolete --persistent flag. ---
Notes: Version 2: - note in the docs that semantics of the flags were fixed
ACK once you note in the commit log that the code was fixed, not the documentation.
tools/virsh-domain.c | 40 +++++++++++++++++++++++++++++----------- tools/virsh.pod | 24 +++++++++++++++++------- 2 files changed, 46 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index fa5ab3d..2a228f5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9246,13 +9246,21 @@ static const vshCmdOptDef opts_update_device[] = { .help = N_("XML file") }, {.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") @@ -9267,7 +9275,21 @@ 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, 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; @@ -9275,19 +9297,15 @@ cmdUpdateDevice(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) goto cleanup;
+ if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) { vshReportError(ctl); goto cleanup; }
- if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - } else { - flags = VIR_DOMAIN_AFFECT_LIVE; - } - if (vshCommandOptBool(cmd, "force")) flags |= VIR_DOMAIN_DEVICE_MODIFY_FORCE;
diff --git a/tools/virsh.pod b/tools/virsh.pod index e7e82e3..04ea542 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1888,18 +1888,28 @@ 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<update-device> I<domain> I<file> [I<--config>] [I<--force>] +=item B<update-device> I<domain> I<file> [I<--force>] +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
Update the characteristics of a device associated with I<domain>, -based on the device definition in an XML I<file>. If the I<--config> -option is used, the changes will take affect the next time libvirt -starts the domain. For compatibility purposes, I<--persistent> is -alias of I<--config>. The I<--force> option can be used to force -device update, e.g., to eject a CD-ROM even if it is locked/mounted in -the domain. See the documentation at +based on the device definition in an XML I<file>. The I<--force> option +can be used to force device update, e.g., to eject a CD-ROM even if it is +locked/mounted in the domain. See the documentation at L<http://libvirt.org/formatdomain.html#elementsDevices> to learn about libvirt XML format for a device.
+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>. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. + +Note that older versions of virsh used I<--config> as an alias for +I<--persistent>. + =item B<change-media> I<domain> I<path> [I<--eject>] [I<--insert>] [I<--update>] [I<source>] [I<--force>] [[I<--live>] [I<--config>] | [I<--current>]]
On 04/01/13 21:46, Laine Stump wrote:
On 03/31/2013 05:22 PM, Peter Krempa wrote:
The man page states that with --config the next boot is affected. This can be understood as if _only_ the next boot was affected. This isn't true if the machine is running.
You should probably change this comment to make it clear that you're changing the behavior of the option, rather than the documentation of what it does.
After our discussion last week, I do agree that, although this is a change in behavior of an already-released command, it's acceptable because 1) it was different behavior from all other commands using --config, and 2) it was documented as behaving as all other commands.
This patch adds the full --live, --config, --current infrastructure and tweaks stuff to correctly support the obsolete --persistent flag. ---
Notes: Version 2: - note in the docs that semantics of the flags were fixed
ACK once you note in the commit log that the code was fixed, not the documentation.
I mentioned this in the commit message, removed the stray newline in 3/4 and pushed this with the rest of the series. Thanks. Peter
Use the established approach to improve this function too. --- Notes: Version 2: - state that legacy API has no well defined semantics - add note about possible semantics change - remove mutual exclusivity of --persistent --live - remove --force parameter added due to too much copy and paste tools/virsh-domain.c | 47 ++++++++++++++++++++++++++++++++++------------- tools/virsh.pod | 19 +++++++++++++++---- 2 files changed, 49 insertions(+), 17 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2a228f5..d7778dd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9352,13 +9352,21 @@ 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 = NULL} }; @@ -9372,12 +9380,26 @@ 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, 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; @@ -9388,13 +9410,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; } @@ -9459,10 +9482,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); @@ -9478,6 +9498,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) } cleanup: + VIR_FREE(doc); virDomainFree(dom); xmlXPathFreeObject(obj); xmlXPathFreeContext(ctxt); diff --git a/tools/virsh.pod b/tools/virsh.pod index 04ea542..e67c3cc 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1877,16 +1877,27 @@ 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. When no flag is specified legacy API is used whose behavior depends +on the hypervisor driver. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. + +Note that older versions of virsh used I<--config> as an alias for +I<--persistent>. =item B<update-device> I<domain> I<file> [I<--force>] [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] -- 1.8.1.5
On 03/31/2013 03:22 PM, Peter Krempa wrote:
Use the established approach to improve this function too. ---
Notes: Version 2: - state that legacy API has no well defined semantics - add note about possible semantics change - remove mutual exclusivity of --persistent --live - remove --force parameter added due to too much copy and paste
tools/virsh-domain.c | 47 ++++++++++++++++++++++++++++++++++------------- tools/virsh.pod | 19 +++++++++++++++---- 2 files changed, 49 insertions(+), 17 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the established approach to improve this function too. --- Notes: Version 2: - same changes as previous patch tools/virsh-domain.c | 41 ++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 19 +++++++++++++++---- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d7778dd..fa9279b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9167,14 +9167,23 @@ static const vshCmdOptDef opts_detach_device[] = { .help = N_("XML file") }, {.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 = NULL} + }; static bool @@ -9185,11 +9194,29 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd) char *buffer = NULL; int ret; bool funcRet = false; - unsigned int flags; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + 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; + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0) goto cleanup; @@ -9198,14 +9225,10 @@ cmdDetachDevice(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, buffer, flags); - } else { + else ret = virDomainDetachDevice(dom, buffer); - } if (ret < 0) { vshError(ctl, _("Failed to detach device from %s"), from); diff --git a/tools/virsh.pod b/tools/virsh.pod index e67c3cc..b3aad62 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1859,16 +1859,27 @@ B<Note>: the optional target value is the name of a device to be created as the back-end on the node. If not provided a device named "vnetN" or "vifN" will be created automatically. -=item B<detach-device> I<domain> I<FILE> [I<--config>] +=item B<detach-device> I<domain> I<FILE> +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] Detach a device from the domain, takes the same kind of XML descriptions as command B<attach-device>. -If I<--config> is specified, alter persistent configuration, effect observed -on next boot, for compatibility purposes, I<--persistent> is alias of -I<--config>. For passthrough host devices, see also B<nodedev-reattach>, needed if the device does not use managed mode. +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. When no flag is specified legacy API is used whose behavior depends +on the hypervisor driver. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. + +Note that older versions of virsh used I<--config> as an alias for +I<--persistent>. + =item B<detach-disk> I<domain> I<target> [I<--config>] Detach a disk device from a domain. The I<target> is the device as seen -- 1.8.1.5
On 03/31/2013 03:22 PM, Peter Krempa wrote:
Use the established approach to improve this function too. ---
Notes: Version 2: - same changes as previous patch
tools/virsh-domain.c | 41 ++++++++++++++++++++++++++++++++--------- tools/virsh.pod | 19 +++++++++++++++---- 2 files changed, 47 insertions(+), 13 deletions(-)
ACK.
+ }, {.name = NULL} + };
Looks like this blank line is an accident. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Use the established approach to improve this function too. --- Notes: Version 2: - same notes as previous patch tools/virsh-domain.c | 54 +++++++++++++++++++++++++++++++++++----------------- tools/virsh.pod | 19 ++++++++++++++---- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index fa9279b..1be1a26 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9771,13 +9771,21 @@ static const vshCmdOptDef opts_detach_disk[] = { .help = N_("target of disk device") }, {.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 = NULL} }; @@ -9790,8 +9798,22 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) char *doc = NULL; int ret; bool functionReturn = false; - unsigned int flags; xmlNodePtr disk_node = NULL; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool persistent = vshCommandOptBool(cmd, "persistent"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + 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; @@ -9799,10 +9821,13 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "target", &target) < 0) goto cleanup; - doc = virDomainGetXMLDesc(dom, 0); - if (!doc) + if (!(doc = virDomainGetXMLDesc(dom, 0))) goto cleanup; + if (persistent && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!(disk_node = vshFindDisk(doc, target, VSH_FIND_DISK_NORMAL))) goto cleanup; @@ -9810,24 +9835,19 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) VSH_PREPARE_DISK_XML_NONE))) goto cleanup; - if (vshCommandOptBool(cmd, "config")) { - flags = VIR_DOMAIN_AFFECT_CONFIG; - if (virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; - ret = virDomainDetachDeviceFlags(dom, - disk_xml, - flags); - } else { + if (flags != 0) + ret = virDomainDetachDeviceFlags(dom, disk_xml, flags); + else ret = virDomainDetachDevice(dom, disk_xml); - } if (ret != 0) { vshError(ctl, "%s", _("Failed to detach disk")); - } else { - vshPrint(ctl, "%s", _("Disk detached successfully\n")); - functionReturn = true; + goto cleanup; } + vshPrint(ctl, "%s", _("Disk detached successfully\n")); + functionReturn = true; + cleanup: xmlFreeNode(disk_node); VIR_FREE(disk_xml); diff --git a/tools/virsh.pod b/tools/virsh.pod index b3aad62..6ef509b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1880,13 +1880,24 @@ an offline domain, and like I<--live> I<--config> for a running domain. Note that older versions of virsh used I<--config> as an alias for I<--persistent>. -=item B<detach-disk> I<domain> I<target> [I<--config>] +=item B<detach-disk> I<domain> I<target> +[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] Detach a disk device from a domain. The I<target> is the device as seen from 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. When no flag is specified legacy API is used whose behavior depends +on the hypervisor driver. + +For compatibility purposes, I<--persistent> behaves like I<--config> for +an offline domain, and like I<--live> I<--config> for a running domain. + +Note that older versions of virsh used I<--config> as an alias for +I<--persistent>. =item B<detach-interface> I<domain> I<type> [I<--mac mac>] [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] -- 1.8.1.5
On 03/31/2013 03:22 PM, Peter Krempa wrote:
Use the established approach to improve this function too. ---
Notes: Version 2: - same notes as previous patch
tools/virsh-domain.c | 54 +++++++++++++++++++++++++++++++++++----------------- tools/virsh.pod | 19 ++++++++++++++---- 2 files changed, 52 insertions(+), 21 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Laine Stump -
Peter Krempa