[libvirt] [PATCH] [0/4] Managed save APIs version 2
Change w.r.t. version 1 is the saving path under /var/lib and not /var/run, the restore bug fix spotted by Laine and the various typo and improvement from Eric. As posted earlier, I have implemented the small set of managed save APIs, where libvirt stores the domain state itself and can then recover that state when the domain is started up. I think the code is complete, but not really tested (I still need to debug a failure which seems unrelated), with the exception of the virsh commands which probably need to be extended for convenience. Also I implemented it only for the qemu driver, I would not be surprized if an ESX backend could be implemented since there is no file path in this API. A command "virsh saveall" would be convenient, to be added later. More documentation is needed too. Thanks to Chris Lalancette who wrote a large part of this code ! 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/
[unchanged in v2] Add managed save API entry points virDomainManagedSave() is to be run on a running domain. Once the call complete, as in virDomainSave() the domain is stopped upon completion, but there is no restore counterpart as any order to start the domain from the API would load the state from the managed file, similary if the domain is autostarted when libvirtd starts. Once a domain has restarted his managed save image is destroyed, basically managed save image can only exist for a stopped domain, for a running domain that would be by definition outdated data. * include/libvirt/libvirt.h.in src/libvirt.c src/libvirt_public.syms: adds the new entry points virDomainManagedSave(), virDomainHasManagedSaveImage() and virDomainManagedSaveRemove() * src/driver.h src/esx/esx_driver.c src/lxc/lxc_driver.c src/opennebula/one_driver.c src/openvz/openvz_driver.c src/phyp/phyp_driver.c src/qemu/qemu_driver.c src/vbox/vbox_tmpl.c src/remote/remote_driver.c src/test/test_driver.c src/uml/uml_driver.c src/xen/xen_driver.c: add corresponding new internal drivers entry points diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6d8552f..431e485 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -639,6 +639,16 @@ int virDomainRestore (virConnectPtr conn, const char *from); /* + * Managed domain save + */ +int virDomainManagedSave (virDomainPtr dom, + unsigned int flags); +int virDomainHasManagedSaveImage(virDomainPtr dom, + unsigned int flags); +int virDomainManagedSaveRemove(virDomainPtr dom, + unsigned int flags); + +/* * Domain core dump */ int virDomainCoreDump (virDomainPtr domain, diff --git a/src/driver.h b/src/driver.h index 8f86463..7e2536d 100644 --- a/src/driver.h +++ b/src/driver.h @@ -402,6 +402,15 @@ typedef int (*virDrvDomainEventDeregisterAny)(virConnectPtr conn, int callbackID); +typedef int + (*virDrvDomainManagedSave)(virDomainPtr domain, unsigned int flags); + +typedef int + (*virDrvDomainHasManagedSaveImage)(virDomainPtr domain, unsigned int flags); + +typedef int + (*virDrvDomainManagedSaveRemove)(virDomainPtr domain, unsigned int flags); + /** * _virDriver: * @@ -499,6 +508,9 @@ struct _virDriver { virDrvDomainMigrateSetMaxDowntime domainMigrateSetMaxDowntime; virDrvDomainEventRegisterAny domainEventRegisterAny; virDrvDomainEventDeregisterAny domainEventDeregisterAny; + virDrvDomainManagedSave domainManagedSave; + virDrvDomainHasManagedSaveImage domainHasManagedSaveImage; + virDrvDomainManagedSaveRemove domainManagedSaveRemove; }; typedef int diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 663c560..7e57fff 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3408,6 +3408,9 @@ static virDriver esxDriver = { NULL, /* domainMigrateSetMaxDowntime */ NULL, /* domainEventRegisterAny */ NULL, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; diff --git a/src/libvirt.c b/src/libvirt.c index cc5b4c5..293ea17 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -12135,3 +12135,143 @@ error: virDispatchError(conn); return -1; } + +/** + * virDomainManagedSave: + * @dom: pointer to the domain + * @flags: optional flags currently unused + * + * This method will suspend a domain and save its memory contents to + * a file on disk. After the call, if successful, the domain is not + * listed as running anymore. + * The difference from virDomainSave() is that libvirt is keeping track of + * the saved state itself, and will reuse it once the domain is being + * restarted (automatically or via an explicit libvirt call). + * As a result any running domain is sure to not have a managed saved image. + * + * Returns 0 in case of success or -1 in case of failure + */ +int virDomainManagedSave(virDomainPtr dom, unsigned int flags) +{ + virConnectPtr conn; + + VIR_DEBUG("dom=%p, flags=%u", dom, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(dom)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = dom->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibDomainError(dom, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->driver->domainManagedSave) { + int ret; + + ret = conn->driver->domainManagedSave(dom, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + +/** + * virDomainHasManagedSaveImage: + * @dom: pointer to the domain + * @flags: optional flags currently unused + * + * Check if a domain has a managed save image as created by + * virDomainManagedSave(). Note that any running domain should not have + * such an image, as it should have been removed on restart. + * + * Returns 0 if no image is present, 1 if an image is present, and + * -1 in case of error + */ +int virDomainHasManagedSaveImage(virDomainPtr dom, unsigned int flags) +{ + virConnectPtr conn; + + VIR_DEBUG("dom=%p, flags=%u", dom, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(dom)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = dom->conn; + + if (conn->driver->domainHasManagedSaveImage) { + int ret; + + ret = conn->driver->domainHasManagedSaveImage(dom, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + +/** + * virDomainManagedSaveRemove: + * @dom: pointer to the domain + * @flags: optional flags currently unused + * + * Remove any managed save image as for this domain. + * + * Returns 0 in case of success, and -1 in case of error + */ +int virDomainManagedSaveRemove(virDomainPtr dom, unsigned int flags) +{ + virConnectPtr conn; + + VIR_DEBUG("dom=%p, flags=%u", dom, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(dom)) { + virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = dom->conn; + if (conn->flags & VIR_CONNECT_RO) { + virLibDomainError(dom, VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (conn->driver->domainManagedSaveRemove) { + int ret; + + ret = conn->driver->domainManagedSaveRemove(dom, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 2f812a1..24a422f 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -378,6 +378,9 @@ LIBVIRT_0.7.8 { virNWFilterRef; virNWFilterDefineXML; virNWFilterUndefine; + virDomainManagedSave; + virDomainHasManagedSaveImage; + virDomainManagedSaveRemove; } LIBVIRT_0.7.7; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7ebc7ae..1049f1b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2544,6 +2544,9 @@ static virDriver lxcDriver = { NULL, /* domainMigrateSetMaxDowntime */ lxcDomainEventRegisterAny, /* domainEventRegisterAny */ lxcDomainEventDeregisterAny, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; static virStateDriver lxcStateDriver = { diff --git a/src/opennebula/one_driver.c b/src/opennebula/one_driver.c index e901a03..91d7459 100644 --- a/src/opennebula/one_driver.c +++ b/src/opennebula/one_driver.c @@ -792,6 +792,9 @@ static virDriver oneDriver = { NULL, /* domainMigrateSetMaxDowntime */ NULL, /* domainEventRegisterAny */ NULL, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; static virStateDriver oneStateDriver = { diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 6176577..32bc3c2 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1544,6 +1544,9 @@ static virDriver openvzDriver = { NULL, /* domainMigrateSetMaxDowntime */ NULL, /* domainEventRegisterAny */ NULL, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; int openvzRegister(void) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 4f7efdb..0e1d35f 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1651,6 +1651,9 @@ virDriver phypDriver = { NULL, /* domainMigrateSetMaxDowntime */ NULL, /* domainEventRegisterAny */ NULL, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9ee5da7..17e6118 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10240,6 +10240,9 @@ static virDriver qemuDriver = { qemuDomainMigrateSetMaxDowntime, /* domainMigrateSetMaxDowntime */ qemuDomainEventRegisterAny, /* domainEventRegisterAny */ qemuDomainEventDeregisterAny, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 3b8be21..9b500d0 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -9818,6 +9818,9 @@ static virDriver remote_driver = { remoteDomainMigrateSetMaxDowntime, /* domainMigrateSetMaxDowntime */ remoteDomainEventRegisterAny, /* domainEventRegisterAny */ remoteDomainEventDeregisterAny, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; static virNetworkDriver network_driver = { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 646c7db..105f825 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5306,6 +5306,9 @@ static virDriver testDriver = { NULL, /* domainMigrateSetMaxDowntime */ testDomainEventRegisterAny, /* domainEventRegisterAny */ testDomainEventDeregisterAny, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; static virNetworkDriver testNetworkDriver = { diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 835e5d4..5fa1beb 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -1936,6 +1936,9 @@ static virDriver umlDriver = { NULL, /* domainMigrateSetMaxDowntime */ NULL, /* domainEventRegisterAny */ NULL, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index f7a9b9f..86f6a5a 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -7188,6 +7188,9 @@ virDriver NAME(Driver) = { vboxDomainEventRegisterAny, /* domainEventRegisterAny */ vboxDomainEventDeregisterAny, /* domainEventDeregisterAny */ #endif + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; virNetworkDriver NAME(NetworkDriver) = { diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index ebdc600..a5d58d0 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1980,6 +1980,9 @@ static virDriver xenUnifiedDriver = { NULL, /* domainMigrateSetMaxDowntime */ xenUnifiedDomainEventRegisterAny, /* domainEventRegisterAny */ xenUnifiedDomainEventDeregisterAny, /* domainEventDeregisterAny */ + NULL, /* domainManagedSave */ + NULL, /* domainHasManagedSaveImage */ + NULL, /* domainManagedSaveRemove */ }; /** -- 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 04/02/2010 01:53 PM, Daniel Veillard wrote:
[unchanged in v2]
Add managed save API entry points
virDomainManagedSave() is to be run on a running domain. Once the call complete, as in virDomainSave() the domain is stopped upon completion, but there is no restore counterpart as any order to start the domain from the API would load the state from the managed file, similary if the domain is autostarted when libvirtd starts.
Grammar and spelling tweaks for the commit message. This would read better as: virDomainManagedSave() is to be run on a running domain. Once the call completes, the domain is stopped (as in virDomainSave()). There is no restore counterpart, as the managed file will be automatically restored on any request to start the domain from the API, or if the domain is autostarted when libvirtd starts. But nothing wrong with the commit itself, at least that I could see. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
[unchanged in v2] Implement remote protocol for managed save * src/remote/remote_protocol.x src/remote/remote_protocol.h src/remote/remote_protocol.c src/remote/remote_driver.c: add the entry points in the remote driver * daemon/remote.c daemon/remote_dispatch_args.h daemon/remote_dispatch_prototypes.h daemon/remote_dispatch_table.h: and implement the daemon counterpart diff --git a/daemon/remote.c b/daemon/remote.c index 67162d5..b708027 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -2416,6 +2416,84 @@ remoteDispatchListDomains (struct qemud_server *server ATTRIBUTE_UNUSED, } static int +remoteDispatchDomainManagedSave (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_domain_managed_save_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + virDomainPtr dom; + + dom = get_nonnull_domain (conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + if (virDomainManagedSave (dom, args->flags) == -1) { + virDomainFree(dom); + remoteDispatchConnError(rerr, conn); + return -1; + } + virDomainFree(dom); + return 0; +} + +static int +remoteDispatchDomainHasManagedSaveImage (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_domain_has_managed_save_image_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + virDomainPtr dom; + + dom = get_nonnull_domain (conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + if (virDomainHasManagedSaveImage (dom, args->flags) == -1) { + virDomainFree(dom); + remoteDispatchConnError(rerr, conn); + return -1; + } + virDomainFree(dom); + return 0; +} + +static int +remoteDispatchDomainManagedSaveRemove (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_domain_managed_save_remove_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + virDomainPtr dom; + + dom = get_nonnull_domain (conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + if (virDomainManagedSaveRemove (dom, args->flags) == -1) { + virDomainFree(dom); + remoteDispatchConnError(rerr, conn); + return -1; + } + virDomainFree(dom); + return 0; +} + +static int remoteDispatchListNetworks (struct qemud_server *server ATTRIBUTE_UNUSED, struct qemud_client *client ATTRIBUTE_UNUSED, virConnectPtr conn, diff --git a/daemon/remote_dispatch_args.h b/daemon/remote_dispatch_args.h index b188e6e..51c1675 100644 --- a/daemon/remote_dispatch_args.h +++ b/daemon/remote_dispatch_args.h @@ -151,3 +151,6 @@ remote_list_nwfilters_args val_remote_list_nwfilters_args; remote_nwfilter_define_xml_args val_remote_nwfilter_define_xml_args; remote_nwfilter_undefine_args val_remote_nwfilter_undefine_args; + remote_domain_managed_save_args val_remote_domain_managed_save_args; + remote_domain_has_managed_save_image_args val_remote_domain_has_managed_save_image_args; + remote_domain_managed_save_remove_args val_remote_domain_managed_save_remove_args; diff --git a/daemon/remote_dispatch_prototypes.h b/daemon/remote_dispatch_prototypes.h index e155c69..3671a26 100644 --- a/daemon/remote_dispatch_prototypes.h +++ b/daemon/remote_dispatch_prototypes.h @@ -282,6 +282,14 @@ static int remoteDispatchDomainGetVcpus( remote_error *err, remote_domain_get_vcpus_args *args, remote_domain_get_vcpus_ret *ret); +static int remoteDispatchDomainHasManagedSaveImage( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_domain_has_managed_save_image_args *args, + void *ret); static int remoteDispatchDomainInterfaceStats( struct qemud_server *server, struct qemud_client *client, @@ -330,6 +338,22 @@ static int remoteDispatchDomainLookupByUuid( remote_error *err, remote_domain_lookup_by_uuid_args *args, remote_domain_lookup_by_uuid_ret *ret); +static int remoteDispatchDomainManagedSave( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_domain_managed_save_args *args, + void *ret); +static int remoteDispatchDomainManagedSaveRemove( + struct qemud_server *server, + struct qemud_client *client, + virConnectPtr conn, + remote_message_header *hdr, + remote_error *err, + remote_domain_managed_save_remove_args *args, + void *ret); static int remoteDispatchDomainMemoryPeek( struct qemud_server *server, struct qemud_client *client, diff --git a/daemon/remote_dispatch_table.h b/daemon/remote_dispatch_table.h index 24756d7..ac54f00 100644 --- a/daemon/remote_dispatch_table.h +++ b/daemon/remote_dispatch_table.h @@ -912,3 +912,18 @@ .args_filter = (xdrproc_t) xdr_remote_nwfilter_undefine_args, .ret_filter = (xdrproc_t) xdr_void, }, +{ /* DomainManagedSave => 182 */ + .fn = (dispatch_fn) remoteDispatchDomainManagedSave, + .args_filter = (xdrproc_t) xdr_remote_domain_managed_save_args, + .ret_filter = (xdrproc_t) xdr_void, +}, +{ /* DomainHasManagedSaveImage => 183 */ + .fn = (dispatch_fn) remoteDispatchDomainHasManagedSaveImage, + .args_filter = (xdrproc_t) xdr_remote_domain_has_managed_save_image_args, + .ret_filter = (xdrproc_t) xdr_void, +}, +{ /* DomainManagedSaveRemove => 184 */ + .fn = (dispatch_fn) remoteDispatchDomainManagedSaveRemove, + .args_filter = (xdrproc_t) xdr_remote_domain_managed_save_remove_args, + .ret_filter = (xdrproc_t) xdr_void, +}, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9b500d0..47e9c7d 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3616,6 +3616,78 @@ done: return rv; } +static int +remoteDomainManagedSave (virDomainPtr domain, unsigned int flags) +{ + int rv = -1; + remote_domain_managed_save_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_MANAGED_SAVE, + (xdrproc_t) xdr_remote_domain_managed_save_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int +remoteDomainHasManagedSaveImage (virDomainPtr domain, unsigned int flags) +{ + int rv = -1; + remote_domain_has_managed_save_image_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_HAS_MANAGED_SAVE_IMAGE, + (xdrproc_t) xdr_remote_domain_has_managed_save_image_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int +remoteDomainManagedSaveRemove (virDomainPtr domain, unsigned int flags) +{ + int rv = -1; + remote_domain_managed_save_remove_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_MANAGED_SAVE_REMOVE, + (xdrproc_t) xdr_remote_domain_managed_save_remove_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +} + /*----------------------------------------------------------------------*/ static virDrvOpenStatus ATTRIBUTE_NONNULL (1) @@ -9818,9 +9890,9 @@ static virDriver remote_driver = { remoteDomainMigrateSetMaxDowntime, /* domainMigrateSetMaxDowntime */ remoteDomainEventRegisterAny, /* domainEventRegisterAny */ remoteDomainEventDeregisterAny, /* domainEventDeregisterAny */ - NULL, /* domainManagedSave */ - NULL, /* domainHasManagedSaveImage */ - NULL, /* domainManagedSaveRemove */ + remoteDomainManagedSave, /* domainManagedSave */ + remoteDomainHasManagedSaveImage, /* domainHasManagedSaveImage */ + remoteDomainManagedSaveRemove, /* domainManagedSaveRemove */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.c b/src/remote/remote_protocol.c index f252d85..47a98e0 100644 --- a/src/remote/remote_protocol.c +++ b/src/remote/remote_protocol.c @@ -3289,6 +3289,39 @@ xdr_remote_domain_event_graphics_msg (XDR *xdrs, remote_domain_event_graphics_ms } bool_t +xdr_remote_domain_managed_save_args (XDR *xdrs, remote_domain_managed_save_args *objp) +{ + + if (!xdr_remote_nonnull_domain (xdrs, &objp->dom)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t +xdr_remote_domain_has_managed_save_image_args (XDR *xdrs, remote_domain_has_managed_save_image_args *objp) +{ + + if (!xdr_remote_nonnull_domain (xdrs, &objp->dom)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t +xdr_remote_domain_managed_save_remove_args (XDR *xdrs, remote_domain_managed_save_remove_args *objp) +{ + + if (!xdr_remote_nonnull_domain (xdrs, &objp->dom)) + return FALSE; + if (!xdr_u_int (xdrs, &objp->flags)) + return FALSE; + return TRUE; +} + +bool_t xdr_remote_procedure (XDR *xdrs, remote_procedure *objp) { diff --git a/src/remote/remote_protocol.h b/src/remote/remote_protocol.h index d898502..45dc652 100644 --- a/src/remote/remote_protocol.h +++ b/src/remote/remote_protocol.h @@ -1860,6 +1860,24 @@ struct remote_domain_event_graphics_msg { } subject; }; typedef struct remote_domain_event_graphics_msg remote_domain_event_graphics_msg; + +struct remote_domain_managed_save_args { + remote_nonnull_domain dom; + u_int flags; +}; +typedef struct remote_domain_managed_save_args remote_domain_managed_save_args; + +struct remote_domain_has_managed_save_image_args { + remote_nonnull_domain dom; + u_int flags; +}; +typedef struct remote_domain_has_managed_save_image_args remote_domain_has_managed_save_image_args; + +struct remote_domain_managed_save_remove_args { + remote_nonnull_domain dom; + u_int flags; +}; +typedef struct remote_domain_managed_save_remove_args remote_domain_managed_save_remove_args; #define REMOTE_PROGRAM 0x20008086 #define REMOTE_PROTOCOL_VERSION 1 @@ -2045,6 +2063,9 @@ enum remote_procedure { REMOTE_PROC_LIST_NWFILTERS = 179, REMOTE_PROC_NWFILTER_DEFINE_XML = 180, REMOTE_PROC_NWFILTER_UNDEFINE = 181, + REMOTE_PROC_DOMAIN_MANAGED_SAVE = 182, + REMOTE_PROC_DOMAIN_HAS_MANAGED_SAVE_IMAGE = 183, + REMOTE_PROC_DOMAIN_MANAGED_SAVE_REMOVE = 184, }; typedef enum remote_procedure remote_procedure; @@ -2380,6 +2401,9 @@ extern bool_t xdr_remote_domain_event_io_error_msg (XDR *, remote_domain_event_ extern bool_t xdr_remote_domain_event_graphics_address (XDR *, remote_domain_event_graphics_address*); extern bool_t xdr_remote_domain_event_graphics_identity (XDR *, remote_domain_event_graphics_identity*); extern bool_t xdr_remote_domain_event_graphics_msg (XDR *, remote_domain_event_graphics_msg*); +extern bool_t xdr_remote_domain_managed_save_args (XDR *, remote_domain_managed_save_args*); +extern bool_t xdr_remote_domain_has_managed_save_image_args (XDR *, remote_domain_has_managed_save_image_args*); +extern bool_t xdr_remote_domain_managed_save_remove_args (XDR *, remote_domain_managed_save_remove_args*); extern bool_t xdr_remote_procedure (XDR *, remote_procedure*); extern bool_t xdr_remote_message_type (XDR *, remote_message_type*); extern bool_t xdr_remote_message_status (XDR *, remote_message_status*); @@ -2689,6 +2713,9 @@ extern bool_t xdr_remote_domain_event_io_error_msg (); extern bool_t xdr_remote_domain_event_graphics_address (); extern bool_t xdr_remote_domain_event_graphics_identity (); extern bool_t xdr_remote_domain_event_graphics_msg (); +extern bool_t xdr_remote_domain_managed_save_args (); +extern bool_t xdr_remote_domain_has_managed_save_image_args (); +extern bool_t xdr_remote_domain_managed_save_remove_args (); extern bool_t xdr_remote_procedure (); extern bool_t xdr_remote_message_type (); extern bool_t xdr_remote_message_status (); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index bf87c33..f86c7f1 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1647,6 +1647,21 @@ struct remote_domain_event_graphics_msg { remote_domain_event_graphics_identity subject<REMOTE_DOMAIN_EVENT_GRAPHICS_IDENTITY_MAX>; }; +struct remote_domain_managed_save_args { + remote_nonnull_domain dom; + unsigned flags; +}; + +struct remote_domain_has_managed_save_image_args { + remote_nonnull_domain dom; + unsigned flags; +}; + +struct remote_domain_managed_save_remove_args { + remote_nonnull_domain dom; + unsigned flags; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -1852,7 +1867,10 @@ enum remote_procedure { REMOTE_PROC_LIST_NWFILTERS = 179, REMOTE_PROC_NWFILTER_DEFINE_XML = 180, - REMOTE_PROC_NWFILTER_UNDEFINE = 181 + REMOTE_PROC_NWFILTER_UNDEFINE = 181, + REMOTE_PROC_DOMAIN_MANAGED_SAVE = 182, + REMOTE_PROC_DOMAIN_HAS_MANAGED_SAVE_IMAGE = 183, + REMOTE_PROC_DOMAIN_MANAGED_SAVE_REMOVE = 184 /* * Notice how the entries are grouped in sets of 10 ? -- 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 04/02/2010 01:54 PM, Daniel Veillard wrote:
+static int +remoteDispatchDomainHasManagedSaveImage (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_domain_has_managed_save_image_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + virDomainPtr dom; + + dom = get_nonnull_domain (conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + if (virDomainHasManagedSaveImage (dom, args->flags) == -1) { + virDomainFree(dom); + remoteDispatchConnError(rerr, conn); + return -1; + } + virDomainFree(dom); + return 0; +}
Shouldn't this be able to return 1 in the case where virDomainHasManagedSaveImage returns 1?
+static int +remoteDomainHasManagedSaveImage (virDomainPtr domain, unsigned int flags) +{ + int rv = -1; + remote_domain_has_managed_save_image_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_HAS_MANAGED_SAVE_IMAGE, + (xdrproc_t) xdr_remote_domain_has_managed_save_image_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +}
Likewise. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Apr 02, 2010 at 02:12:12PM -0600, Eric Blake wrote:
On 04/02/2010 01:54 PM, Daniel Veillard wrote:
+static int +remoteDispatchDomainHasManagedSaveImage (struct qemud_server *server ATTRIBUTE_UNUSED, + struct qemud_client *client ATTRIBUTE_UNUSED, + virConnectPtr conn, + remote_message_header *hdr ATTRIBUTE_UNUSED, + remote_error *rerr, + remote_domain_has_managed_save_image_args *args, + void *ret ATTRIBUTE_UNUSED) +{ + virDomainPtr dom; + + dom = get_nonnull_domain (conn, args->dom); + if (dom == NULL) { + remoteDispatchConnError(rerr, conn); + return -1; + } + + if (virDomainHasManagedSaveImage (dom, args->flags) == -1) { + virDomainFree(dom); + remoteDispatchConnError(rerr, conn); + return -1; + } + virDomainFree(dom); + return 0; +}
Shouldn't this be able to return 1 in the case where virDomainHasManagedSaveImage returns 1?
yes it's a bug, we need a real return value structure
+static int +remoteDomainHasManagedSaveImage (virDomainPtr domain, unsigned int flags) +{ + int rv = -1; + remote_domain_has_managed_save_image_args args; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain (&args.dom, domain); + args.flags = flags; + + if (call (domain->conn, priv, 0, REMOTE_PROC_DOMAIN_HAS_MANAGED_SAVE_IMAGE, + (xdrproc_t) xdr_remote_domain_has_managed_save_image_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + +done: + remoteDriverUnlock(priv); + return rv; +}
Likewise.
Yup, I end up with the following change on top of the previous patch, thanks a lot ! 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 04/02/2010 04:56 PM, Daniel Veillard wrote:
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f86c7f1..0374f9a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1657,6 +1657,10 @@ struct remote_domain_has_managed_save_image_args { unsigned flags; };
+struct remote_domain_has_managed_save_image_ret { + int ret; +}; +
Hm, I don't think this is necessary. I think the return value is always going to be an int, so you should just be able to return -1, 0, or 1 in the remote driver as necessary. At least, that's how all of the other things that return numbers (such as virDomainNumDefinedDomains) work. -- Chris Lalancette
On Fri, Apr 02, 2010 at 05:29:58PM -0400, Chris Lalancette wrote:
On 04/02/2010 04:56 PM, Daniel Veillard wrote:
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f86c7f1..0374f9a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1657,6 +1657,10 @@ struct remote_domain_has_managed_save_image_args { unsigned flags; };
+struct remote_domain_has_managed_save_image_ret { + int ret; +}; +
Hm, I don't think this is necessary. I think the return value is always going to be an int, so you should just be able to return -1, 0, or 1 in the remote driver as necessary.
My initial reaction was the same, then I looked at GetMaxVcpus and other examples and converted the code accordingly.
At least, that's how all of the other things that return numbers (such as virDomainNumDefinedDomains) work.
In the cases I checked and looked for it seems the network call() return values is always 0 or -1, and looking at virDomainGetMaxVcpus() it does use struct remote_domain_get_max_vcpus_ret { int num; }; same for virDomainNumDomains() and I also see struct remote_num_of_defined_domains_ret { int num; }; in the src/remote/remote_protocol.x right now, remoteNumOfDefinedDomains( does use remote_num_of_defined_domains_ret ret; and remoteDispatchNumOfDefinedDomains() do use a remote_num_of_defined_domains_ret *ret argument, so I'm wondering if we are really looking at the same code. In the case we just return 0 for success and -1 in case of error, we clearly don't need the return structure, but all examples I checked for an full int reurn used a structure. So I assume the change is needed, or at least it's safe :-) 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 04/04/2010 05:24 AM, Daniel Veillard wrote:
In the cases I checked and looked for it seems the network call() return values is always 0 or -1, and looking at virDomainGetMaxVcpus() it does use
struct remote_domain_get_max_vcpus_ret { int num; };
same for virDomainNumDomains()
and I also see
struct remote_num_of_defined_domains_ret { int num; };
in the src/remote/remote_protocol.x right now, remoteNumOfDefinedDomains( does use remote_num_of_defined_domains_ret ret; and remoteDispatchNumOfDefinedDomains() do use a remote_num_of_defined_domains_ret *ret argument, so I'm wondering if we are really looking at the same code.
In the case we just return 0 for success and -1 in case of error, we clearly don't need the return structure, but all examples I checked for an full int reurn used a structure. So I assume the change is needed, or at least it's safe :-)
Sigh, never mind me. I looked at the wrong code. You are right :). -- Chris Lalancette
On Sun, Apr 04, 2010 at 11:24:56AM +0200, Daniel Veillard wrote:
On Fri, Apr 02, 2010 at 05:29:58PM -0400, Chris Lalancette wrote:
On 04/02/2010 04:56 PM, Daniel Veillard wrote:
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f86c7f1..0374f9a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -1657,6 +1657,10 @@ struct remote_domain_has_managed_save_image_args { unsigned flags; };
+struct remote_domain_has_managed_save_image_ret { + int ret; +}; +
Hm, I don't think this is necessary. I think the return value is always going to be an int, so you should just be able to return -1, 0, or 1 in the remote driver as necessary.
My initial reaction was the same, then I looked at GetMaxVcpus and other examples and converted the code accordingly.
At least, that's how all of the other things that return numbers (such as virDomainNumDefinedDomains) work.
In the cases I checked and looked for it seems the network call() return values is always 0 or -1, and looking at virDomainGetMaxVcpus() it does use
struct remote_domain_get_max_vcpus_ret { int num; };
same for virDomainNumDomains()
and I also see
struct remote_num_of_defined_domains_ret { int num; };
in the src/remote/remote_protocol.x right now, remoteNumOfDefinedDomains( does use remote_num_of_defined_domains_ret ret; and remoteDispatchNumOfDefinedDomains() do use a remote_num_of_defined_domains_ret *ret argument, so I'm wondering if we are really looking at the same code.
In the case we just return 0 for success and -1 in case of error, we clearly don't need the return structure, but all examples I checked for an full int reurn used a structure. So I assume the change is needed, or at least it's safe :-)
That is correct. The success vs fail error code in the wire protocol is a simple enum enum remote_message_status { /* Status is always REMOTE_OK for calls. * For replies, indicates no error. */ REMOTE_OK = 0, /* For replies, indicates that an error happened, and a struct * remote_error follows. */ REMOTE_ERROR = 1, /* For streams, indicates that more data is still expected */ REMOTE_CONTINUE = 2 }; Thus, it can't cope with any data that needs to be returned. So in this case, you need the 'int ret' return value for the positive case - this is very like the virDomainIsPersistent() method. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
[ v2 with new saving path, bug fix and typo fix ] Implement managed save operations for qemu driver The images are saved in /var/lib/libvirt/qemu/save/ and named $domainname.save . The directory is created appropriately at daemon startup. When a domain is started while a saved image is available, libvirt will try to load this saved image, and start the domain as usual in case of failure. In any case the saved image is discarded once the domain is created. * src/qemu/qemu_conf.h: adds an extra save path to the driver config * src/qemu/qemu_driver.c: implement the 3 new operations and handling of the image directory diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 39518ca..0b247d6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -120,6 +120,7 @@ struct qemud_driver { * the QEMU user/group */ char *libDir; char *cacheDir; + char *saveDir; unsigned int vncTLS : 1; unsigned int vncTLSx509verify : 1; unsigned int vncSASL : 1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 94f7fef..5a678c9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1399,6 +1399,9 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->cacheDir, "%s/cache/libvirt/qemu", LOCAL_STATE_DIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, + "%s/lib/libvirt/qemu/save/", LOCAL_STATE_DIR) == -1) + goto out_of_memory; } else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -1423,6 +1426,8 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->cacheDir, "%s/qemu/cache", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, "%s/qemu/save", base) == -1) + goto out_of_memory; } if (virFileMakePath(qemu_driver->stateDir) != 0) { @@ -1443,6 +1448,12 @@ qemudStartup(int privileged) { qemu_driver->cacheDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } + if (virFileMakePath(qemu_driver->saveDir) != 0) { + char ebuf[1024]; + VIR_ERROR(_("Failed to create save dir '%s': %s"), + qemu_driver->saveDir, virStrerror(errno, ebuf, sizeof ebuf)); + goto error; + } /* Configuration paths are either ~/.libvirt/qemu/... (session) or * /etc/libvirt/qemu/... (system). @@ -1493,6 +1504,12 @@ qemudStartup(int privileged) { qemu_driver->cacheDir, qemu_driver->user, qemu_driver->group); goto error; } + if (chown(qemu_driver->saveDir, qemu_driver->user, qemu_driver->group) < 0) { + virReportSystemError(errno, + _("unable to set ownership of '%s' to %d:%d"), + qemu_driver->saveDir, qemu_driver->user, qemu_driver->group); + goto error; + } } /* If hugetlbfs is present, then we need to create a sub-directory within @@ -1645,6 +1662,7 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->stateDir); VIR_FREE(qemu_driver->libDir); VIR_FREE(qemu_driver->cacheDir); + VIR_FREE(qemu_driver->saveDir); VIR_FREE(qemu_driver->vncTLSx509certdir); VIR_FREE(qemu_driver->vncListen); VIR_FREE(qemu_driver->vncPassword); @@ -4570,8 +4588,8 @@ endjob: } -static int qemudDomainSave(virDomainPtr dom, - const char *path) +static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, + int compressed) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm = NULL; @@ -4589,18 +4607,8 @@ static int qemudDomainSave(virDomainPtr dom, header.version = QEMUD_SAVE_VERSION; qemuDriverLock(driver); - if (driver->saveImageFormat == NULL) - header.compressed = QEMUD_SAVE_FORMAT_RAW; - else { - header.compressed = - qemudSaveCompressionTypeFromString(driver->saveImageFormat); - if (header.compressed < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Invalid save image format specified " - "in configuration file")); - goto cleanup; - } - } + + header.compressed = compressed; vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4829,6 +4837,165 @@ cleanup: return ret; } +static int qemudDomainSave(virDomainPtr dom, const char *path) +{ + struct qemud_driver *driver = dom->conn->privateData; + int compressed; + + /* Hm, is this safe against qemudReload? */ + if (driver->saveImageFormat == NULL) + compressed = QEMUD_SAVE_FORMAT_RAW; + else { + compressed = qemudSaveCompressionTypeFromString(driver->saveImageFormat); + if (compressed < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("Invalid save image format specified " + "in configuration file")); + return -1; + } + } + + return qemudDomainSaveFlag(dom, path, compressed); +} + +static char * +qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) { + char *ret; + + if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) { + virReportOOMError(); + return(NULL); + } + + return(ret); +} + +static int +qemuDomainManagedSave(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char *name = NULL; + int ret = -1; + int compressed; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto error; + } + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto error; + } + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + goto error; + + VIR_DEBUG("Saving state to %s", name); + + /* FIXME: we should take the flags parameter, and use bits out + * of there to control whether we are compressing or not + */ + compressed = QEMUD_SAVE_FORMAT_RAW; + + /* we have to drop our locks here because qemudDomainSaveFlag is + * going to pick them back up. Unfortunately it opens a race window + * between us dropping and qemudDomainSaveFlag picking it back up, but + * if we want to allow other operations to be able to happen while + * qemuDomainSaveFlag is running (possibly for a long time), I'm not + * sure if there is a better solution + */ + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + + ret = qemudDomainSaveFlag(dom, name, compressed); + +cleanup: + VIR_FREE(name); + + return ret; + +error: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + goto cleanup; +} + +static int +qemuDomainHasManagedSaveImage(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + char *name = NULL; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + goto cleanup; + + ret = virFileExists(name); + +cleanup: + VIR_FREE(name); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int +qemuDomainManagedSaveRemove(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + char *name = NULL; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + goto cleanup; + + ret = unlink(name); + +cleanup: + VIR_FREE(name); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} static int qemudDomainCoreDump(virDomainPtr dom, const char *path, @@ -5979,6 +6146,7 @@ static int qemudDomainStart(virDomainPtr dom) { virDomainObjPtr vm; int ret = -1; virDomainEventPtr event = NULL; + char *managed_save = NULL; qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6000,6 +6168,32 @@ static int qemudDomainStart(virDomainPtr dom) { goto endjob; } + /* + * If there is a managed saved state restore it instead of starting + * from scratch. In any case the old state is removed. + */ + managed_save = qemuDomainManagedSavePath(driver, vm); + if ((managed_save) && (virFileExists(managed_save))) { + /* We should still have a reference left to vm but */ + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + ret = qemudDomainRestore(dom->conn, managed_save); + + if (unlink(managed_save) < 0) { + VIR_WARN("Failed to remove the managed state %s", managed_save); + } + + if (ret == 0) { + /* qemudDomainRestore should have sent the Started/Restore event */ + VIR_FREE(managed_save); + return(ret); + } + qemuDriverLock(driver); + virDomainObjLock(vm); + } + ret = qemudStartVMDaemon(dom->conn, driver, vm, NULL, -1); if (ret != -1) event = virDomainEventNewFromObj(vm, @@ -6011,6 +6205,7 @@ endjob: vm = NULL; cleanup: + VIR_FREE(managed_save); if (vm) virDomainObjUnlock(vm); if (event) @@ -10312,9 +10507,9 @@ static virDriver qemuDriver = { qemuDomainMigrateSetMaxDowntime, /* domainMigrateSetMaxDowntime */ qemuDomainEventRegisterAny, /* domainEventRegisterAny */ qemuDomainEventDeregisterAny, /* domainEventDeregisterAny */ - NULL, /* domainManagedSave */ - NULL, /* domainHasManagedSaveImage */ - NULL, /* domainManagedSaveRemove */ + qemuDomainManagedSave, /* domainManagedSave */ + qemuDomainHasManagedSaveImage, /* domainHasManagedSaveImage */ + qemuDomainManagedSaveRemove, /* domainManagedSaveRemove */ }; -- 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 04/02/2010 01:56 PM, Daniel Veillard wrote:
+++ b/src/qemu/qemu_driver.c @@ -1399,6 +1399,9 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->cacheDir, "%s/cache/libvirt/qemu", LOCAL_STATE_DIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, + "%s/lib/libvirt/qemu/save/", LOCAL_STATE_DIR) == -1) + goto out_of_memory;
Would it be more efficient to write a followup patch that does: if ((qemu_driver->savedir = strdup(LOCAL_STATE_DIR "/lib/libvirt/qemu/save/") == NULL) goto out_of_memory; on the grounds that compile-time concatenation and strdup are much lighter-weight than run-time concatenation of virAsprintf? But that doesn't affect the quality of your patch.
} else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -1423,6 +1426,8 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->cacheDir, "%s/qemu/cache", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, "%s/qemu/save", base) == -1) + goto out_of_memory;
And constructs like this still need virAsprintf.
-static int qemudDomainSave(virDomainPtr dom, - const char *path) +static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, + int compressed)
Should that be bool compressed, since we are only using it as a binary? Or are there plans to extend it in the future to allow choice between multiple acceptable compression algorithms, in which case it is better as an unsigned int?
+static int qemudDomainSave(virDomainPtr dom, const char *path) +{ + struct qemud_driver *driver = dom->conn->privateData; + int compressed; + + /* Hm, is this safe against qemudReload? */ + if (driver->saveImageFormat == NULL) + compressed = QEMUD_SAVE_FORMAT_RAW; + else { + compressed = qemudSaveCompressionTypeFromString(driver->saveImageFormat); + if (compressed < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("Invalid save image format specified " + "in configuration file")); + return -1; + } + } + + return qemudDomainSaveFlag(dom, path, compressed);
If you convert the type of the last argument of qemudDomanSaveFlag, then here is where you'd convert from int (qemudSaveCompressionTypeFromString must remain int, to detect failure) to bool.
+static int +qemuDomainManagedSave(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED)
No - for future compability, we NEED to check that flags==0 or fail now. Otherwise, a future version, where flags has meaning, will mistakenly think that our older version can honor those flags.
+{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char *name = NULL; + int ret = -1; + int compressed; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto error; + } + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto error; + } + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + goto error; + + VIR_DEBUG("Saving state to %s", name); + + /* FIXME: we should take the flags parameter, and use bits out + * of there to control whether we are compressing or not + */ + compressed = QEMUD_SAVE_FORMAT_RAW; + + /* we have to drop our locks here because qemudDomainSaveFlag is + * going to pick them back up. Unfortunately it opens a race window + * between us dropping and qemudDomainSaveFlag picking it back up, but + * if we want to allow other operations to be able to happen while + * qemuDomainSaveFlag is running (possibly for a long time), I'm not + * sure if there is a better solution + */
Is there a way to tell qemudDomainSaveFlag that we called it while the lock was already held (that is, consume one of the bits of the flag argument that we pass to qemudDomainSaveFlag for internal use)? That way, we don't have to drop the lock here, but let qemudDomainSaveFlag drop it on our behalf.
+ virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + + ret = qemudDomainSaveFlag(dom, name, compressed); + +cleanup: + VIR_FREE(name); + + return ret; + +error: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + goto cleanup; +} + +static int +qemuDomainHasManagedSaveImage(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED)
Again, we MUST ensure flags==0 now, to allow future compatibility.
+{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + char *name = NULL; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + goto cleanup; + + ret = virFileExists(name); + +cleanup: + VIR_FREE(name); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int +qemuDomainManagedSaveRemove(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED)
And again.
+{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + int ret = -1; + char *name = NULL; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + goto cleanup; + + ret = unlink(name); + +cleanup: + VIR_FREE(name); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +}
static int qemudDomainCoreDump(virDomainPtr dom, const char *path, @@ -5979,6 +6146,7 @@ static int qemudDomainStart(virDomainPtr dom) { virDomainObjPtr vm; int ret = -1; virDomainEventPtr event = NULL; + char *managed_save = NULL;
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6000,6 +6168,32 @@ static int qemudDomainStart(virDomainPtr dom) { goto endjob; }
+ /* + * If there is a managed saved state restore it instead of starting + * from scratch. In any case the old state is removed. + */ + managed_save = qemuDomainManagedSavePath(driver, vm); + if ((managed_save) && (virFileExists(managed_save))) { + /* We should still have a reference left to vm but */
Incomplete comment?
+ if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + ret = qemudDomainRestore(dom->conn, managed_save); + + if (unlink(managed_save) < 0) { + VIR_WARN("Failed to remove the managed state %s", managed_save); + } + + if (ret == 0) { + /* qemudDomainRestore should have sent the Started/Restore event */ + VIR_FREE(managed_save); + return(ret); + } + qemuDriverLock(driver); + virDomainObjLock(vm); + } +
Overall, it looks like it does what you claim, but I think there's still some work needed before an ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/02/2010 04:26 PM, Eric Blake wrote:
On 04/02/2010 01:56 PM, Daniel Veillard wrote:
+static int +qemuDomainManagedSave(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED)
No - for future compability, we NEED to check that flags==0 or fail now. Otherwise, a future version, where flags has meaning, will mistakenly think that our older version can honor those flags.
That makes sense, but there's a lot of precedence to the contrary already in the code: find . -name \*.c | xargs grep "flags ATTRIBUTE_UNUSED" | wc 155 832 14106 (some of these are probably false positives, but still...) Sounds like you're volunteering for a global cleanup ;-)
On Fri, Apr 02, 2010 at 04:48:36PM -0400, Laine Stump wrote:
On 04/02/2010 04:26 PM, Eric Blake wrote:
On 04/02/2010 01:56 PM, Daniel Veillard wrote:
+static int +qemuDomainManagedSave(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED) No - for future compability, we NEED to check that flags==0 or fail now. Otherwise, a future version, where flags has meaning, will mistakenly think that our older version can honor those flags.
That makes sense, but there's a lot of precedence to the contrary already in the code:
find . -name \*.c | xargs grep "flags ATTRIBUTE_UNUSED" | wc 155 832 14106
(some of these are probably false positives, but still...)
Sounds like you're volunteering for a global cleanup ;-)
The problem is that the cleanup could break existing behaviour, so I would be quite cautious about it and really look on a case by case basis. But for new interfaces, yes we should do this, 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 04/02/2010 04:26 PM, Eric Blake wrote:
-static int qemudDomainSave(virDomainPtr dom, - const char *path) +static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, + int compressed)
Should that be bool compressed, since we are only using it as a binary? Or are there plans to extend it in the future to allow choice between multiple acceptable compression algorithms, in which case it is better as an unsigned int?
No, it's definitely not a bool. It's meant to take a number of different arguments (like compressed-gzip, compressed-bzip2, etc). So an unsigned int seems better. -- Chris Lalancette
On Fri, Apr 02, 2010 at 02:26:10PM -0600, Eric Blake wrote:
On 04/02/2010 01:56 PM, Daniel Veillard wrote:
+++ b/src/qemu/qemu_driver.c @@ -1399,6 +1399,9 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->cacheDir, "%s/cache/libvirt/qemu", LOCAL_STATE_DIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, + "%s/lib/libvirt/qemu/save/", LOCAL_STATE_DIR) == -1) + goto out_of_memory;
Would it be more efficient to write a followup patch that does:
if ((qemu_driver->savedir = strdup(LOCAL_STATE_DIR "/lib/libvirt/qemu/save/") == NULL) goto out_of_memory;
on the grounds that compile-time concatenation and strdup are much lighter-weight than run-time concatenation of virAsprintf?
But that doesn't affect the quality of your patch.
there is a block of 4 allocations that way, I think they should be kept similar
} else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -1423,6 +1426,8 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->cacheDir, "%s/qemu/cache", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, "%s/qemu/save", base) == -1) + goto out_of_memory;
And constructs like this still need virAsprintf.
same thing, kept code similar to previous blocks. We could optimize, yes, but it's run once in libvirtd lifetime :-)
-static int qemudDomainSave(virDomainPtr dom, - const char *path) +static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, + int compressed)
Should that be bool compressed, since we are only using it as a binary? Or are there plans to extend it in the future to allow choice between multiple acceptable compression algorithms, in which case it is better as an unsigned int?
we can have multiple compression options, so an int, yes
+static int qemudDomainSave(virDomainPtr dom, const char *path) +{ + struct qemud_driver *driver = dom->conn->privateData; + int compressed; + + /* Hm, is this safe against qemudReload? */ + if (driver->saveImageFormat == NULL) + compressed = QEMUD_SAVE_FORMAT_RAW; + else { + compressed = qemudSaveCompressionTypeFromString(driver->saveImageFormat); + if (compressed < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("Invalid save image format specified " + "in configuration file")); + return -1; + } + } + + return qemudDomainSaveFlag(dom, path, compressed);
If you convert the type of the last argument of qemudDomanSaveFlag, then here is where you'd convert from int (qemudSaveCompressionTypeFromString must remain int, to detect failure) to bool.
+static int +qemuDomainManagedSave(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED)
No - for future compability, we NEED to check that flags==0 or fail now. Otherwise, a future version, where flags has meaning, will mistakenly think that our older version can honor those flags.
Hum, we used to not check undefined flags, we are changing that now, it makes sense. But no capital/yelling please !
+ /* FIXME: we should take the flags parameter, and use bits out + * of there to control whether we are compressing or not + */ + compressed = QEMUD_SAVE_FORMAT_RAW; + + /* we have to drop our locks here because qemudDomainSaveFlag is + * going to pick them back up. Unfortunately it opens a race window + * between us dropping and qemudDomainSaveFlag picking it back up, but + * if we want to allow other operations to be able to happen while + * qemuDomainSaveFlag is running (possibly for a long time), I'm not + * sure if there is a better solution + */
Is there a way to tell qemudDomainSaveFlag that we called it while the lock was already held (that is, consume one of the bits of the flag argument that we pass to qemudDomainSaveFlag for internal use)? That way, we don't have to drop the lock here, but let qemudDomainSaveFlag drop it on our behalf.
Not in the current code as I understand it. This would need some refactoring.
+static int +qemuDomainHasManagedSaveImage(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED)
Again, we MUST ensure flags==0 now, to allow future compatibility. [...]
+static int +qemuDomainManagedSaveRemove(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED)
And again.
+ managed_save = qemuDomainManagedSavePath(driver, vm); + if ((managed_save) && (virFileExists(managed_save))) { + /* We should still have a reference left to vm but */
Incomplete comment?
not really, that could be "but ..." or "but one should check for 0 anyway" I end up with the following additional patch, thanks ! 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 04/02/2010 03:19 PM, Daniel Veillard wrote:
+ managed_save = qemuDomainManagedSavePath(driver, vm); + if ((managed_save) && (virFileExists(managed_save))) { + /* We should still have a reference left to vm but */
Incomplete comment?
not really, that could be "but ..." or "but one should check for 0 anyway"
I end up with the following additional patch,
- /* We should still have a reference left to vm but */ + /* + * We should still have a reference left to vm but + * but one should check for 0 anyway + */
s/but but/but/ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Apr 02, 2010 at 03:27:53PM -0600, Eric Blake wrote:
On 04/02/2010 03:19 PM, Daniel Veillard wrote:
+ managed_save = qemuDomainManagedSavePath(driver, vm); + if ((managed_save) && (virFileExists(managed_save))) { + /* We should still have a reference left to vm but */
Incomplete comment?
in retrospect, yes, unless you could read my mind :-)
not really, that could be "but ..." or "but one should check for 0 anyway"
I end up with the following additional patch,
- /* We should still have a reference left to vm but */ + /* + * We should still have a reference left to vm but + * but one should check for 0 anyway + */
s/but but/but/
Dohh right, fixed, thanks ! It's interesting how one perceive things differently in code and comments between a cold out of context analysis, and a warm one where a lot of mental assumptions have already be made around it. And what makes this external review process so effective (an I guess core part of the Extreme Programming method). So thanks a lot of your feedback :-) 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 Fri, Apr 02, 2010 at 09:56:00PM +0200, Daniel Veillard wrote:
[ v2 with new saving path, bug fix and typo fix ]
Implement managed save operations for qemu driver
The images are saved in /var/lib/libvirt/qemu/save/ and named $domainname.save . The directory is created appropriately at daemon startup. When a domain is started while a saved image is available, libvirt will try to load this saved image, and start the domain as usual in case of failure. In any case the saved image is discarded once the domain is created.
* src/qemu/qemu_conf.h: adds an extra save path to the driver config * src/qemu/qemu_driver.c: implement the 3 new operations and handling of the image directory
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 39518ca..0b247d6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -120,6 +120,7 @@ struct qemud_driver { * the QEMU user/group */ char *libDir; char *cacheDir; + char *saveDir; unsigned int vncTLS : 1; unsigned int vncTLSx509verify : 1; unsigned int vncSASL : 1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 94f7fef..5a678c9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1399,6 +1399,9 @@ qemudStartup(int privileged) { if (virAsprintf(&qemu_driver->cacheDir, "%s/cache/libvirt/qemu", LOCAL_STATE_DIR) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, + "%s/lib/libvirt/qemu/save/", LOCAL_STATE_DIR) == -1) + goto out_of_memory; } else { uid_t uid = geteuid(); char *userdir = virGetUserDirectory(uid); @@ -1423,6 +1426,8 @@ qemudStartup(int privileged) { goto out_of_memory; if (virAsprintf(&qemu_driver->cacheDir, "%s/qemu/cache", base) == -1) goto out_of_memory; + if (virAsprintf(&qemu_driver->saveDir, "%s/qemu/save", base) == -1) + goto out_of_memory; }
if (virFileMakePath(qemu_driver->stateDir) != 0) { @@ -1443,6 +1448,12 @@ qemudStartup(int privileged) { qemu_driver->cacheDir, virStrerror(errno, ebuf, sizeof ebuf)); goto error; } + if (virFileMakePath(qemu_driver->saveDir) != 0) { + char ebuf[1024]; + VIR_ERROR(_("Failed to create save dir '%s': %s"), + qemu_driver->saveDir, virStrerror(errno, ebuf, sizeof ebuf)); + goto error; + }
/* Configuration paths are either ~/.libvirt/qemu/... (session) or * /etc/libvirt/qemu/... (system). @@ -1493,6 +1504,12 @@ qemudStartup(int privileged) { qemu_driver->cacheDir, qemu_driver->user, qemu_driver->group); goto error; } + if (chown(qemu_driver->saveDir, qemu_driver->user, qemu_driver->group) < 0) { + virReportSystemError(errno, + _("unable to set ownership of '%s' to %d:%d"), + qemu_driver->saveDir, qemu_driver->user, qemu_driver->group); + goto error; + } }
/* If hugetlbfs is present, then we need to create a sub-directory within @@ -1645,6 +1662,7 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->stateDir); VIR_FREE(qemu_driver->libDir); VIR_FREE(qemu_driver->cacheDir); + VIR_FREE(qemu_driver->saveDir); VIR_FREE(qemu_driver->vncTLSx509certdir); VIR_FREE(qemu_driver->vncListen); VIR_FREE(qemu_driver->vncPassword); @@ -4570,8 +4588,8 @@ endjob: }
-static int qemudDomainSave(virDomainPtr dom, - const char *path) +static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, + int compressed) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm = NULL; @@ -4589,18 +4607,8 @@ static int qemudDomainSave(virDomainPtr dom, header.version = QEMUD_SAVE_VERSION;
qemuDriverLock(driver); - if (driver->saveImageFormat == NULL) - header.compressed = QEMUD_SAVE_FORMAT_RAW; - else { - header.compressed = - qemudSaveCompressionTypeFromString(driver->saveImageFormat); - if (header.compressed < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Invalid save image format specified " - "in configuration file")); - goto cleanup; - } - } + + header.compressed = compressed;
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
@@ -4829,6 +4837,165 @@ cleanup: return ret; }
+static int qemudDomainSave(virDomainPtr dom, const char *path) +{ + struct qemud_driver *driver = dom->conn->privateData; + int compressed; + + /* Hm, is this safe against qemudReload? */ + if (driver->saveImageFormat == NULL) + compressed = QEMUD_SAVE_FORMAT_RAW; + else { + compressed = qemudSaveCompressionTypeFromString(driver->saveImageFormat); + if (compressed < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("Invalid save image format specified " + "in configuration file")); + return -1; + } + } + + return qemudDomainSaveFlag(dom, path, compressed); +} + +static char * +qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) { + char *ret; + + if (virAsprintf(&ret, "%s/%s.save", driver->saveDir, vm->def->name) < 0) { + virReportOOMError(); + return(NULL); + } + + return(ret); +} + +static int +qemuDomainManagedSave(virDomainPtr dom, + unsigned int flags ATTRIBUTE_UNUSED) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + char *name = NULL; + int ret = -1; + int compressed; + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto error; + } + + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto error; + } + + name = qemuDomainManagedSavePath(driver, vm); + if (name == NULL) + goto error; + + VIR_DEBUG("Saving state to %s", name); + + /* FIXME: we should take the flags parameter, and use bits out + * of there to control whether we are compressing or not + */ + compressed = QEMUD_SAVE_FORMAT_RAW; + + /* we have to drop our locks here because qemudDomainSaveFlag is + * going to pick them back up. Unfortunately it opens a race window + * between us dropping and qemudDomainSaveFlag picking it back up, but + * if we want to allow other operations to be able to happen while + * qemuDomainSaveFlag is running (possibly for a long time), I'm not + * sure if there is a better solution + */ + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + + ret = qemudDomainSaveFlag(dom, name, compressed);
What you need todo here is to - Instead of passing virDomainPtr into qemudDomainSaveFlag, pass the 'driver' and 'vm' objects. - Move all the locking from qemudDomainSaveFlag into qemudDomainSavePath So both qemudDomainSavePath() and this qemuDomainManagedSave() can then be passing a pre-locked object into the qemudDomainSaveFlag() method.
@@ -6000,6 +6168,32 @@ static int qemudDomainStart(virDomainPtr dom) { goto endjob; }
+ /* + * If there is a managed saved state restore it instead of starting + * from scratch. In any case the old state is removed. + */ + managed_save = qemuDomainManagedSavePath(driver, vm); + if ((managed_save) && (virFileExists(managed_save))) { + /* We should still have a reference left to vm but */ + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + ret = qemudDomainRestore(dom->conn, managed_save); + + if (unlink(managed_save) < 0) { + VIR_WARN("Failed to remove the managed state %s", managed_save); + } + + if (ret == 0) { + /* qemudDomainRestore should have sent the Started/Restore event */ + VIR_FREE(managed_save); + return(ret); + } + qemuDriverLock(driver); + virDomainObjLock(vm); + }
Here as well, I think we need to split qemudDomainRestore() into two methods. qemudDomainRestoreInternal() which takes a driver + vm object which have been pre-locked. This qemudDomainStart() would call the qemudDomainRestoreInternal() method directly. And likewise qemudDomainRestore would then also call qemudDomainRestoreInternal. The general rule with the locking is that driver API entry points should not call other driver API entry points. The common functionality should be pulled out into a separate method which is passed pre-locked objects of type virDomainObjPtr instead of virDomainPtr. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
[v2 with typo and doc improvement] Add a managedsave command to virsh This command implements the managed save operation * tools/virsh.c: new command * tools/virsh.pod: documentation diff --git a/tools/virsh.c b/tools/virsh.c index 5c56fa6..ca1a003 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1333,6 +1333,44 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) } /* + * "managedsave" command + */ +static const vshCmdInfo info_managedsave[] = { + {"help", N_("managed save of a domain state")}, + {"desc", N_("Save and stop a running domain, so libvirt can restart it from the same state")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_managedsave[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {NULL, 0, 0, NULL} +}; + +static int +cmdManagedSave(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom; + char *name; + int ret = TRUE; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + return FALSE; + + if (virDomainManagedSave(dom, 0) == 0) { + vshPrint(ctl, _("Domain %s state saved by libvirt\n"), name); + } else { + vshError(ctl, _("Failed to save domain %s state"), name); + ret = FALSE; + } + + virDomainFree(dom); + return ret; +} + +/* * "schedinfo" command */ static const vshCmdInfo info_schedinfo[] = { @@ -8208,6 +8246,8 @@ static const vshCmdDef commands[] = { {"iface-start", cmdInterfaceStart, opts_interface_start, info_interface_start}, {"iface-destroy", cmdInterfaceDestroy, opts_interface_destroy, info_interface_destroy}, + {"managedsave", cmdManagedSave, opts_managedsave, info_managedsave}, + {"nodeinfo", cmdNodeinfo, NULL, info_nodeinfo}, {"nodedev-list", cmdNodeListDevices, opts_node_list_devices, info_node_list_devices}, diff --git a/tools/virsh.pod b/tools/virsh.pod index fc4a70c..62395d7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -335,6 +335,12 @@ except that it does some error checking. The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>. +=item B<managedsave> I<domain-id> + +Ask libvirt to save a running domain state in a place managed by libvirt. +If libvirt is asked to restart the domain later on it will resume it from +the saved domain state (and the state is discarded). + =item B<migrate> optional I<--live> I<--suspend> I<domain-id> I<desturi> I<migrateuri> Migrate domain to another host. Add --live for live migration; --suspend -- 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 04/02/2010 01:57 PM, Daniel Veillard wrote:
[v2 with typo and doc improvement]
Add a managedsave command to virsh
This command implements the managed save operation
* tools/virsh.c: new command * tools/virsh.pod: documentation
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/02/2010 03:51 PM, Daniel Veillard wrote:
Change w.r.t. version 1 is the saving path under /var/lib and not /var/run, the restore bug fix spotted by Laine
My test verifies this bug is fixed. I am s now seeing a different problem, but I'm fairly certain it's unrelated to your patch, and is instead a race condition in the qemu domain restore code that I've seen before (and that seems to not show up on other people's hardware) - if the domain is paused prior to the save, it will restore properly, and function once unpaused, but if it's running during the save, restore will have strange results unless I step through the code with gdb. So, from a functional point of view, I can ACK this. (I haven't don't the thorough review of the source that Eric has, though.)
On Sat, Apr 03, 2010 at 02:10:50AM -0400, Laine Stump wrote:
On 04/02/2010 03:51 PM, Daniel Veillard wrote:
Change w.r.t. version 1 is the saving path under /var/lib and not /var/run, the restore bug fix spotted by Laine
My test verifies this bug is fixed.
I am s now seeing a different problem, but I'm fairly certain it's unrelated to your patch, and is instead a race condition in the qemu domain restore code that I've seen before (and that seems to not show up on other people's hardware) - if the domain is paused prior to the save, it will restore properly, and function once unpaused, but if it's running during the save, restore will have strange results unless I step through the code with gdb.
So, from a functional point of view, I can ACK this. (I haven't don't the thorough review of the source that Eric has, though.)
Okay, after a final cleanup based on Eric's review, I pushed this patch set, thanks ! 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 (5)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump