On 07/13/2011 04:19 AM, Osier Yang wrote: ACK. 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/13/2011 04:19 AM, Osier Yang wrote: s/to remove/removing/ 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". 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

--- 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: This needs to reflect whatever naming we have in 2/8. Conditional ACK once we agree on the API name. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/13/2011 04:19 AM, Osier Yang wrote: This hunk won't apply to existing libvirt.git. Did you forget to rebase out your first attempt? 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. 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

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

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

于 2011年07月14日 02:05, Eric Blake 写道: Oops, yes.

On Wed, Jul 13, 2011 at 06:19:44PM +0800, Osier Yang wrote: IMO, it would be clearer to call this option remove-managed-state. Dave

于 2011年07月14日 02:23, Eric Blake 写道: This will be added in v2, No, document in libvirt.c is not correct, undefining on a running domain will be refused.