[libvirt] [PATCH 0/8] Introduce new API virDomainUndefineFlags
Hi, Per discussion on https://www.redhat.com/archives/libvir-list/2011-July/msg00556.html, This patch series introduce new API virDomainUndefineFlags, which only support flag VIR_DOMAIN_UNDEFINE_MANAGED_STATE now, might introduce more flags in future. If the flag is specified, the domain managed state file will be removed along with domain undefine process once it exists, the entire domain undefine process will fail if it fails on removing the managed state file. [PATCH 1/8] is small fix on rpc generator scripts, not related with the new API. [PATCH 1/8] Small fixes on rpc generator scripts [PATCH 2/8] UndefineFlags: Define the public API [PATCH 3/8] UndefineFlags: Implement the public API [PATCH 4/8] UndefineFlags: Define the internal API [PATCH 5/8] UndefineFlags: Implement qemu driver handling [PATCH 6/8] UndefineFlags: Implement libxl driver handling [PATCH 7/8] UndefineFlags: Implement the remote protocol [PATCH 8/8] UndefineFlags: Extend virsh undefine to support new flag Regards Osier
1. Fix typos caused by the renaming 2. It should substitute "remote/remote_protocol.h" to "remote_protocol.h". --- src/rpc/gendispatch.pl | 6 +++--- src/rpc/genprotocol.pl | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index c69c5a2..39d0ce6 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -9,8 +9,8 @@ # for both remote_protocol.x and qemu_protocol.x, you would run the # following: # -# remote_generator.pl -c -t remote ../src/remote/remote_protocol.x -# remote_generator.pl -t qemu ../src/remote/qemu_protocol.x +# gendispatch.pl -c -t remote ../src/remote/remote_protocol.x +# gendispatch.pl -t qemu ../src/remote/qemu_protocol.x # # By Richard Jones <rjones@redhat.com> # Extended by Matthias Bolte <matthias.bolte@googlemail.com> @@ -239,7 +239,7 @@ sub hyper_to_long # Output print <<__EOF__; -/* Automatically generated by remote_generator.pl. +/* Automatically generated by gendispatch.pl. * Do not edit this file. Any changes you make will be lost. */ __EOF__ diff --git a/src/rpc/genprotocol.pl b/src/rpc/genprotocol.pl index 4edba98..7124e5c 100755 --- a/src/rpc/genprotocol.pl +++ b/src/rpc/genprotocol.pl @@ -59,7 +59,7 @@ while (<RPCGEN>) { s/xdr_u_quad_t/xdr_uint64_t/g; s/xdr_quad_t/xdr_int64_t/g; s/(?<!IXDR_GET_INT32 )IXDR_GET_LONG/IXDR_GET_INT32/g; - s,#include "\./remote/remote_protocol\.h",#include "remote_protocol.h",; + s,#include "remote/remote_protocol\.h",#include "remote_protocol.h",; if (m/^}/) { $in_function = 0; -- 1.7.6
On 07/13/2011 04:19 AM, Osier Yang wrote:
1. Fix typos caused by the renaming 2. It should substitute "remote/remote_protocol.h" to "remote_protocol.h". --- src/rpc/gendispatch.pl | 6 +++--- src/rpc/genprotocol.pl | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
ACK.
+++ b/src/rpc/genprotocol.pl @@ -59,7 +59,7 @@ while (<RPCGEN>) { s/xdr_u_quad_t/xdr_uint64_t/g; s/xdr_quad_t/xdr_int64_t/g; s/(?<!IXDR_GET_INT32 )IXDR_GET_LONG/IXDR_GET_INT32/g; - s,#include "\./remote/remote_protocol\.h",#include "remote_protocol.h",; + s,#include "remote/remote_protocol\.h",#include "remote_protocol.h",;
I wonder if we could even go one step further in using: s,#include ".*remote/remote_protocol\.h",#include "remote_protocol.h",; but as written, this was okay, and things compiled fine both before and after this change. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Only support to remove domain managed state file when undefining the domain currently. --- include/libvirt/libvirt.h.in | 10 ++++++++++ python/generator.py | 1 + src/libvirt_public.syms | 5 +++++ 3 files changed, 16 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d5a7105..98f1454 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1200,6 +1200,16 @@ int virDomainMemoryPeek (virDomainPtr dom, virDomainPtr virDomainDefineXML (virConnectPtr conn, const char *xml); int virDomainUndefine (virDomainPtr domain); + +/* Domain undefine flags */ +typedef enum { + VIR_DOMAIN_UNDEFINE_MANAGED_STATE = 1, + + /* Future undefine control flags should come here */ +} virDomainUndefineFlags; + +int virDomainUndefineWithFlags (virDomainPtr domain, + unsigned int flags); int virConnectNumOfDefinedDomains (virConnectPtr conn); int virConnectListDefinedDomains (virConnectPtr conn, char **const names, diff --git a/python/generator.py b/python/generator.py index c27ff73..0510ec0 100755 --- a/python/generator.py +++ b/python/generator.py @@ -366,6 +366,7 @@ skip_impl = ( 'virDomainSendKey', 'virNodeGetCPUStats', 'virNodeGetMemoryStats', + 'virDomainUndefineWithFlags', ) diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 5f2541a..54e0131 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -466,4 +466,9 @@ LIBVIRT_0.9.3 { virNodeGetMemoryStats; } LIBVIRT_0.9.2; +LIBVIRT_0.9.4 { + global: + virDomainUndefineWithFlags; +} LIBVIRT_0.9.3; + # .... define new API here using predicted next version number .... -- 1.7.6
On 07/13/2011 04:19 AM, Osier Yang wrote:
Only support to remove domain managed state file when undefining
s/to remove/removing/
the domain currently. --- include/libvirt/libvirt.h.in | 10 ++++++++++ python/generator.py | 1 + src/libvirt_public.syms | 5 +++++ 3 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d5a7105..98f1454 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1200,6 +1200,16 @@ int virDomainMemoryPeek (virDomainPtr dom, virDomainPtr virDomainDefineXML (virConnectPtr conn, const char *xml); int virDomainUndefine (virDomainPtr domain); + +/* Domain undefine flags */ +typedef enum { + VIR_DOMAIN_UNDEFINE_MANAGED_STATE = 1, + + /* Future undefine control flags should come here */ +} virDomainUndefineFlags; + +int virDomainUndefineWithFlags (virDomainPtr domain, + unsigned int flags);
I'm not sure I like the "WithFlags" name. We _had_ to use it on virDomainCreateWithFlags, because the public name virDomainCreateFlags was already in existence. But here, you could just as easily go with: typedef enum { ... } virDomainUndefineFlagValues; int virDomainUndefineFlags(virDomainPtr domain, unsigned int flags); and avoid the extra "With".
+++ b/python/generator.py @@ -366,6 +366,7 @@ skip_impl = ( 'virDomainSendKey', 'virNodeGetCPUStats', 'virNodeGetMemoryStats', + 'virDomainUndefineWithFlags',
Why does this have to be skipped? virDomainUndefine is not skipped, and it seems like having python bindings for this new function would be worthwhile. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月14日 01:43, Eric Blake 写道:
On 07/13/2011 04:19 AM, Osier Yang wrote:
Only support to remove domain managed state file when undefining s/to remove/removing/
the domain currently. --- include/libvirt/libvirt.h.in | 10 ++++++++++ python/generator.py | 1 + src/libvirt_public.syms | 5 +++++ 3 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index d5a7105..98f1454 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1200,6 +1200,16 @@ int virDomainMemoryPeek (virDomainPtr dom, virDomainPtr virDomainDefineXML (virConnectPtr conn, const char *xml); int virDomainUndefine (virDomainPtr domain); + +/* Domain undefine flags */ +typedef enum { + VIR_DOMAIN_UNDEFINE_MANAGED_STATE = 1, + + /* Future undefine control flags should come here */ +} virDomainUndefineFlags; + +int virDomainUndefineWithFlags (virDomainPtr domain, + unsigned int flags); I'm not sure I like the "WithFlags" name. We _had_ to use it on virDomainCreateWithFlags, because the public name virDomainCreateFlags was already in existence. But here, you could just as easily go with:
typedef enum { ... } virDomainUndefineFlagValues;
int virDomainUndefineFlags(virDomainPtr domain, unsigned int flags);
and avoid the extra "With".
Yes, I didn't get a better idea of how to name the enum and the function, virDomainUndefineFlagValue seems a good idea.
+++ b/python/generator.py @@ -366,6 +366,7 @@ skip_impl = ( 'virDomainSendKey', 'virNodeGetCPUStats', 'virNodeGetMemoryStats', + 'virDomainUndefineWithFlags', Why does this have to be skipped? virDomainUndefine is not skipped, and it seems like having python bindings for this new function would be worthwhile.
Ok, this will be added in v2. Osier
--- src/libvirt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 52 insertions(+), 1 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 7e70caa..eeb25a5 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -6381,7 +6381,8 @@ error: * Returns 0 in case of success, -1 in case of error */ int -virDomainUndefine(virDomainPtr domain) { +virDomainUndefine(virDomainPtr domain) +{ virConnectPtr conn; VIR_DOMAIN_DEBUG(domain); @@ -6415,6 +6416,56 @@ error: } /** + * virDomainUndefineWithFlags: + * @domain: pointer to a defined domain + * @flags: bitwise-or of supported virDomainUndefineFlags + * + * Undefine a domain but does not stop it if it is running + * + * If VIR_DOMAIN_UNDEFINE_MANAGED_STATE is specified, the domain + * managed state file will be removed along with domain undefine + * process, the entire domain undefine process will fail if + * it fails on removing the managed state file. + * + * Returns 0 in case of success, -1 in case of error + */ +int +virDomainUndefineWithFlags(virDomainPtr domain, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "flags=%x", flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + conn = domain->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->driver->domainUndefineWithFlags) { + int ret; + ret = conn->driver->domainUndefineWithFlags (domain, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** * virConnectNumOfDefinedDomains: * @conn: pointer to the hypervisor connection * -- 1.7.6
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- src/libvirt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 52 insertions(+), 1 deletions(-)
per my comments in 2/8,
/** + * virDomainUndefineWithFlags:
s/virDomainUndefineWithFlags/virDomainUndefineFlags/
+ * @domain: pointer to a defined domain + * @flags: bitwise-or of supported virDomainUndefineFlags
s/virDomainUndefineFlags/virDomainUndefineFlagValues/
+ * + * Undefine a domain but does not stop it if it is running
Copy and paste from the existing virDomainUndefine, but I think it would read better (in both places) as: Undefine a persistent domain. A running domain is left running but converted into a transient domain.
+ * + * If VIR_DOMAIN_UNDEFINE_MANAGED_STATE is specified, the domain
s/the/any/ - since a managed state file might not exist
+ * managed state file will be removed along with domain undefine + * process, the entire domain undefine process will fail if
s/process, the/process, and the/
+ * it fails on removing the managed state file.
We also need to clearly document what happens if a managed state file exists but the flag is not present. My preference would be rewording this entire paragraph as: If the domain has a managed state file (see virDomainHasManagedSaveImage()), then including VIR_DOMAIN_UNDEFINE_MANAGED_STATE in @flags will also remove that file, and omitting the flag will cause the undefine process to fail. and back at virDomainUndefine, add a paragraph: If the domain has a managed state file (see virDomainHasManagedSaveImage()), then the undefine will fail. See virDomainUndefineFlags() for more control. The code additions look fine except for the choice of API name, but the documentation is just as important, so this needs a v2. Also, I'd probably squash patch 2 and 3 into one patch - that is, it would be nice to add both the declaration and the documentation of the API in the same patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月14日 01:53, Eric Blake 写道:
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- src/libvirt.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 52 insertions(+), 1 deletions(-)
per my comments in 2/8,
/** + * virDomainUndefineWithFlags: s/virDomainUndefineWithFlags/virDomainUndefineFlags/
+ * @domain: pointer to a defined domain + * @flags: bitwise-or of supported virDomainUndefineFlags
s/virDomainUndefineFlags/virDomainUndefineFlagValues/
+ * + * Undefine a domain but does not stop it if it is running Copy and paste from the existing virDomainUndefine, but I think it would read better (in both places) as:
Undefine a persistent domain. A running domain is left running but converted into a transient domain.
+ * + * If VIR_DOMAIN_UNDEFINE_MANAGED_STATE is specified, the domain s/the/any/ - since a managed state file might not exist
+ * managed state file will be removed along with domain undefine + * process, the entire domain undefine process will fail if s/process, the/process, and the/
+ * it fails on removing the managed state file. We also need to clearly document what happens if a managed state file exists but the flag is not present. My preference would be rewording this entire paragraph as:
If the domain has a managed state file (see virDomainHasManagedSaveImage()), then including VIR_DOMAIN_UNDEFINE_MANAGED_STATE in @flags will also remove that file, and omitting the flag will cause the undefine process to fail.
and back at virDomainUndefine, add a paragraph:
If the domain has a managed state file (see virDomainHasManagedSaveImage()), then the undefine will fail. See virDomainUndefineFlags() for more control.
The code additions look fine except for the choice of API name, but the documentation is just as important, so this needs a v2. Also, I'd probably squash patch 2 and 3 into one patch - that is, it would be nice to add both the declaration and the documentation of the API in the same patch.
This is nice document, but is it good to change behaviour of virDomainUndefine? (fails if it has a managed state file) Osier
--- src/driver.h | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/src/driver.h b/src/driver.h index 71a52c9..3db9fb7 100644 --- a/src/driver.h +++ b/src/driver.h @@ -219,6 +219,9 @@ typedef virDomainPtr typedef int (*virDrvDomainUndefine) (virDomainPtr dom); typedef int + (*virDrvDomainUndefineWithFlags) (virDomainPtr dom, + unsigned int flags); +typedef int (*virDrvDomainSetVcpus) (virDomainPtr domain, unsigned int nvcpus); typedef int @@ -727,6 +730,7 @@ struct _virDriver { virDrvDomainCreateWithFlags domainCreateWithFlags; virDrvDomainDefineXML domainDefineXML; virDrvDomainUndefine domainUndefine; + virDrvDomainUndefineWithFlags domainUndefineWithFlags; virDrvDomainAttachDevice domainAttachDevice; virDrvDomainAttachDeviceFlags domainAttachDeviceFlags; virDrvDomainDetachDevice domainDetachDevice; -- 1.7.6
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- src/driver.h | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/driver.h b/src/driver.h index 71a52c9..3db9fb7 100644 --- a/src/driver.h +++ b/src/driver.h @@ -219,6 +219,9 @@ typedef virDomainPtr typedef int (*virDrvDomainUndefine) (virDomainPtr dom); typedef int + (*virDrvDomainUndefineWithFlags) (virDomainPtr dom, + unsigned int flags);
This needs to reflect whatever naming we have in 2/8. Conditional ACK once we agree on the API name. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
--- src/qemu/qemu_driver.c | 31 +++++++++++++++++++++++++++---- 1 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f962e2c..a9f6986 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2402,10 +2402,8 @@ cleanup: static char * qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) { char *ret; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, uuidstr) < 0) { + if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) { virReportOOMError(); return(NULL); } @@ -4283,11 +4281,16 @@ cleanup: return dom; } -static int qemudDomainUndefine(virDomainPtr dom) { +static int qemudDomainUndefineWithFlags(virDomainPtr dom, + unsigned int flags) +{ struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; virDomainEventPtr event = NULL; int ret = -1; + char *name = NULL; + + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_STATE, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4312,6 +4315,20 @@ static int qemudDomainUndefine(virDomainPtr dom) { goto cleanup; } + if (flags & VIR_DOMAIN_UNDEFINE_MANAGED_STATE) { + name = qemuDomainManagedSavePath(driver, vm); + + if (name == NULL) + goto cleanup; + + if (virFileExists(name) && (unlink(name) < 0)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("failed on removing domain managed state " + "file '%s'"), name); + goto cleanup; + } + } + if (virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm) < 0) goto cleanup; @@ -4326,6 +4343,7 @@ static int qemudDomainUndefine(virDomainPtr dom) { ret = 0; cleanup: + VIR_FREE(name); if (vm) virDomainObjUnlock(vm); if (event) @@ -4334,6 +4352,10 @@ cleanup: return ret; } +static int qemudDomainUndefine(virDomainPtr dom) { + return qemudDomainUndefineWithFlags(dom, 0); +} + static int qemuDomainAttachDeviceDiskLive(struct qemud_driver *driver, virDomainObjPtr vm, @@ -8487,6 +8509,7 @@ static virDriver qemuDriver = { .domainCreateWithFlags = qemudDomainStartWithFlags, /* 0.8.2 */ .domainDefineXML = qemudDomainDefine, /* 0.2.0 */ .domainUndefine = qemudDomainUndefine, /* 0.2.0 */ + .domainUndefineWithFlags = qemudDomainUndefineWithFlags, /* 0.9.4 */ .domainAttachDevice = qemuDomainAttachDevice, /* 0.4.1 */ .domainAttachDeviceFlags = qemuDomainAttachDeviceFlags, /* 0.7.7 */ .domainDetachDevice = qemuDomainDetachDevice, /* 0.5.0 */ -- 1.7.6
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- src/qemu/qemu_driver.c | 31 +++++++++++++++++++++++++++---- 1 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f962e2c..a9f6986 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2402,10 +2402,8 @@ cleanup: static char * qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) { char *ret; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr);
- if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, uuidstr) < 0) { + if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) {
This hunk won't apply to existing libvirt.git. Did you forget to rebase out your first attempt?
@@ -4312,6 +4315,20 @@ static int qemudDomainUndefine(virDomainPtr dom) { goto cleanup; }
+ if (flags & VIR_DOMAIN_UNDEFINE_MANAGED_STATE) { + name = qemuDomainManagedSavePath(driver, vm); + + if (name == NULL) + goto cleanup; + + if (virFileExists(name) && (unlink(name) < 0)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("failed on removing domain managed state " + "file '%s'"), name); + goto cleanup; + } + }
I think we need to explicitly reject attempts to undefine a domain while a state file exists. That is, I think this logic needs to be: name = qemuDomainManagedSavePath(driver, vm); if (name == NULL) goto cleanup; if (virFileExists(name)) { if (flags & VIR_DOMAIN_UNDEFINE_MANAGED_STATE) { if (unlink(name) < 0) { error - failed to remove } } else { error - refusing to undefine with managed state file present } } Yes, it will change existing behavior (previously, you could undefine a domain and leave the state file behind), but that was unsafe, and this is arguably a bug fix. The default should be as safe as possible, with the flags (VIR_DOMAIN_UNDEFINE_MANAGED_STATE) in place to make things faster.
@@ -8487,6 +8509,7 @@ static virDriver qemuDriver = { .domainCreateWithFlags = qemudDomainStartWithFlags, /* 0.8.2 */ .domainDefineXML = qemudDomainDefine, /* 0.2.0 */ .domainUndefine = qemudDomainUndefine, /* 0.2.0 */ + .domainUndefineWithFlags = qemudDomainUndefineWithFlags, /* 0.9.4 */
Also, I think that for every hypervisor that supports domainUndefine, we should add a (trivial) domainUndefine[With]Flags wrapper, so that the new API is available everywhere in 0.9.4, but do that as a separate patch. I'm about to post a series to add virDomainSaveFlags, which might serve as an example for what I'm thinking about. That series will add a no-semantic-change wrapper in the first commit, and only later changes the qemu wrapper to learn to honor a new flag value. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月14日 02:01, Eric Blake 写道:
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- src/qemu/qemu_driver.c | 31 +++++++++++++++++++++++++++---- 1 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f962e2c..a9f6986 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2402,10 +2402,8 @@ cleanup: static char * qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) { char *ret; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr);
- if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, uuidstr)< 0) { + if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name)< 0) { This hunk won't apply to existing libvirt.git. Did you forget to rebase out your first attempt?
Ah, yes.
@@ -4312,6 +4315,20 @@ static int qemudDomainUndefine(virDomainPtr dom) { goto cleanup; }
+ if (flags& VIR_DOMAIN_UNDEFINE_MANAGED_STATE) { + name = qemuDomainManagedSavePath(driver, vm); + + if (name == NULL) + goto cleanup; + + if (virFileExists(name)&& (unlink(name)< 0)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("failed on removing domain managed state " + "file '%s'"), name); + goto cleanup; + } + } I think we need to explicitly reject attempts to undefine a domain while a state file exists. That is, I think this logic needs to be:
name = qemuDomainManagedSavePath(driver, vm); if (name == NULL) goto cleanup; if (virFileExists(name)) { if (flags& VIR_DOMAIN_UNDEFINE_MANAGED_STATE) { if (unlink(name)< 0) { error - failed to remove } } else { error - refusing to undefine with managed state file present } }
Yes, it will change existing behavior (previously, you could undefine a domain and leave the state file behind), but that was unsafe, and this is arguably a bug fix. The default should be as safe as possible, with the flags (VIR_DOMAIN_UNDEFINE_MANAGED_STATE) in place to make things faster.
This answer my question in 3/8
@@ -8487,6 +8509,7 @@ static virDriver qemuDriver = { .domainCreateWithFlags = qemudDomainStartWithFlags, /* 0.8.2 */ .domainDefineXML = qemudDomainDefine, /* 0.2.0 */ .domainUndefine = qemudDomainUndefine, /* 0.2.0 */ + .domainUndefineWithFlags = qemudDomainUndefineWithFlags, /* 0.9.4 */ Also, I think that for every hypervisor that supports domainUndefine, we should add a (trivial) domainUndefine[With]Flags wrapper, so that the new API is available everywhere in 0.9.4, but do that as a separate patch.
Agree, even if other hypervisors except libxl and qemu don't support managed saving.
I'm about to post a series to add virDomainSaveFlags, which might serve as an example for what I'm thinking about. That series will add a no-semantic-change wrapper in the first commit, and only later changes the qemu wrapper to learn to honor a new flag value.
--- src/libxl/libxl_driver.c | 32 ++++++++++++++++++++++++++++---- 1 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 808480f..f04931b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -220,10 +220,8 @@ libxlDoNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) static char * libxlDomainManagedSavePath(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { char *ret; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr); - if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, uuidstr) < 0) { + if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) { virReportOOMError(); return NULL; } @@ -2716,13 +2714,17 @@ cleanup: } static int -libxlDomainUndefine(virDomainPtr dom) +libxlDomainUndefineWithFlags(virDomainPtr dom, + unsigned int flags) { libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; virDomainEventPtr event = NULL; + char *name = NULL; int ret = -1; + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_STATE, -1); + libxlDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -2746,6 +2748,20 @@ libxlDomainUndefine(virDomainPtr dom) "%s", _("cannot undefine transient domain")); goto cleanup; } + + if (flags & VIR_DOMAIN_UNDEFINE_MANAGED_STATE) { + name = libxlDomainManagedSavePath(driver, vm); + + if (name == NULL) + goto cleanup; + + if (virFileExists(name) && (unlink(name) < 0)) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("failed on removing domain managed state " + "file '%s'"), name); + goto cleanup; + } + } if (virDomainDeleteConfig(driver->configDir, driver->autostartDir, @@ -2760,6 +2776,7 @@ libxlDomainUndefine(virDomainPtr dom) ret = 0; cleanup: + VIR_FREE(name); if (vm) virDomainObjUnlock(vm); if (event) @@ -2769,6 +2786,12 @@ libxlDomainUndefine(virDomainPtr dom) } static int +libxlDomainUndefine(virDomainPtr dom) +{ + return libxlDomainUndefineWithFlags(dom, 0); +} + +static int libxlDomainChangeEjectableMedia(libxlDomainObjPrivatePtr priv, virDomainObjPtr vm, virDomainDiskDefPtr disk) { @@ -3828,6 +3851,7 @@ static virDriver libxlDriver = { .domainCreateWithFlags = libxlDomainCreateWithFlags, /* 0.9.0 */ .domainDefineXML = libxlDomainDefineXML, /* 0.9.0 */ .domainUndefine = libxlDomainUndefine, /* 0.9.0 */ + .domainUndefineWithFlags = libxlDomainUndefineWithFlags, /* 0.9.4 */ .domainAttachDevice = libxlDomainAttachDevice, /* 0.9.2 */ .domainAttachDeviceFlags = libxlDomainAttachDeviceFlags, /* 0.9.2 */ .domainDetachDevice = libxlDomainDetachDevice, /* 0.9.2 */ -- 1.7.6
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- src/libxl/libxl_driver.c | 32 ++++++++++++++++++++++++++++---- 1 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 808480f..f04931b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -220,10 +220,8 @@ libxlDoNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) static char * libxlDomainManagedSavePath(libxlDriverPrivatePtr driver, virDomainObjPtr vm) { char *ret; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - virUUIDFormat(vm->def->uuid, uuidstr);
- if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, uuidstr) < 0) { + if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) {
Same rebase problem as in 5/8.
+ + if (flags & VIR_DOMAIN_UNDEFINE_MANAGED_STATE) { + name = libxlDomainManagedSavePath(driver, vm); + + if (name == NULL) + goto cleanup; + + if (virFileExists(name) && (unlink(name) < 0)) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("failed on removing domain managed state " + "file '%s'"), name); + goto cleanup; + } + }
Same logic problem as in 5/8. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
With small typo fixes (s/remote_generator/gendispatch/, and s/remote_internal/remote_driver/) --- src/remote/qemu_protocol.x | 2 +- src/remote/remote_driver.c | 5 +++-- src/remote/remote_protocol.x | 14 ++++++++++---- src/remote_protocol-structs | 4 ++++ 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/remote/qemu_protocol.x b/src/remote/qemu_protocol.x index fa453f4..9381f7c 100644 --- a/src/remote/qemu_protocol.x +++ b/src/remote/qemu_protocol.x @@ -43,7 +43,7 @@ const QEMU_PROTOCOL_VERSION = 1; enum qemu_procedure { /* Each function must have a two-word comment. The first word is - * whether remote_generator.pl handles daemon, the second whether + * whether gendispatch.pl handles daemon, the second whether * it handles src/remote. */ QEMU_PROC_MONITOR_COMMAND = 1 /* skipgen skipgen */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 5c0457e..dafb04a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1,5 +1,5 @@ /* - * remote_internal.c: driver to provide access to libvirtd running + * remote_driver.c: driver to provide access to libvirtd running * on a remote machine * * Copyright (C) 2007-2011 Red Hat, Inc. @@ -4177,6 +4177,7 @@ static virDriver remote_driver = { .domainCreateWithFlags = remoteDomainCreateWithFlags, /* 0.8.2 */ .domainDefineXML = remoteDomainDefineXML, /* 0.3.0 */ .domainUndefine = remoteDomainUndefine, /* 0.3.0 */ + .domainUndefineWithFlags = remoteDomainUndefineWithFlags, /* 0.9.4 */ .domainAttachDevice = remoteDomainAttachDevice, /* 0.3.0 */ .domainAttachDeviceFlags = remoteDomainAttachDeviceFlags, /* 0.7.7 */ .domainDetachDevice = remoteDomainDetachDevice, /* 0.3.0 */ @@ -4244,7 +4245,7 @@ static virDriver remote_driver = { .domainMigratePerform3 = remoteDomainMigratePerform3, /* 0.9.2 */ .domainMigrateFinish3 = remoteDomainMigrateFinish3, /* 0.9.2 */ .domainMigrateConfirm3 = remoteDomainMigrateConfirm3, /* 0.9.2 */ - .domainSendKey = remoteDomainSendKey, /* 0.9.3 */ + .domainSendKey = remoteDomainSendKey /* 0.9.3 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index ee169fd..3adaca8 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -339,7 +339,7 @@ struct remote_node_get_memory_stats { * connection). Errors are returned implicitly in the RPC protocol. * * Please follow the naming convention carefully - this file is - * parsed by 'remote_generator.pl'. + * parsed by 'gendispatch.pl'. * * 'remote_CALL_ret' members that are filled via call-by-reference must be * annotated with a insert@<offset> comment to indicate the offset in the @@ -848,6 +848,11 @@ struct remote_domain_undefine_args { remote_nonnull_domain dom; }; +struct remote_domain_undefine_with_flags_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + struct remote_domain_inject_nmi_args { remote_nonnull_domain dom; unsigned int flags; @@ -2123,7 +2128,7 @@ const REMOTE_PROTOCOL_VERSION = 1; enum remote_procedure { /* Each function must have a two-word comment. The first word is - * whether remote_generator.pl handles daemon, the second whether + * whether gendispatch.pl handles daemon, the second whether * it handles src/remote. Additional flags can be specified after a * pipe. * @@ -2383,14 +2388,15 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_CPU_STATS = 227, /* skipgen skipgen */ REMOTE_PROC_NODE_GET_MEMORY_STATS = 228, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_GET_CONTROL_INFO = 229, /* autogen autogen */ - REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_UNDEFINE_WITH_FLAGS = 231 /* autogen autogen */ /* * Notice how the entries are grouped in sets of 10 ? * Nice isn't it. Please keep it this way when adding more. * * Each function must have a two-word comment. The first word is - * whether remote_generator.pl handles daemon, the second whether + * whether gendispatch.pl handles daemon, the second whether * it handles src/remote. Additional flags can be specified after a * pipe. * diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index b2de8e9..c8fed2c 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -533,6 +533,10 @@ struct remote_domain_define_xml_ret { struct remote_domain_undefine_args { remote_nonnull_domain dom; }; +struct remote_domain_undefine_with_flags_args { + remote_nonnull_domain dom; + u_int flags; +}; struct remote_domain_inject_nmi_args { remote_nonnull_domain dom; u_int flags; -- 1.7.6
On 07/13/2011 04:19 AM, Osier Yang wrote:
With small typo fixes (s/remote_generator/gendispatch/, and s/remote_internal/remote_driver/) --- src/remote/qemu_protocol.x | 2 +- src/remote/remote_driver.c | 5 +++-- src/remote/remote_protocol.x | 14 ++++++++++---- src/remote_protocol-structs | 4 ++++ 4 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/src/remote/qemu_protocol.x b/src/remote/qemu_protocol.x index fa453f4..9381f7c 100644 --- a/src/remote/qemu_protocol.x +++ b/src/remote/qemu_protocol.x @@ -43,7 +43,7 @@ const QEMU_PROTOCOL_VERSION = 1;
enum qemu_procedure { /* Each function must have a two-word comment. The first word is - * whether remote_generator.pl handles daemon, the second whether + * whether gendispatch.pl handles daemon, the second whether
If you haven't already pushed 1/8, then I'd squash these typo fixes into that patch.
@@ -4177,6 +4177,7 @@ static virDriver remote_driver = { .domainCreateWithFlags = remoteDomainCreateWithFlags, /* 0.8.2 */ .domainDefineXML = remoteDomainDefineXML, /* 0.3.0 */ .domainUndefine = remoteDomainUndefine, /* 0.3.0 */ + .domainUndefineWithFlags = remoteDomainUndefineWithFlags, /* 0.9.4 */
May reflect any API renaming.
.domainAttachDevice = remoteDomainAttachDevice, /* 0.3.0 */ .domainAttachDeviceFlags = remoteDomainAttachDeviceFlags, /* 0.7.7 */ .domainDetachDevice = remoteDomainDetachDevice, /* 0.3.0 */ @@ -4244,7 +4245,7 @@ static virDriver remote_driver = { .domainMigratePerform3 = remoteDomainMigratePerform3, /* 0.9.2 */ .domainMigrateFinish3 = remoteDomainMigrateFinish3, /* 0.9.2 */ .domainMigrateConfirm3 = remoteDomainMigrateConfirm3, /* 0.9.2 */ - .domainSendKey = remoteDomainSendKey, /* 0.9.3 */ + .domainSendKey = remoteDomainSendKey /* 0.9.3 */
Spurious change. Leave the trailing comma here.
+++ b/src/remote/remote_protocol.x @@ -339,7 +339,7 @@ struct remote_node_get_memory_stats { * connection). Errors are returned implicitly in the RPC protocol. * * Please follow the naming convention carefully - this file is - * parsed by 'remote_generator.pl'. + * parsed by 'gendispatch.pl'.
Another typo fix to squash into 1/8.
@@ -2383,14 +2388,15 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_CPU_STATS = 227, /* skipgen skipgen */ REMOTE_PROC_NODE_GET_MEMORY_STATS = 228, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_GET_CONTROL_INFO = 229, /* autogen autogen */ - REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_UNDEFINE_WITH_FLAGS = 231 /* autogen autogen */
/* * Notice how the entries are grouped in sets of 10 ? * Nice isn't it. Please keep it this way when adding more.
Oops - you missed the blank line that this comment asked for. Conditional ACK once you split out the typo changes and account for any API rename. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月14日 02:05, Eric Blake 写道:
With small typo fixes (s/remote_generator/gendispatch/, and s/remote_internal/remote_driver/) --- src/remote/qemu_protocol.x | 2 +- src/remote/remote_driver.c | 5 +++-- src/remote/remote_protocol.x | 14 ++++++++++---- src/remote_protocol-structs | 4 ++++ 4 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/src/remote/qemu_protocol.x b/src/remote/qemu_protocol.x index fa453f4..9381f7c 100644 --- a/src/remote/qemu_protocol.x +++ b/src/remote/qemu_protocol.x @@ -43,7 +43,7 @@ const QEMU_PROTOCOL_VERSION = 1;
enum qemu_procedure { /* Each function must have a two-word comment. The first word is - * whether remote_generator.pl handles daemon, the second whether + * whether gendispatch.pl handles daemon, the second whether If you haven't already pushed 1/8, then I'd squash these typo fixes into
On 07/13/2011 04:19 AM, Osier Yang wrote: that patch.
@@ -4177,6 +4177,7 @@ static virDriver remote_driver = { .domainCreateWithFlags = remoteDomainCreateWithFlags, /* 0.8.2 */ .domainDefineXML = remoteDomainDefineXML, /* 0.3.0 */ .domainUndefine = remoteDomainUndefine, /* 0.3.0 */ + .domainUndefineWithFlags = remoteDomainUndefineWithFlags, /* 0.9.4 */ May reflect any API renaming.
.domainAttachDevice = remoteDomainAttachDevice, /* 0.3.0 */ .domainAttachDeviceFlags = remoteDomainAttachDeviceFlags, /* 0.7.7 */ .domainDetachDevice = remoteDomainDetachDevice, /* 0.3.0 */ @@ -4244,7 +4245,7 @@ static virDriver remote_driver = { .domainMigratePerform3 = remoteDomainMigratePerform3, /* 0.9.2 */ .domainMigrateFinish3 = remoteDomainMigrateFinish3, /* 0.9.2 */ .domainMigrateConfirm3 = remoteDomainMigrateConfirm3, /* 0.9.2 */ - .domainSendKey = remoteDomainSendKey, /* 0.9.3 */ + .domainSendKey = remoteDomainSendKey /* 0.9.3 */
Spurious change. Leave the trailing comma here.
Oops, yes.
+++ b/src/remote/remote_protocol.x @@ -339,7 +339,7 @@ struct remote_node_get_memory_stats { * connection). Errors are returned implicitly in the RPC protocol. * * Please follow the naming convention carefully - this file is - * parsed by 'remote_generator.pl'. + * parsed by 'gendispatch.pl'. Another typo fix to squash into 1/8.
@@ -2383,14 +2388,15 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_CPU_STATS = 227, /* skipgen skipgen */ REMOTE_PROC_NODE_GET_MEMORY_STATS = 228, /* skipgen skipgen */ REMOTE_PROC_DOMAIN_GET_CONTROL_INFO = 229, /* autogen autogen */ - REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_UNDEFINE_WITH_FLAGS = 231 /* autogen autogen */
/* * Notice how the entries are grouped in sets of 10 ? * Nice isn't it. Please keep it this way when adding more. Oops - you missed the blank line that this comment asked for.
Conditional ACK once you split out the typo changes and account for any API rename.
--- tools/virsh.c | 12 +++++++++++- tools/virsh.pod | 6 +++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3cdf043..f81e923 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1409,6 +1409,7 @@ static const vshCmdInfo info_undefine[] = { static const vshCmdOptDef opts_undefine[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")}, + {"undefine-managed-state", VSH_OT_BOOL, 0, N_("remove domain managed state file")}, {NULL, 0, 0, NULL} }; @@ -1419,6 +1420,14 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool ret = true; const char *name = NULL; int id; + int flags = 0; + int undefine_managed_state = vshCommandOptBool(cmd, "undefine-managed-state"); + + if (undefine_managed_state) + flags |= VIR_DOMAIN_UNDEFINE_MANAGED_STATE; + + if (!undefine_managed_state) + flags = -1; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1440,7 +1449,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) VSH_BYNAME|VSH_BYUUID))) return false; - if (virDomainUndefine(dom) == 0) { + if (((flags == -1) ? virDomainUndefine(dom) : + virDomainUndefineWithFlags(dom, flags)) == 0) { vshPrint(ctl, _("Domain %s has been undefined\n"), name); } else { vshError(ctl, _("Failed to undefine domain %s"), name); diff --git a/tools/virsh.pod b/tools/virsh.pod index 8b820d2..393d014 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -804,11 +804,15 @@ hypervisor. Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1. -=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state> Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>. +If I<--undefine-managed-state> is specified, the managed state file will +be removed along with the domain undefine peocess, the entire domain +undefine process will fail if it fails on removing the managed state file. + =item B<vcpucount> I<domain-id> optional I<--maximum> I<--current> I<--config> I<--live> -- 1.7.6
On Wed, Jul 13, 2011 at 06:19:44PM +0800, Osier Yang wrote:
--- tools/virsh.c | 12 +++++++++++- tools/virsh.pod | 6 +++++- 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 3cdf043..f81e923 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1409,6 +1409,7 @@ static const vshCmdInfo info_undefine[] = {
static const vshCmdOptDef opts_undefine[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")}, + {"undefine-managed-state", VSH_OT_BOOL, 0, N_("remove domain managed state file")},
IMO, it would be clearer to call this option remove-managed-state. Dave
{NULL, 0, 0, NULL} };
@@ -1419,6 +1420,14 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool ret = true; const char *name = NULL; int id; + int flags = 0; + int undefine_managed_state = vshCommandOptBool(cmd, "undefine-managed-state"); + + if (undefine_managed_state) + flags |= VIR_DOMAIN_UNDEFINE_MANAGED_STATE; + + if (!undefine_managed_state) + flags = -1;
if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1440,7 +1449,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) VSH_BYNAME|VSH_BYUUID))) return false;
- if (virDomainUndefine(dom) == 0) { + if (((flags == -1) ? virDomainUndefine(dom) : + virDomainUndefineWithFlags(dom, flags)) == 0) { vshPrint(ctl, _("Domain %s has been undefined\n"), name); } else { vshError(ctl, _("Failed to undefine domain %s"), name); diff --git a/tools/virsh.pod b/tools/virsh.pod index 8b820d2..393d014 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -804,11 +804,15 @@ hypervisor. Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1.
-=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state>
Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>.
+If I<--undefine-managed-state> is specified, the managed state file will +be removed along with the domain undefine peocess, the entire domain +undefine process will fail if it fails on removing the managed state file. + =item B<vcpucount> I<domain-id> optional I<--maximum> I<--current> I<--config> I<--live>
-- 1.7.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- tools/virsh.c | 12 +++++++++++- tools/virsh.pod | 6 +++++- 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 3cdf043..f81e923 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1409,6 +1409,7 @@ static const vshCmdInfo info_undefine[] = {
static const vshCmdOptDef opts_undefine[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")}, + {"undefine-managed-state", VSH_OT_BOOL, 0, N_("remove domain managed state file")},
Dave suggested '--remove-managed-state', but given the name of our existing virsh command: managedsave-remove I'd feel slightly better with: virsh undefine domain --managed-save [Meanwhile, we don't have any virsh command that directly exposes virDomainHasManagedSaveImage - that information should probably be added as a flag to an existing command, perhaps 'virsh list --managed-save' adding a column on whether a managed save image exists for each domain.]
+ int flags = 0; + int undefine_managed_state = vshCommandOptBool(cmd, "undefine-managed-state"); + + if (undefine_managed_state) + flags |= VIR_DOMAIN_UNDEFINE_MANAGED_STATE; + + if (!undefine_managed_state) + flags = -1;
if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1440,7 +1449,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) VSH_BYNAME|VSH_BYUUID))) return false;
- if (virDomainUndefine(dom) == 0) { + if (((flags == -1) ? virDomainUndefine(dom) : + virDomainUndefineWithFlags(dom, flags)) == 0) {
Most times when we add a new flag to virsh, we can't support it without using the new API. But here, we can, and so I think we should. That is, if you virsh 0.9.4 as the client, and are talking to two different servers (one 0.9.3 and one 0.9.4), you get the following behavior for a domain that has a managed save image: server 0.9.3: virsh undefine dom -> call virDomainHasManagedSaveImage - if that is true, then refuse to run virDomainUndefine (thus papering over the 0.9.3 safety bug). If false, then virDomainUndefine is safe. virsh undefine dom --managed-save -> tries virDomainUndefineFlags, which fails with unimplemented. Falls back to combo virDomainManagedSaveRemove/virDomainUndefine, which does the job we want. server 0.9.4 virsh undefine dom -> call virDomainHasManagedSaveImage - if that is true, then refuse to run virDomainUndefine. If false, then virDomainUndefine is safe. virsh undefine dom --managed-save -> tries virDomainUndefineFlags, which succeeds and does the job we want (or fails, but with an error different than unimplemented) an alternative would be that calling 'virsh undefine dom' without flags skips virDomainHasManagedSaveImage, and just blindly calls virDomainUndefine - in that case, a server at 0.9.4 would properly fail, but a server at 0.9.3 would still expose the bug that we are trying to avoid of leaving stale managed state behind. For precedence in making virsh try a fallback to older API when the new API is unsupported, see the setvcpus command.
+++ b/tools/virsh.pod @@ -804,11 +804,15 @@ hypervisor. Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1.
-=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state>
Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>.
Oh, this is different than libvirt.c, which claimed you can run 'undefine' on a running persistent domain. Which is it? Can undefine make a running domain transient, or must it be on an inactive domain?
+If I<--undefine-managed-state> is specified, the managed state file will +be removed along with the domain undefine peocess, the entire domain +undefine process will fail if it fails on removing the managed state file.
Given my above thoughts (and once I validate the behavior of undefine on a running domain), I'd word this as: =item B<undefine> I<domain-id> [I<--managed-save>] Undefine the configuration for a domain. If domain is not running, the domain name or UUID must be used as the I<domain-id>; if the domain is running, then this converts a persistent domain to a transient domain. The I<--managed-save> flag guarantees that any managed state (see the B<managesave> command) is also cleaned up. Without the flag, attempts to undefine a domain with managed state will fail. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月14日 02:23, Eric Blake 写道:
On 07/13/2011 04:19 AM, Osier Yang wrote:
--- tools/virsh.c | 12 +++++++++++- tools/virsh.pod | 6 +++++- 2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 3cdf043..f81e923 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1409,6 +1409,7 @@ static const vshCmdInfo info_undefine[] = {
static const vshCmdOptDef opts_undefine[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name or uuid")}, + {"undefine-managed-state", VSH_OT_BOOL, 0, N_("remove domain managed state file")}, Dave suggested '--remove-managed-state', but given the name of our existing virsh command:
managedsave-remove
I'd feel slightly better with:
virsh undefine domain --managed-save
[Meanwhile, we don't have any virsh command that directly exposes virDomainHasManagedSaveImage - that information should probably be added as a flag to an existing command, perhaps 'virsh list --managed-save' adding a column on whether a managed save image exists for each domain.]
This will be added in v2,
+ int flags = 0; + int undefine_managed_state = vshCommandOptBool(cmd, "undefine-managed-state"); + + if (undefine_managed_state) + flags |= VIR_DOMAIN_UNDEFINE_MANAGED_STATE; + + if (!undefine_managed_state) + flags = -1;
if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1440,7 +1449,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) VSH_BYNAME|VSH_BYUUID))) return false;
- if (virDomainUndefine(dom) == 0) { + if (((flags == -1) ? virDomainUndefine(dom) : + virDomainUndefineWithFlags(dom, flags)) == 0) { Most times when we add a new flag to virsh, we can't support it without using the new API. But here, we can, and so I think we should. That is, if you virsh 0.9.4 as the client, and are talking to two different servers (one 0.9.3 and one 0.9.4), you get the following behavior for a domain that has a managed save image:
server 0.9.3: virsh undefine dom -> call virDomainHasManagedSaveImage - if that is true, then refuse to run virDomainUndefine (thus papering over the 0.9.3 safety bug). If false, then virDomainUndefine is safe. virsh undefine dom --managed-save -> tries virDomainUndefineFlags, which fails with unimplemented. Falls back to combo virDomainManagedSaveRemove/virDomainUndefine, which does the job we want.
server 0.9.4 virsh undefine dom -> call virDomainHasManagedSaveImage - if that is true, then refuse to run virDomainUndefine. If false, then virDomainUndefine is safe. virsh undefine dom --managed-save -> tries virDomainUndefineFlags, which succeeds and does the job we want (or fails, but with an error different than unimplemented)
an alternative would be that calling 'virsh undefine dom' without flags skips virDomainHasManagedSaveImage, and just blindly calls virDomainUndefine - in that case, a server at 0.9.4 would properly fail, but a server at 0.9.3 would still expose the bug that we are trying to avoid of leaving stale managed state behind.
For precedence in making virsh try a fallback to older API when the new API is unsupported, see the setvcpus command.
+++ b/tools/virsh.pod @@ -804,11 +804,15 @@ hypervisor. Output the device used for the TTY console of the domain. If the information is not available the processes will provide an exit code of 1.
-=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state>
Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>. Oh, this is different than libvirt.c, which claimed you can run 'undefine' on a running persistent domain. Which is it? Can undefine make a running domain transient, or must it be on an inactive domain?
No, document in libvirt.c is not correct, undefining on a running domain will be refused.
+If I<--undefine-managed-state> is specified, the managed state file will +be removed along with the domain undefine peocess, the entire domain +undefine process will fail if it fails on removing the managed state file.
Given my above thoughts (and once I validate the behavior of undefine on a running domain), I'd word this as:
=item B<undefine> I<domain-id> [I<--managed-save>]
Undefine the configuration for a domain. If domain is not running, the domain name or UUID must be used as the I<domain-id>; if the domain is running, then this converts a persistent domain to a transient domain.
The I<--managed-save> flag guarantees that any managed state (see the B<managesave> command) is also cleaned up. Without the flag, attempts to undefine a domain with managed state will fail.
On 07/13/2011 09:56 PM, Osier Yang wrote:
-=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state>
Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>. Oh, this is different than libvirt.c, which claimed you can run 'undefine' on a running persistent domain. Which is it? Can undefine make a running domain transient, or must it be on an inactive domain?
No, document in libvirt.c is not correct, undefining on a running domain will be refused.
I argue that we found a bug in the qemu driver then. Right now, you can use 'virDomainDefineXML' to convert a transient guest into a persistent one, but there is no counterpart operation to convert a persistent guest back into a transient one. That is, I think the libvirt.c docs are right in implying that you can 'undefine' a running domain, and that the qemu driver needs to be fixed to allow this use case. Either Dan, do you want to weigh in on this API question? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月15日 00:25, Eric Blake 写道:
On 07/13/2011 09:56 PM, Osier Yang wrote:
-=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state>
Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>. Oh, this is different than libvirt.c, which claimed you can run 'undefine' on a running persistent domain. Which is it? Can undefine make a running domain transient, or must it be on an inactive domain? No, document in libvirt.c is not correct, undefining on a running domain will be refused. I argue that we found a bug in the qemu driver then.
Right now, you can use 'virDomainDefineXML' to convert a transient guest into a persistent one, but there is no counterpart operation to convert a persistent guest back into a transient one.
That is, I think the libvirt.c docs are right in implying that you can 'undefine' a running domain, and that the qemu driver needs to be fixed to allow this use case.
It looks to me all of the dirvers which supports domain undefine don't allow to undefine a running domain, except xend, ESX, and xenapi, which leave the determination to the underlying hypervisor.
Either Dan, do you want to weigh in on this API question?
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
于 2011年07月15日 03:50, Osier Yang 写道:
于 2011年07月15日 00:25, Eric Blake 写道:
On 07/13/2011 09:56 PM, Osier Yang wrote:
-=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state>
Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>. Oh, this is different than libvirt.c, which claimed you can run 'undefine' on a running persistent domain. Which is it? Can undefine make a running domain transient, or must it be on an inactive domain? No, document in libvirt.c is not correct, undefining on a running domain will be refused. I argue that we found a bug in the qemu driver then.
Right now, you can use 'virDomainDefineXML' to convert a transient guest into a persistent one, but there is no counterpart operation to convert a persistent guest back into a transient one.
That is, I think the libvirt.c docs are right in implying that you can 'undefine' a running domain, and that the qemu driver needs to be fixed to allow this use case.
It looks to me all of the dirvers which supports domain undefine don't allow to undefine a running domain, except xend, ESX, and xenapi, which leave the determination to the underlying hypervisor.
if (virDomainObjIsActive(vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot delete active domain")); goto cleanup; } if (!vm->persistent) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot undefine transient domain")); goto cleanup; } Is the codes above to check if the domain is persistent bogus? If we don't intend to support undefine a running domain. A transient domain can only be in running or paused state, in which cases the domain id is already not -1, means for a transient domain, codes above will never be executed (fails when checking if it's active)
Either Dan, do you want to weigh in on this API question?
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jul 14, 2011 at 10:25:43AM -0600, Eric Blake wrote:
On 07/13/2011 09:56 PM, Osier Yang wrote:
-=item B<undefine> I<domain-id> +=item B<undefine> I<domain-id> optional I<--undefine-managed-state>
Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>. Oh, this is different than libvirt.c, which claimed you can run 'undefine' on a running persistent domain. Which is it? Can undefine make a running domain transient, or must it be on an inactive domain?
No, document in libvirt.c is not correct, undefining on a running domain will be refused.
I argue that we found a bug in the qemu driver then.
Right now, you can use 'virDomainDefineXML' to convert a transient guest into a persistent one, but there is no counterpart operation to convert a persistent guest back into a transient one.
That is, I think the libvirt.c docs are right in implying that you can 'undefine' a running domain, and that the qemu driver needs to be fixed to allow this use case.
Either Dan, do you want to weigh in on this API question?
I also side on the side that it should be allowed and it's "just" a qemu driver bug :-) 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/
participants (4)
-
Daniel Veillard -
Dave Allan -
Eric Blake -
Osier Yang