[libvirt] [PATCH 1/2] virsh: don't reject undefine on active domain
The public API documents that undefine may be used to transition a running persistent domain into a transient one. Many drivers still do not support this usage, but virsh shouldn't be getting in the way of those that do support it. * tools/virsh.c (cmdUndefine): Allow undefine on active domains; the drivers may still reject it, but it is a valid API usage. * tests/undefine (error): Fix the test to match. --- tests/undefine | 9 +++++---- tools/virsh.c | 15 +-------------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/tests/undefine b/tests/undefine index f89a91e..5c39e27 100755 --- a/tests/undefine +++ b/tests/undefine @@ -1,7 +1,7 @@ #!/bin/sh # exercise virsh's "undefine" command -# Copyright (C) 2008-2009 Red Hat, Inc. +# Copyright (C) 2008-2009, 2011 Red Hat, Inc. # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -30,6 +30,7 @@ fi fail=0 # Attempt to undefine a running domain, by domain name. +# Although the API allows this, the test hypervisor does not. $abs_top_builddir/tools/virsh -q -c test:///default undefine test > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 @@ -38,12 +39,12 @@ error: internal error Domain 'test' is still running EOF compare exp out || fail=1 -# A different diagnostic when specifying a domain ID +# A similar diagnostic when specifying a domain ID $abs_top_builddir/tools/virsh -q -c test:///default undefine 1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: a running domain like 1 cannot be undefined; -to undefine, first shutdown then undefine using its name or UUID +error: Failed to undefine domain 1 +error: internal error Domain 'test' is still running EOF compare exp out || fail=1 diff --git a/tools/virsh.c b/tools/virsh.c index b0d43ba..371e010 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1431,7 +1431,6 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; bool ret = true; const char *name = NULL; - int id; int flags = 0; int managed_save = vshCommandOptBool(cmd, "managed-save"); int has_managed_save = 0; @@ -1449,19 +1448,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "domain", &name) <= 0) return false; - if (name && virStrToLong_i(name, NULL, 10, &id) == 0 - && id >= 0 && (dom = virDomainLookupByID(ctl->conn, id))) { - vshError(ctl, - _("a running domain like %s cannot be undefined;\n" - "to undefine, first shutdown then undefine" - " using its name or UUID"), - name); - virDomainFree(dom); - return false; - } - if (!(dom = vshCommandOptDomainBy(ctl, cmd, &name, - VSH_BYNAME|VSH_BYUUID))) - return false; + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) has_managed_save = virDomainHasManagedSaveImage(dom, 0); if (has_managed_save < 0) { -- 1.7.4.4
We forgot to add virDomainUndefineFlags for a couple of hypervisors. This wires up trivial versions (since neither hypervisor supports managed save yet, they do not need to support any flags). * src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Update caller. (vboxDomainUndefine): Move guts... (vboxDomainUndefineFlags): ...to new function. * src/xenapi/xenapi_driver.c (xenapiDomainUndefine) (xenapiDomainUndefineFlags): Likewise. --- I'm still debating on whether implementations that lack managed save should trivially support 'undefine --managed-save' (there's none to undefine), instead of the current behavior of rejecting it as an unknown flag. But that's an independent question. src/vbox/vbox_tmpl.c | 14 +++++++++++--- src/xenapi/xenapi_driver.c | 13 +++++++++++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 5c71729..822e899 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -251,7 +251,7 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL; static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); -static int vboxDomainUndefine(virDomainPtr dom); +static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags); static void vboxDriverLock(vboxGlobalData *data) { virMutexLock(&data->lock); @@ -1193,7 +1193,7 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml, return NULL; if (vboxDomainCreate(dom) < 0) { - vboxDomainUndefine(dom); + vboxDomainUndefineFlags(dom, 0); virUnrefDomain(dom); return NULL; } @@ -4973,7 +4973,7 @@ cleanup: } static int -vboxDomainUndefine(virDomainPtr dom) +vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, int, -1); IMachine *machine = NULL; @@ -4982,6 +4982,7 @@ vboxDomainUndefine(virDomainPtr dom) #if VBOX_API_VERSION >= 4000 vboxArray media = VBOX_ARRAY_INITIALIZER; #endif + virCheckFlags(0, -1); vboxIIDFromUUID(&iid, dom->uuid); @@ -5131,6 +5132,12 @@ vboxDomainUndefine(virDomainPtr dom) return ret; } +static int +vboxDomainUndefine(virDomainPtr dom) +{ + return vboxDomainUndefineFlags(dom, 0); +} + static int vboxDomainAttachDeviceImpl(virDomainPtr dom, const char *xml, int mediaChangeOnly ATTRIBUTE_UNUSED) { @@ -8806,6 +8813,7 @@ virDriver NAME(Driver) = { .domainCreateWithFlags = vboxDomainCreateWithFlags, /* 0.8.2 */ .domainDefineXML = vboxDomainDefineXML, /* 0.6.3 */ .domainUndefine = vboxDomainUndefine, /* 0.6.3 */ + .domainUndefineFlags = vboxDomainUndefineFlags, /* 0.9.5 */ .domainAttachDevice = vboxDomainAttachDevice, /* 0.6.3 */ .domainAttachDeviceFlags = vboxDomainAttachDeviceFlags, /* 0.7.7 */ .domainDetachDevice = vboxDomainDetachDevice, /* 0.6.3 */ diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index d40bc3e..80a706a 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1696,17 +1696,19 @@ xenapiDomainDefineXML (virConnectPtr conn, const char *xml) } /* - * xenapiDomainUndefine + * xenapiDomainUndefineFlags * * destroys a domain * Return 0 on success or -1 in case of error */ static int -xenapiDomainUndefine (virDomainPtr dom) +xenapiDomainUndefineFlags(virDomainPtr dom, unsigned int flags) { struct xen_vm_set *vms; xen_vm vm; xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session; + virCheckFlags(0, -1); + if (xen_vm_get_by_name_label(session, &vms, dom->name) && vms->size > 0) { if (vms->size != 1) { xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, @@ -1728,6 +1730,12 @@ xenapiDomainUndefine (virDomainPtr dom) return -1; } +static int +xenapiDomainUndefine(virDomainPtr dom) +{ + return xenapiDomainUndefineFlags(dom, 0); +} + /* * xenapiDomainGetAutostart * @@ -1922,6 +1930,7 @@ static virDriver xenapiDriver = { .domainCreateWithFlags = xenapiDomainCreateWithFlags, /* 0.8.2 */ .domainDefineXML = xenapiDomainDefineXML, /* 0.8.0 */ .domainUndefine = xenapiDomainUndefine, /* 0.8.0 */ + .domainUndefineFlags = xenapiDomainUndefineFlags, /* 0.9.5 */ .domainGetAutostart = xenapiDomainGetAutostart, /* 0.8.0 */ .domainSetAutostart = xenapiDomainSetAutostart, /* 0.8.0 */ .domainGetSchedulerType = xenapiDomainGetSchedulerType, /* 0.8.0 */ -- 1.7.4.4
On Thu, Aug 11, 2011 at 08:39:23PM -0600, Eric Blake wrote:
We forgot to add virDomainUndefineFlags for a couple of hypervisors. This wires up trivial versions (since neither hypervisor supports managed save yet, they do not need to support any flags).
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Update caller. (vboxDomainUndefine): Move guts... (vboxDomainUndefineFlags): ...to new function. * src/xenapi/xenapi_driver.c (xenapiDomainUndefine) (xenapiDomainUndefineFlags): Likewise. ---
I'm still debating on whether implementations that lack managed save should trivially support 'undefine --managed-save' (there's none to undefine), instead of the current behavior of rejecting it as an unknown flag. But that's an independent question.
src/vbox/vbox_tmpl.c | 14 +++++++++++--- src/xenapi/xenapi_driver.c | 13 +++++++++++-- 2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 5c71729..822e899 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -251,7 +251,7 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); -static int vboxDomainUndefine(virDomainPtr dom); +static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags);
static void vboxDriverLock(vboxGlobalData *data) { virMutexLock(&data->lock); @@ -1193,7 +1193,7 @@ static virDomainPtr vboxDomainCreateXML(virConnectPtr conn, const char *xml, return NULL;
if (vboxDomainCreate(dom) < 0) { - vboxDomainUndefine(dom); + vboxDomainUndefineFlags(dom, 0); virUnrefDomain(dom); return NULL; } @@ -4973,7 +4973,7 @@ cleanup: }
static int -vboxDomainUndefine(virDomainPtr dom) +vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags) { VBOX_OBJECT_CHECK(dom->conn, int, -1); IMachine *machine = NULL; @@ -4982,6 +4982,7 @@ vboxDomainUndefine(virDomainPtr dom) #if VBOX_API_VERSION >= 4000 vboxArray media = VBOX_ARRAY_INITIALIZER; #endif + virCheckFlags(0, -1);
vboxIIDFromUUID(&iid, dom->uuid);
@@ -5131,6 +5132,12 @@ vboxDomainUndefine(virDomainPtr dom) return ret; }
+static int +vboxDomainUndefine(virDomainPtr dom) +{ + return vboxDomainUndefineFlags(dom, 0); +} + static int vboxDomainAttachDeviceImpl(virDomainPtr dom, const char *xml, int mediaChangeOnly ATTRIBUTE_UNUSED) { @@ -8806,6 +8813,7 @@ virDriver NAME(Driver) = { .domainCreateWithFlags = vboxDomainCreateWithFlags, /* 0.8.2 */ .domainDefineXML = vboxDomainDefineXML, /* 0.6.3 */ .domainUndefine = vboxDomainUndefine, /* 0.6.3 */ + .domainUndefineFlags = vboxDomainUndefineFlags, /* 0.9.5 */ .domainAttachDevice = vboxDomainAttachDevice, /* 0.6.3 */ .domainAttachDeviceFlags = vboxDomainAttachDeviceFlags, /* 0.7.7 */ .domainDetachDevice = vboxDomainDetachDevice, /* 0.6.3 */ diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index d40bc3e..80a706a 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1696,17 +1696,19 @@ xenapiDomainDefineXML (virConnectPtr conn, const char *xml) }
/* - * xenapiDomainUndefine + * xenapiDomainUndefineFlags * * destroys a domain * Return 0 on success or -1 in case of error */ static int -xenapiDomainUndefine (virDomainPtr dom) +xenapiDomainUndefineFlags(virDomainPtr dom, unsigned int flags) { struct xen_vm_set *vms; xen_vm vm; xen_session *session = ((struct _xenapiPrivate *)(dom->conn->privateData))->session; + virCheckFlags(0, -1); + if (xen_vm_get_by_name_label(session, &vms, dom->name) && vms->size > 0) { if (vms->size != 1) { xenapiSessionErrorHandler(dom->conn, VIR_ERR_INTERNAL_ERROR, @@ -1728,6 +1730,12 @@ xenapiDomainUndefine (virDomainPtr dom) return -1; }
+static int +xenapiDomainUndefine(virDomainPtr dom) +{ + return xenapiDomainUndefineFlags(dom, 0); +} + /* * xenapiDomainGetAutostart * @@ -1922,6 +1930,7 @@ static virDriver xenapiDriver = { .domainCreateWithFlags = xenapiDomainCreateWithFlags, /* 0.8.2 */ .domainDefineXML = xenapiDomainDefineXML, /* 0.8.0 */ .domainUndefine = xenapiDomainUndefine, /* 0.8.0 */ + .domainUndefineFlags = xenapiDomainUndefineFlags, /* 0.9.5 */ .domainGetAutostart = xenapiDomainGetAutostart, /* 0.8.0 */ .domainSetAutostart = xenapiDomainSetAutostart, /* 0.8.0 */ .domainGetSchedulerType = xenapiDomainGetSchedulerType, /* 0.8.0 */
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/11/2011 09:12 PM, Daniel Veillard wrote:
On Thu, Aug 11, 2011 at 08:39:23PM -0600, Eric Blake wrote:
We forgot to add virDomainUndefineFlags for a couple of hypervisors. This wires up trivial versions (since neither hypervisor supports managed save yet, they do not need to support any flags).
* src/vbox/vbox_tmpl.c (vboxDomainCreateXML): Update caller. (vboxDomainUndefine): Move guts... (vboxDomainUndefineFlags): ...to new function. * src/xenapi/xenapi_driver.c (xenapiDomainUndefine) (xenapiDomainUndefineFlags): Likewise. ---
I'm still debating on whether implementations that lack managed save should trivially support 'undefine --managed-save' (there's none to undefine), instead of the current behavior of rejecting it as an unknown flag. But that's an independent question.
What I'll probably do is teach virsh to treat --managed-save as a no-op if a domain does not support managed save rather than forcing the flag down to the client to be rejected; touching just virsh is nicer than touching every single hypervisor to add an ignored flag bit, and I already have to touch virsh undefine to implement new --snapshot flags.
ACK,
Pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 11, 2011 at 08:39:22PM -0600, Eric Blake wrote:
The public API documents that undefine may be used to transition a running persistent domain into a transient one. Many drivers still do not support this usage, but virsh shouldn't be getting in the way of those that do support it.
* tools/virsh.c (cmdUndefine): Allow undefine on active domains; the drivers may still reject it, but it is a valid API usage. * tests/undefine (error): Fix the test to match. --- tests/undefine | 9 +++++---- tools/virsh.c | 15 +-------------- 2 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/tests/undefine b/tests/undefine index f89a91e..5c39e27 100755 --- a/tests/undefine +++ b/tests/undefine @@ -1,7 +1,7 @@ #!/bin/sh # exercise virsh's "undefine" command
-# Copyright (C) 2008-2009 Red Hat, Inc. +# Copyright (C) 2008-2009, 2011 Red Hat, Inc.
# This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -30,6 +30,7 @@ fi fail=0
# Attempt to undefine a running domain, by domain name. +# Although the API allows this, the test hypervisor does not. $abs_top_builddir/tools/virsh -q -c test:///default undefine test > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 @@ -38,12 +39,12 @@ error: internal error Domain 'test' is still running EOF compare exp out || fail=1
-# A different diagnostic when specifying a domain ID +# A similar diagnostic when specifying a domain ID $abs_top_builddir/tools/virsh -q -c test:///default undefine 1 > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: a running domain like 1 cannot be undefined; -to undefine, first shutdown then undefine using its name or UUID +error: Failed to undefine domain 1 +error: internal error Domain 'test' is still running EOF compare exp out || fail=1
diff --git a/tools/virsh.c b/tools/virsh.c index b0d43ba..371e010 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1431,7 +1431,6 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; bool ret = true; const char *name = NULL; - int id; int flags = 0; int managed_save = vshCommandOptBool(cmd, "managed-save"); int has_managed_save = 0; @@ -1449,19 +1448,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "domain", &name) <= 0) return false;
- if (name && virStrToLong_i(name, NULL, 10, &id) == 0 - && id >= 0 && (dom = virDomainLookupByID(ctl->conn, id))) { - vshError(ctl, - _("a running domain like %s cannot be undefined;\n" - "to undefine, first shutdown then undefine" - " using its name or UUID"), - name); - virDomainFree(dom); - return false; - } - if (!(dom = vshCommandOptDomainBy(ctl, cmd, &name, - VSH_BYNAME|VSH_BYUUID))) - return false; + if (!(dom = vshCommandOptDomain(ctl, cmd, &name)))
has_managed_save = virDomainHasManagedSaveImage(dom, 0); if (has_managed_save < 0) {
ACK, it also fix the lookup since we are sure to have a name, maybe that should go in the commit log Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/11/2011 09:11 PM, Daniel Veillard wrote:
On Thu, Aug 11, 2011 at 08:39:22PM -0600, Eric Blake wrote:
The public API documents that undefine may be used to transition a running persistent domain into a transient one. Many drivers still do not support this usage, but virsh shouldn't be getting in the way of those that do support it.
@@ -1449,19 +1448,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "domain",&name)<= 0) return false;
This guarantees name is non-NULL,
- if (name&& virStrToLong_i(name, NULL, 10,&id) == 0
so yes, this was a redundant check
ACK, it also fix the lookup since we are sure to have a name, maybe that should go in the commit log
Pushed with a slight commit log tweak. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel Veillard -
Eric Blake