[libvirt] [PATCH] start: allow discarding managed save
There have been several instances of people having problems with a broken managed save file causing 'virsh start' to fail, and not being aware that they could use 'virsh managedsave-remove dom' to fix things. Making it possible to do this as part of starting a domain makes the same functionality easier to find, and one less API call. * include/libvirt/libvirt.h.in (VIR_DOMAIN_START_FORCE_BOOT): New flag. * src/libvirt.c (virDomainCreateWithFlags): Document it. * src/qemu/qemu_driver.c (qemuDomainObjStart): Alter signature. (qemuAutostartDomain, qemuDomainStartWithFlags): Update callers. * tools/virsh.c (cmdStart): Expose it in virsh. * tools/virsh.pod (start): Document it. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 3 ++ src/qemu/qemu_driver.c | 50 +++++++++++++++++++++++++---------------- tools/virsh.c | 7 +++++- tools/virsh.pod | 5 ++- 5 files changed, 43 insertions(+), 23 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 53a2f7d..c51a5b9 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -236,6 +236,7 @@ typedef enum { VIR_DOMAIN_START_PAUSED = 1 << 0, /* Launch guest in paused state */ VIR_DOMAIN_START_AUTODESTROY = 1 << 1, /* Automatically kill guest when virConnectPtr is closed */ VIR_DOMAIN_START_BYPASS_CACHE = 1 << 2, /* Avoid file system cache pollution */ + VIR_DOMAIN_START_FORCE_BOOT = 1 << 3, /* Boot, discarding any managed save */ } virDomainCreateFlags; diff --git a/src/libvirt.c b/src/libvirt.c index 65a099b..80c8b7c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -7081,6 +7081,9 @@ error: * the file, or fail if it cannot do so for the given system; this can allow * less pressure on file system cache, but also risks slowing loads from NFS. * + * If the VIR_DOMAIN_START_FORCE_BOOT flag is set, then any managed save + * file for this domain is discarded, and the domain boots from scratch. + * * Returns 0 in case of success, -1 in case of error */ int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f21122d..5033998 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -120,9 +120,7 @@ static int qemudShutdown(void); static int qemuDomainObjStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - bool start_paused, - bool autodestroy, - bool bypass_cache); + unsigned int flags); static int qemudDomainGetMaxVcpus(virDomainPtr dom); @@ -135,11 +133,16 @@ struct qemuAutostartData { }; static void -qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) +qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, + void *opaque) { virDomainObjPtr vm = payload; struct qemuAutostartData *data = opaque; virErrorPtr err; + int flags = 0; + + if (data->driver->autoStartBypassCache) + flags |= VIR_DOMAIN_START_BYPASS_CACHE; virDomainObjLock(vm); virResetLastError(); @@ -152,9 +155,7 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq } else { if (vm->autostart && !virDomainObjIsActive(vm) && - qemuDomainObjStart(data->conn, data->driver, vm, - false, false, - data->driver->autoStartBypassCache) < 0) { + qemuDomainObjStart(data->conn, data->driver, vm, flags) < 0) { err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, @@ -4441,12 +4442,14 @@ static int qemuDomainObjStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - bool start_paused, - bool autodestroy, - bool bypass_cache) + unsigned int flags) { int ret = -1; char *managed_save; + bool start_paused = (flags & VIR_DOMAIN_START_PAUSED) != 0; + bool autodestroy = (flags & VIR_DOMAIN_START_AUTODESTROY) != 0; + bool bypass_cache = (flags & VIR_DOMAIN_START_BYPASS_CACHE) != 0; + bool force_boot = (flags & VIR_DOMAIN_START_FORCE_BOOT) != 0; /* * If there is a managed saved state restore it instead of starting @@ -4458,13 +4461,22 @@ qemuDomainObjStart(virConnectPtr conn, goto cleanup; if (virFileExists(managed_save)) { - ret = qemuDomainObjRestore(conn, driver, vm, managed_save, - bypass_cache); + if (force_boot) { + if (unlink(managed_save) < 0) { + virReportSystemError(errno, + _("cannot remove managed save file %s"), + managed_save); + goto cleanup; + } + } else { + ret = qemuDomainObjRestore(conn, driver, vm, managed_save, + bypass_cache); - if ((ret == 0) && (unlink(managed_save) < 0)) - VIR_WARN("Failed to remove the managed state %s", managed_save); + if ((ret == 0) && (unlink(managed_save) < 0)) + VIR_WARN("Failed to remove the managed state %s", managed_save); - goto cleanup; + goto cleanup; + } } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, @@ -4493,7 +4505,8 @@ qemuDomainStartWithFlags(virDomainPtr dom, unsigned int flags) virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY | - VIR_DOMAIN_START_BYPASS_CACHE, -1); + VIR_DOMAIN_START_BYPASS_CACHE | + VIR_DOMAIN_START_FORCE_BOOT, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4515,10 +4528,7 @@ qemuDomainStartWithFlags(virDomainPtr dom, unsigned int flags) goto endjob; } - if (qemuDomainObjStart(dom->conn, driver, vm, - (flags & VIR_DOMAIN_START_PAUSED) != 0, - (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, - (flags & VIR_DOMAIN_START_BYPASS_CACHE) != 0) < 0) + if (qemuDomainObjStart(dom->conn, driver, vm, flags) < 0) goto endjob; ret = 0; diff --git a/tools/virsh.c b/tools/virsh.c index 15b9bdd..49034ae 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1537,9 +1537,12 @@ static const vshCmdOptDef opts_start[] = { {"console", VSH_OT_BOOL, 0, N_("attach to console after creation")}, #endif {"paused", VSH_OT_BOOL, 0, N_("leave the guest paused after creation")}, - {"autodestroy", VSH_OT_BOOL, 0, N_("automatically destroy the guest when virsh disconnects")}, + {"autodestroy", VSH_OT_BOOL, 0, + N_("automatically destroy the guest when virsh disconnects")}, {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when loading")}, + {"force-boot", VSH_OT_BOOL, 0, + N_("force fresh boot by discarding any managed save")}, {NULL, 0, 0, NULL} }; @@ -1572,6 +1575,8 @@ cmdStart(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_START_AUTODESTROY; if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_START_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "force-boot")) + flags |= VIR_DOMAIN_START_FORCE_BOOT; /* Prefer older API unless we have to pass a flag. */ if ((flags ? virDomainCreateWithFlags(dom, flags) diff --git a/tools/virsh.pod b/tools/virsh.pod index 81d7a1e..2cd0f73 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -890,7 +890,7 @@ The exact behavior of a domain when it shuts down is set by the I<on_shutdown> parameter in the domain's XML definition. =item B<start> I<domain-name> [I<--console>] [I<--paused>] [I<--autodestroy>] -[I<--bypass-cache>] +[I<--bypass-cache>] [I<--force-boot>] Start a (previously defined) inactive domain, either from the last B<managedsave> state, or via a fresh boot if no managedsave state is @@ -901,7 +901,8 @@ If I<--autodestroy> is requested, then the guest will be automatically destroyed when virsh closes its connection to libvirt, or otherwise exits. If I<--bypass-cache> is specified, and managedsave state exists, the restore will avoid the file system cache, although this may slow -down the operation. +down the operation. If I<--force-boot> is specified, then any +managedsave state is discarded and a fresh boot occurs. =item B<suspend> I<domain-id> -- 1.7.4.4
On Sat, Aug 27, 2011 at 05:21:11PM -0600, Eric Blake wrote:
There have been several instances of people having problems with a broken managed save file causing 'virsh start' to fail, and not being aware that they could use 'virsh managedsave-remove dom' to fix things. Making it possible to do this as part of starting a domain makes the same functionality easier to find, and one less API call.
* include/libvirt/libvirt.h.in (VIR_DOMAIN_START_FORCE_BOOT): New flag. * src/libvirt.c (virDomainCreateWithFlags): Document it. * src/qemu/qemu_driver.c (qemuDomainObjStart): Alter signature. (qemuAutostartDomain, qemuDomainStartWithFlags): Update callers. * tools/virsh.c (cmdStart): Expose it in virsh. * tools/virsh.pod (start): Document it. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 3 ++ src/qemu/qemu_driver.c | 50 +++++++++++++++++++++++++---------------- tools/virsh.c | 7 +++++- tools/virsh.pod | 5 ++- 5 files changed, 43 insertions(+), 23 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 53a2f7d..c51a5b9 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -236,6 +236,7 @@ typedef enum { VIR_DOMAIN_START_PAUSED = 1 << 0, /* Launch guest in paused state */ VIR_DOMAIN_START_AUTODESTROY = 1 << 1, /* Automatically kill guest when virConnectPtr is closed */ VIR_DOMAIN_START_BYPASS_CACHE = 1 << 2, /* Avoid file system cache pollution */ + VIR_DOMAIN_START_FORCE_BOOT = 1 << 3, /* Boot, discarding any managed save */ } virDomainCreateFlags;
diff --git a/src/libvirt.c b/src/libvirt.c index 65a099b..80c8b7c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -7081,6 +7081,9 @@ error: * the file, or fail if it cannot do so for the given system; this can allow * less pressure on file system cache, but also risks slowing loads from NFS. * + * If the VIR_DOMAIN_START_FORCE_BOOT flag is set, then any managed save + * file for this domain is discarded, and the domain boots from scratch. + * * Returns 0 in case of success, -1 in case of error */ int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f21122d..5033998 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -120,9 +120,7 @@ static int qemudShutdown(void); static int qemuDomainObjStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - bool start_paused, - bool autodestroy, - bool bypass_cache); + unsigned int flags);
static int qemudDomainGetMaxVcpus(virDomainPtr dom);
@@ -135,11 +133,16 @@ struct qemuAutostartData { };
static void -qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaque) +qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, + void *opaque) { virDomainObjPtr vm = payload; struct qemuAutostartData *data = opaque; virErrorPtr err; + int flags = 0; + + if (data->driver->autoStartBypassCache) + flags |= VIR_DOMAIN_START_BYPASS_CACHE;
virDomainObjLock(vm); virResetLastError(); @@ -152,9 +155,7 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq } else { if (vm->autostart && !virDomainObjIsActive(vm) && - qemuDomainObjStart(data->conn, data->driver, vm, - false, false, - data->driver->autoStartBypassCache) < 0) { + qemuDomainObjStart(data->conn, data->driver, vm, flags) < 0) { err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, @@ -4441,12 +4442,14 @@ static int qemuDomainObjStart(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - bool start_paused, - bool autodestroy, - bool bypass_cache) + unsigned int flags) { int ret = -1; char *managed_save; + bool start_paused = (flags & VIR_DOMAIN_START_PAUSED) != 0; + bool autodestroy = (flags & VIR_DOMAIN_START_AUTODESTROY) != 0; + bool bypass_cache = (flags & VIR_DOMAIN_START_BYPASS_CACHE) != 0; + bool force_boot = (flags & VIR_DOMAIN_START_FORCE_BOOT) != 0;
/* * If there is a managed saved state restore it instead of starting @@ -4458,13 +4461,22 @@ qemuDomainObjStart(virConnectPtr conn, goto cleanup;
if (virFileExists(managed_save)) { - ret = qemuDomainObjRestore(conn, driver, vm, managed_save, - bypass_cache); + if (force_boot) { + if (unlink(managed_save) < 0) { + virReportSystemError(errno, + _("cannot remove managed save file %s"), + managed_save); + goto cleanup; + } + } else { + ret = qemuDomainObjRestore(conn, driver, vm, managed_save, + bypass_cache);
- if ((ret == 0) && (unlink(managed_save) < 0)) - VIR_WARN("Failed to remove the managed state %s", managed_save); + if ((ret == 0) && (unlink(managed_save) < 0)) + VIR_WARN("Failed to remove the managed state %s", managed_save);
- goto cleanup; + goto cleanup; + } }
ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, @@ -4493,7 +4505,8 @@ qemuDomainStartWithFlags(virDomainPtr dom, unsigned int flags)
virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY | - VIR_DOMAIN_START_BYPASS_CACHE, -1); + VIR_DOMAIN_START_BYPASS_CACHE | + VIR_DOMAIN_START_FORCE_BOOT, -1);
qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4515,10 +4528,7 @@ qemuDomainStartWithFlags(virDomainPtr dom, unsigned int flags) goto endjob; }
- if (qemuDomainObjStart(dom->conn, driver, vm, - (flags & VIR_DOMAIN_START_PAUSED) != 0, - (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, - (flags & VIR_DOMAIN_START_BYPASS_CACHE) != 0) < 0) + if (qemuDomainObjStart(dom->conn, driver, vm, flags) < 0) goto endjob;
ret = 0; diff --git a/tools/virsh.c b/tools/virsh.c index 15b9bdd..49034ae 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1537,9 +1537,12 @@ static const vshCmdOptDef opts_start[] = { {"console", VSH_OT_BOOL, 0, N_("attach to console after creation")}, #endif {"paused", VSH_OT_BOOL, 0, N_("leave the guest paused after creation")}, - {"autodestroy", VSH_OT_BOOL, 0, N_("automatically destroy the guest when virsh disconnects")}, + {"autodestroy", VSH_OT_BOOL, 0, + N_("automatically destroy the guest when virsh disconnects")}, {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when loading")}, + {"force-boot", VSH_OT_BOOL, 0, + N_("force fresh boot by discarding any managed save")}, {NULL, 0, 0, NULL} };
@@ -1572,6 +1575,8 @@ cmdStart(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_START_AUTODESTROY; if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_START_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "force-boot")) + flags |= VIR_DOMAIN_START_FORCE_BOOT;
/* Prefer older API unless we have to pass a flag. */ if ((flags ? virDomainCreateWithFlags(dom, flags) diff --git a/tools/virsh.pod b/tools/virsh.pod index 81d7a1e..2cd0f73 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -890,7 +890,7 @@ The exact behavior of a domain when it shuts down is set by the I<on_shutdown> parameter in the domain's XML definition.
=item B<start> I<domain-name> [I<--console>] [I<--paused>] [I<--autodestroy>] -[I<--bypass-cache>] +[I<--bypass-cache>] [I<--force-boot>]
Start a (previously defined) inactive domain, either from the last B<managedsave> state, or via a fresh boot if no managedsave state is @@ -901,7 +901,8 @@ If I<--autodestroy> is requested, then the guest will be automatically destroyed when virsh closes its connection to libvirt, or otherwise exits. If I<--bypass-cache> is specified, and managedsave state exists, the restore will avoid the file system cache, although this may slow -down the operation. +down the operation. If I<--force-boot> is specified, then any +managedsave state is discarded and a fresh boot occurs.
=item B<suspend> I<domain-id>
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/30/2011 04:47 AM, Daniel Veillard wrote:
On Sat, Aug 27, 2011 at 05:21:11PM -0600, Eric Blake wrote:
There have been several instances of people having problems with a broken managed save file causing 'virsh start' to fail, and not being aware that they could use 'virsh managedsave-remove dom' to fix things. Making it possible to do this as part of starting a domain makes the same functionality easier to find, and one less API call.
ACK,
Thanks; pushed. Meanwhile, would you mind reviewing my followup that emulates virsh start dom --force-boot even when talking to older servers? https://www.redhat.com/archives/libvir-list/2011-August/msg01440.html -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Managed save was added in 0.8.0, virDomainCreateWithFlags in 0.8.2, and FORCE_BOOT in 0.9.5. The virsh flag is more useful if we emulate it for all older servers (note that if a hypervisor fails the query for a managed save image, then it does not have one to be removed, so the flag can be safely ignored). * tools/virsh.c (cmdStart): Add emulation for new flag. --- tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 41 insertions(+), 10 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 49034ae..bbd54de 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1550,11 +1550,12 @@ static bool cmdStart(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - bool ret = true; + bool ret = false; #ifndef WIN32 int console = vshCommandOptBool(cmd, "console"); #endif unsigned int flags = VIR_DOMAIN_NONE; + int rc; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1578,19 +1579,49 @@ cmdStart(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "force-boot")) flags |= VIR_DOMAIN_START_FORCE_BOOT; + /* We can emulate force boot, even for older servers that reject it. */ + if (flags & VIR_DOMAIN_START_FORCE_BOOT) { + if (virDomainCreateWithFlags(dom, flags) == 0) + goto started; + if (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_INVALID_ARG) { + virshReportError(ctl); + goto cleanup; + } + virFreeError(last_error); + last_error = NULL; + rc = virDomainHasManagedSaveImage(dom, 0); + if (rc < 0) { + /* No managed save image to remove */ + virFreeError(last_error); + last_error = NULL; + } else if (rc > 0) { + if (virDomainManagedSaveRemove(dom, 0) < 0) { + virshReportError(ctl); + goto cleanup; + } + } + flags &= ~VIR_DOMAIN_START_FORCE_BOOT; + } + /* Prefer older API unless we have to pass a flag. */ if ((flags ? virDomainCreateWithFlags(dom, flags) - : virDomainCreate(dom)) == 0) { - vshPrint(ctl, _("Domain %s started\n"), - virDomainGetName(dom)); -#ifndef WIN32 - if (console) - cmdRunConsole(ctl, dom, NULL); -#endif - } else { + : virDomainCreate(dom)) < 0) { vshError(ctl, _("Failed to start domain %s"), virDomainGetName(dom)); - ret = false; + goto cleanup; } + +started: + vshPrint(ctl, _("Domain %s started\n"), + virDomainGetName(dom)); +#ifndef WIN32 + if (console && !cmdRunConsole(ctl, dom, NULL)) + goto cleanup; +#endif + + ret = true; + +cleanup: virDomainFree(dom); return ret; } -- 1.7.4.4
On Tue, Aug 30, 2011 at 09:54:42AM -0600, Eric Blake wrote:
Managed save was added in 0.8.0, virDomainCreateWithFlags in 0.8.2, and FORCE_BOOT in 0.9.5. The virsh flag is more useful if we emulate it for all older servers (note that if a hypervisor fails the query for a managed save image, then it does not have one to be removed, so the flag can be safely ignored).
* tools/virsh.c (cmdStart): Add emulation for new flag. --- tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 49034ae..bbd54de 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1550,11 +1550,12 @@ static bool cmdStart(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; - bool ret = true; + bool ret = false; #ifndef WIN32 int console = vshCommandOptBool(cmd, "console"); #endif unsigned int flags = VIR_DOMAIN_NONE; + int rc;
if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1578,19 +1579,49 @@ cmdStart(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "force-boot")) flags |= VIR_DOMAIN_START_FORCE_BOOT;
+ /* We can emulate force boot, even for older servers that reject it. */ + if (flags & VIR_DOMAIN_START_FORCE_BOOT) { + if (virDomainCreateWithFlags(dom, flags) == 0) + goto started; + if (last_error->code != VIR_ERR_NO_SUPPORT && + last_error->code != VIR_ERR_INVALID_ARG) { + virshReportError(ctl); + goto cleanup; + } + virFreeError(last_error); + last_error = NULL; + rc = virDomainHasManagedSaveImage(dom, 0); + if (rc < 0) { + /* No managed save image to remove */ + virFreeError(last_error); + last_error = NULL; + } else if (rc > 0) { + if (virDomainManagedSaveRemove(dom, 0) < 0) { + virshReportError(ctl); + goto cleanup; + } + } + flags &= ~VIR_DOMAIN_START_FORCE_BOOT; + } + /* Prefer older API unless we have to pass a flag. */ if ((flags ? virDomainCreateWithFlags(dom, flags) - : virDomainCreate(dom)) == 0) { - vshPrint(ctl, _("Domain %s started\n"), - virDomainGetName(dom)); -#ifndef WIN32 - if (console) - cmdRunConsole(ctl, dom, NULL); -#endif - } else { + : virDomainCreate(dom)) < 0) { vshError(ctl, _("Failed to start domain %s"), virDomainGetName(dom)); - ret = false; + goto cleanup; } + +started: + vshPrint(ctl, _("Domain %s started\n"), + virDomainGetName(dom)); +#ifndef WIN32 + if (console && !cmdRunConsole(ctl, dom, NULL)) + goto cleanup; +#endif + + ret = true; + +cleanup: virDomainFree(dom); return ret; }
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/01/2011 08:43 AM, Daniel P. Berrange wrote:
On Tue, Aug 30, 2011 at 09:54:42AM -0600, Eric Blake wrote:
Managed save was added in 0.8.0, virDomainCreateWithFlags in 0.8.2, and FORCE_BOOT in 0.9.5. The virsh flag is more useful if we emulate it for all older servers (note that if a hypervisor fails the query for a managed save image, then it does not have one to be removed, so the flag can be safely ignored).
* tools/virsh.c (cmdStart): Add emulation for new flag. --- tools/virsh.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 41 insertions(+), 10 deletions(-)
ACK
Pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
In a SELinux or root-squashing NFS environment, libvirt has to go through some hoops to create a new file that qemu can then open() by name. Snapshots are a case where we want to guarantee an empty file that qemu can open; also, reopening a save file to convert it from being marked partial to complete requires a reopen to avoid O_DIRECT headaches. Refactor some existing code to make it easier to reuse in later patches. * src/qemu/qemu_migration.h (qemuMigrationToFile): Drop parameter. * src/qemu/qemu_migration.c (qemuMigrationToFile): Let cgroup do the stat, rather than asking caller to do it and pass info down. * src/qemu/qemu_driver.c (qemuOpenFile): New function, pulled from... (qemuDomainSaveInternal): ...here. (doCoreDump, qemuDomainSaveImageOpen): Use it here as well. --- Originally posted here: https://www.redhat.com/archives/libvir-list/2011-August/msg01174.html but just a bit more tweaking made it useful for other purposes. src/qemu/qemu_driver.c | 248 ++++++++++++++++++++++---------------------- src/qemu/qemu_migration.c | 13 ++- src/qemu/qemu_migration.h | 2 +- 3 files changed, 133 insertions(+), 130 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4e8c691..9cb034b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2191,6 +2191,113 @@ qemuCompressProgramName(int compress) qemudSaveCompressionTypeToString(compress)); } +/* Internal function to properly create or open existing files, with + * ownership affected by qemu driver setup. */ +static int +qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver) +{ + struct stat sb; + bool is_reg = true; + bool need_unlink = false; + bool bypass_security = false; + int fd = -1; + uid_t uid = getuid(); + gid_t gid = getgid(); + + /* path might be a pre-existing block dev, in which case + * we need to skip the create step, and also avoid unlink + * in the failure case */ + if (oflags & O_CREAT) { + need_unlink = true; + if (stat(path, &sb) == 0) { + is_reg = !!S_ISREG(sb.st_mode); + /* If the path is regular file which exists + * already and dynamic_ownership is off, we don't + * want to change it's ownership, just open it as-is */ + if (is_reg && !driver->dynamicOwnership) { + uid = sb.st_uid; + gid = sb.st_gid; + } + } + } + + /* First try creating the file as root */ + if (!is_reg) { + fd = open(path, oflags & ~O_CREAT); + if (fd < 0) { + virReportSystemError(errno, _("unable to open %s"), path); + goto cleanup; + } + } else { + if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, + uid, gid, 0)) < 0) { + /* If we failed as root, and the error was permission-denied + (EACCES or EPERM), assume it's on a network-connected share + where root access is restricted (eg, root-squashed NFS). If the + qemu user (driver->user) is non-root, just set a flag to + bypass security driver shenanigans, and retry the operation + after doing setuid to qemu user */ + if ((fd != -EACCES && fd != -EPERM) || + driver->user == getuid()) { + virReportSystemError(-fd, + _("Failed to create file '%s'"), + path); + goto cleanup; + } + + /* On Linux we can also verify the FS-type of the directory. */ + switch (virStorageFileIsSharedFS(path)) { + case 1: + /* it was on a network share, so we'll continue + * as outlined above + */ + break; + + case -1: + virReportSystemError(errno, + _("Failed to create file " + "'%s': couldn't determine fs type"), + path); + goto cleanup; + + case 0: + default: + /* local file - log the error returned by virFileOpenAs */ + virReportSystemError(-fd, + _("Failed to create file '%s'"), + path); + goto cleanup; + } + + /* Retry creating the file as driver->user */ + + if ((fd = virFileOpenAs(path, oflags, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, + driver->user, driver->group, + VIR_FILE_OPEN_AS_UID)) < 0) { + virReportSystemError(-fd, + _("Error from child process creating '%s'"), + path); + goto cleanup; + } + + /* Since we had to setuid to create the file, and the fstype + is NFS, we assume it's a root-squashing NFS share, and that + the security driver stuff would have failed anyway */ + + bypass_security = true; + } + } +cleanup: + if (needUnlink) + *needUnlink = need_unlink; + if (bypassSecurityDriver) + *bypassSecurityDriver = bypass_security; + + return fd; +} + /* This internal function expects the driver lock to already be held on * entry and the vm must be active + locked. Vm will be unlocked and * potentially free'd after this returns (eg transient VMs are freed @@ -2209,14 +2316,11 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, int rc; virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; - struct stat sb; - bool is_reg = false; + bool needUnlink = false; size_t len; unsigned long long offset; unsigned long long pad; int fd = -1; - uid_t uid = getuid(); - gid_t gid = getgid(); int directFlag = 0; virFileDirectFdPtr directFd = NULL; @@ -2304,107 +2408,18 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, header.xml_len = len; /* Obtain the file handle. */ - /* path might be a pre-existing block dev, in which case - * we need to skip the create step, and also avoid unlink - * in the failure case */ - if (stat(path, &sb) < 0) { - /* Avoid throwing an error here, since it is possible - * that with NFS we can't actually stat() the file. - * The subsequent codepaths will still raise an error - * if a truly fatal problem is hit */ - is_reg = true; - } else { - is_reg = !!S_ISREG(sb.st_mode); - /* If the path is regular file which exists - * already and dynamic_ownership is off, we don't - * want to change it's ownership, just open it as-is */ - if (is_reg && !driver->dynamicOwnership) { - uid=sb.st_uid; - gid=sb.st_gid; - } - } - - /* First try creating the file as root */ if (bypass_cache) { directFlag = virFileDirectFdFlag(); if (directFlag < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("bypass cache unsupported by this system")); - goto endjob; - } - } - if (!is_reg) { - fd = open(path, O_WRONLY | O_TRUNC | directFlag); - if (fd < 0) { - virReportSystemError(errno, _("unable to open %s"), path); - goto endjob; - } - } else { - int oflags = O_CREAT | O_TRUNC | O_WRONLY | directFlag; - if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, - uid, gid, 0)) < 0) { - /* If we failed as root, and the error was permission-denied - (EACCES or EPERM), assume it's on a network-connected share - where root access is restricted (eg, root-squashed NFS). If the - qemu user (driver->user) is non-root, just set a flag to - bypass security driver shenanigans, and retry the operation - after doing setuid to qemu user */ - rc = fd; - if (((rc != -EACCES) && (rc != -EPERM)) || - driver->user == getuid()) { - virReportSystemError(-rc, - _("Failed to create domain save file '%s'"), - path); - goto endjob; - } - - /* On Linux we can also verify the FS-type of the directory. */ - switch (virStorageFileIsSharedFS(path)) { - case 1: - /* it was on a network share, so we'll continue - * as outlined above - */ - break; - - case -1: - virReportSystemError(errno, - _("Failed to create domain save file " - "'%s': couldn't determine fs type"), - path); - goto endjob; - break; - - case 0: - default: - /* local file - log the error returned by virFileOpenAs */ - virReportSystemError(-rc, - _("Failed to create domain save file '%s'"), - path); - goto endjob; - break; - - } - - /* Retry creating the file as driver->user */ - - if ((fd = virFileOpenAs(path, oflags, - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, - driver->user, driver->group, - VIR_FILE_OPEN_AS_UID)) < 0) { - virReportSystemError(-fd, - _("Error from child process creating '%s'"), - path); - goto endjob; - } - - /* Since we had to setuid to create the file, and the fstype - is NFS, we assume it's a root-squashing NFS share, and that - the security driver stuff would have failed anyway */ - - bypassSecurityDriver = true; + goto cleanup; } } - + fd = qemuOpenFile(driver, path, O_WRONLY | O_TRUNC | O_CREAT | directFlag, + &needUnlink, &bypassSecurityDriver); + if (fd < 0) + goto endjob; if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) goto endjob; @@ -2417,7 +2432,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, /* Perform the migration */ if (qemuMigrationToFile(driver, vm, fd, offset, path, qemuCompressProgramName(compressed), - is_reg, bypassSecurityDriver, + bypassSecurityDriver, QEMU_ASYNC_JOB_SAVE) < 0) goto endjob; if (VIR_CLOSE(fd) < 0) { @@ -2461,7 +2476,7 @@ cleanup: VIR_FORCE_CLOSE(fd); virFileDirectFdFree(directFd); VIR_FREE(xml); - if (ret != 0 && is_reg) + if (ret != 0 && needUnlink) unlink(path); if (event) qemuDomainEventQueue(driver, event); @@ -2705,18 +2720,19 @@ doCoreDump(struct qemud_driver *driver, goto cleanup; } } - if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | directFlag, - S_IRUSR | S_IWUSR)) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("failed to create '%s'"), path); + /* Core dumps usually imply last-ditch analysis efforts are + * desired, so we intentionally do not unlink even if a file was + * created. */ + if ((fd = qemuOpenFile(driver, path, + O_CREAT | O_TRUNC | O_WRONLY | directFlag, + NULL, NULL)) < 0) goto cleanup; - } if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) goto cleanup; if (qemuMigrationToFile(driver, vm, fd, 0, path, - qemuCompressProgramName(compress), true, false, + qemuCompressProgramName(compress), false, QEMU_ASYNC_JOB_DUMP) < 0) goto cleanup; @@ -3778,25 +3794,9 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, } oflags |= directFlag; } - if ((fd = virFileOpenAs(path, oflags, 0, - getuid(), getgid(), 0)) < 0) { - if ((fd != -EACCES && fd != -EPERM) || - driver->user == getuid()) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("cannot read domain image")); - goto error; - } - /* Opening as root failed, but qemu runs as a different user - * that might have better luck. */ - if ((fd = virFileOpenAs(path, oflags, 0, - driver->user, driver->group, - VIR_FILE_OPEN_AS_UID)) < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("cannot read domain image")); - goto error; - } - } + if ((fd = qemuOpenFile(driver, path, oflags, NULL, NULL)) < 0) + goto error; if (bypass_cache && (*directFd = virFileDirectFdNew(&fd, path)) == NULL) goto error; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 38b05a9..d239cc8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2690,7 +2690,7 @@ int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, int fd, off_t offset, const char *path, const char *compressor, - bool is_reg, bool bypassSecurityDriver, + bool bypassSecurityDriver, enum qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2713,11 +2713,11 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, bypassSecurityDriver = true; } else { /* Phooey - we have to fall back on exec migration, where qemu - * has to popen() the file by name. We might also stumble on + * has to popen() the file by name, and block devices have to be + * given cgroup ACL permission. We might also stumble on * a race present in some qemu versions where it does a wait() * that botches pclose. */ - if (!is_reg && - qemuCgroupControllerActive(driver, + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) { if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) != 0) { @@ -2729,7 +2729,10 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, rc = virCgroupAllowDevicePath(cgroup, path, VIR_CGROUP_DEVICE_RW); virDomainAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc); - if (rc < 0) { + if (rc == 1) { + /* path was not a device, no further need for cgroup */ + virCgroupFree(&cgroup); + } else if (rc < 0) { virReportSystemError(-rc, _("Unable to allow device %s for %s"), path, vm->def->name); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 5c6921d..ace411d 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -143,7 +143,7 @@ int qemuMigrationConfirm(struct qemud_driver *driver, int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm, int fd, off_t offset, const char *path, const char *compressor, - bool is_reg, bool bypassSecurityDriver, + bool bypassSecurityDriver, enum qemuDomainAsyncJob asyncJob) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; -- 1.7.4.4
On Tue, Aug 30, 2011 at 03:04:48PM -0600, Eric Blake wrote:
In a SELinux or root-squashing NFS environment, libvirt has to go through some hoops to create a new file that qemu can then open() by name. Snapshots are a case where we want to guarantee an empty file that qemu can open; also, reopening a save file to convert it from being marked partial to complete requires a reopen to avoid O_DIRECT headaches. Refactor some existing code to make it easier to reuse in later patches.
* src/qemu/qemu_migration.h (qemuMigrationToFile): Drop parameter. * src/qemu/qemu_migration.c (qemuMigrationToFile): Let cgroup do the stat, rather than asking caller to do it and pass info down. * src/qemu/qemu_driver.c (qemuOpenFile): New function, pulled from... (qemuDomainSaveInternal): ...here. (doCoreDump, qemuDomainSaveImageOpen): Use it here as well. ---
Originally posted here: https://www.redhat.com/archives/libvir-list/2011-August/msg01174.html
but just a bit more tweaking made it useful for other purposes.
src/qemu/qemu_driver.c | 248 ++++++++++++++++++++++---------------------- src/qemu/qemu_migration.c | 13 ++- src/qemu/qemu_migration.h | 2 +- 3 files changed, 133 insertions(+), 130 deletions(-) [...]
Okay that was a fairly complex patch, but it looks fine after scrutinity (but I didn't try to run it), ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Several users have reported problems with 'virsh start' failing because it was encountering a managed save situation where the managed save file was incomplete. Be more robust to this by using two different magic numbers, so that newer libvirt can gracefully handle an incomplete file differently than a complete one, while older libvirt will at least fail up front rather than trying to load only to have qemu fail at the end. Managed save is a convenience - it exists to preserve as much state as possible; if the state was not preserved, it is reasonable to just log that fact, then proceed with a fresh boot. On the other hand, user saves are under user control, so we must fail, but by making the failure message distinct, the user can better decide how to handle the situation of an incomplete save file. * src/qemu/qemu_driver.c (QEMUD_SAVE_PARTIAL): New define. (qemuDomainSaveInternal): Use it to mark incomplete images. (qemuDomainSaveImageOpen, qemuDomainObjRestore): Add parameter that controls what to do with partial images. (qemuDomainRestoreFlags, qemuDomainSaveImageGetXMLDesc) (qemuDomainSaveImageDefineXML, qemuDomainObjStart): Update callers. Based on an initial idea by Osier Yang. --- This should implement everything mentioned here: https://www.redhat.com/archives/libvir-list/2011-August/msg00854.html src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9cb034b..61b33fe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2111,9 +2111,12 @@ cleanup: } -#define QEMUD_SAVE_MAGIC "LibvirtQemudSave" +#define QEMUD_SAVE_MAGIC "LibvirtQemudSave" +#define QEMUD_SAVE_PARTIAL "LibvirtQemudPart" #define QEMUD_SAVE_VERSION 2 +verify(sizeof(QEMUD_SAVE_MAGIC) == sizeof(QEMUD_SAVE_PARTIAL)); + enum qemud_save_formats { QEMUD_SAVE_FORMAT_RAW = 0, QEMUD_SAVE_FORMAT_GZIP = 1, @@ -2331,7 +2334,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, } memset(&header, 0, sizeof(header)); - memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); + memcpy(header.magic, QEMUD_SAVE_PARTIAL, sizeof(header.magic)); header.version = QEMUD_SAVE_VERSION; header.compressed = compressed; @@ -2435,12 +2438,37 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, bypassSecurityDriver, QEMU_ASYNC_JOB_SAVE) < 0) goto endjob; + + /* Touch up file header to mark image complete. */ + if (bypass_cache) { + /* Reopen the file to touch up the header, since we aren't set + * up to seek backwards on directFd. The reopened fd will + * trigger a single page of file system cache pollution, but + * that's acceptable. */ + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("unable to close %s"), path); + goto endjob; + } + if (virFileDirectFdClose(directFd) < 0) + goto endjob; + fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL); + if (fd < 0) + goto endjob; + } else { + if (lseek(fd, 0, SEEK_SET) != 0) { + virReportSystemError(errno, _("unable to seek %s"), path); + goto endjob; + } + } + memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); + if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { + virReportSystemError(errno, _("unable to write %s"), path); + goto endjob; + } if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to close %s"), path); goto endjob; } - if (virFileDirectFdClose(directFd) < 0) - goto endjob; ret = 0; @@ -3769,15 +3797,16 @@ cleanup: return ret; } -/* Return -1 on failure, -2 if edit was specified but xmlin does not - * represent any changes, and opened fd on all other success. */ +/* Return -1 on most failures after raising error, -2 if edit was specified + * but xmlin does not represent any changes (no error raised), -3 if corrupt + * image was unlinked (no error raised), and opened fd on success. */ static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, bool bypass_cache, virFileDirectFdPtr *directFd, - const char *xmlin, bool edit) + const char *xmlin, bool edit, bool unlink_corrupt) { int fd; struct qemud_save_header header; @@ -3807,8 +3836,22 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, } if (memcmp(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)) != 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("image magic is incorrect")); + const char *msg = _("image magic is incorrect"); + + if (memcmp(header.magic, QEMUD_SAVE_PARTIAL, + sizeof(header.magic)) == 0) { + msg = _("save image is incomplete"); + if (unlink_corrupt) { + if (VIR_CLOSE(fd) < 0 || unlink(path) < 0) { + virReportSystemError(errno, + _("cannot remove corrupt file: %s"), + path); + goto error; + } + return -3; + } + } + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", msg); goto error; } @@ -4009,7 +4052,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd, dxml, false); + &directFd, dxml, false, false); if (fd < 0) goto cleanup; @@ -4071,7 +4114,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, qemuDriverLock(driver); fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, - NULL, false); + NULL, false, false); if (fd < 0) goto cleanup; @@ -4102,7 +4145,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, qemuDriverLock(driver); fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, - dxml, true); + dxml, true, false); if (fd < 0) { /* Check for special case of no change needed. */ @@ -4147,6 +4190,8 @@ cleanup: return ret; } +/* Return 0 on success, 1 if incomplete saved image was silently unlinked, + * and -1 on failure with error raised. */ static int qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, @@ -4161,9 +4206,12 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd, NULL, false); - if (fd < 0) + bypass_cache, &directFd, NULL, false, true); + if (fd < 0) { + if (fd == -3) + ret = 1; goto cleanup; + } if (STRNEQ(vm->def->name, def->name) || memcmp(vm->def->uuid, def->uuid, VIR_UUID_BUFLEN)) { @@ -4472,10 +4520,12 @@ qemuDomainObjStart(virConnectPtr conn, ret = qemuDomainObjRestore(conn, driver, vm, managed_save, bypass_cache); - if ((ret == 0) && (unlink(managed_save) < 0)) + if (ret == 0 && unlink(managed_save) < 0) VIR_WARN("Failed to remove the managed state %s", managed_save); - - goto cleanup; + if (ret > 0) + VIR_WARN("Ignoring incomplete managed state %s", managed_save); + else + goto cleanup; } } -- 1.7.4.4
On Tue, Aug 30, 2011 at 03:06:36PM -0600, Eric Blake wrote:
Several users have reported problems with 'virsh start' failing because it was encountering a managed save situation where the managed save file was incomplete. Be more robust to this by using two different magic numbers, so that newer libvirt can gracefully handle an incomplete file differently than a complete one, while older libvirt will at least fail up front rather than trying to load only to have qemu fail at the end.
Managed save is a convenience - it exists to preserve as much state as possible; if the state was not preserved, it is reasonable to just log that fact, then proceed with a fresh boot. On the other hand, user saves are under user control, so we must fail, but by making the failure message distinct, the user can better decide how to handle the situation of an incomplete save file.
* src/qemu/qemu_driver.c (QEMUD_SAVE_PARTIAL): New define. (qemuDomainSaveInternal): Use it to mark incomplete images. (qemuDomainSaveImageOpen, qemuDomainObjRestore): Add parameter that controls what to do with partial images. (qemuDomainRestoreFlags, qemuDomainSaveImageGetXMLDesc) (qemuDomainSaveImageDefineXML, qemuDomainObjStart): Update callers. Based on an initial idea by Osier Yang. ---
This should implement everything mentioned here: https://www.redhat.com/archives/libvir-list/2011-August/msg00854.html
src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 67 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9cb034b..61b33fe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2111,9 +2111,12 @@ cleanup: }
-#define QEMUD_SAVE_MAGIC "LibvirtQemudSave" +#define QEMUD_SAVE_MAGIC "LibvirtQemudSave" +#define QEMUD_SAVE_PARTIAL "LibvirtQemudPart" #define QEMUD_SAVE_VERSION 2
+verify(sizeof(QEMUD_SAVE_MAGIC) == sizeof(QEMUD_SAVE_PARTIAL)); + enum qemud_save_formats { QEMUD_SAVE_FORMAT_RAW = 0, QEMUD_SAVE_FORMAT_GZIP = 1, @@ -2331,7 +2334,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, }
memset(&header, 0, sizeof(header)); - memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); + memcpy(header.magic, QEMUD_SAVE_PARTIAL, sizeof(header.magic)); header.version = QEMUD_SAVE_VERSION;
header.compressed = compressed; @@ -2435,12 +2438,37 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, bypassSecurityDriver, QEMU_ASYNC_JOB_SAVE) < 0) goto endjob; + + /* Touch up file header to mark image complete. */ + if (bypass_cache) { + /* Reopen the file to touch up the header, since we aren't set + * up to seek backwards on directFd. The reopened fd will + * trigger a single page of file system cache pollution, but + * that's acceptable. */ + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("unable to close %s"), path); + goto endjob; + } + if (virFileDirectFdClose(directFd) < 0) + goto endjob; + fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL); + if (fd < 0) + goto endjob; + } else { + if (lseek(fd, 0, SEEK_SET) != 0) { + virReportSystemError(errno, _("unable to seek %s"), path); + goto endjob; + } + } + memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); + if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { + virReportSystemError(errno, _("unable to write %s"), path); + goto endjob; + } if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to close %s"), path); goto endjob; } - if (virFileDirectFdClose(directFd) < 0) - goto endjob;
ret = 0;
@@ -3769,15 +3797,16 @@ cleanup: return ret; }
-/* Return -1 on failure, -2 if edit was specified but xmlin does not - * represent any changes, and opened fd on all other success. */ +/* Return -1 on most failures after raising error, -2 if edit was specified + * but xmlin does not represent any changes (no error raised), -3 if corrupt + * image was unlinked (no error raised), and opened fd on success. */ static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, bool bypass_cache, virFileDirectFdPtr *directFd, - const char *xmlin, bool edit) + const char *xmlin, bool edit, bool unlink_corrupt) { int fd; struct qemud_save_header header; @@ -3807,8 +3836,22 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, }
if (memcmp(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)) != 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("image magic is incorrect")); + const char *msg = _("image magic is incorrect"); + + if (memcmp(header.magic, QEMUD_SAVE_PARTIAL, + sizeof(header.magic)) == 0) { + msg = _("save image is incomplete"); + if (unlink_corrupt) { + if (VIR_CLOSE(fd) < 0 || unlink(path) < 0) { + virReportSystemError(errno, + _("cannot remove corrupt file: %s"), + path); + goto error; + } + return -3; + } + } + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", msg); goto error; }
@@ -4009,7 +4052,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd, dxml, false); + &directFd, dxml, false, false); if (fd < 0) goto cleanup;
@@ -4071,7 +4114,7 @@ qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, qemuDriverLock(driver);
fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, - NULL, false); + NULL, false, false);
if (fd < 0) goto cleanup; @@ -4102,7 +4145,7 @@ qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, qemuDriverLock(driver);
fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, - dxml, true); + dxml, true, false);
if (fd < 0) { /* Check for special case of no change needed. */ @@ -4147,6 +4190,8 @@ cleanup: return ret; }
+/* Return 0 on success, 1 if incomplete saved image was silently unlinked, + * and -1 on failure with error raised. */ static int qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, @@ -4161,9 +4206,12 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL;
fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd, NULL, false); - if (fd < 0) + bypass_cache, &directFd, NULL, false, true); + if (fd < 0) { + if (fd == -3) + ret = 1; goto cleanup; + }
if (STRNEQ(vm->def->name, def->name) || memcmp(vm->def->uuid, def->uuid, VIR_UUID_BUFLEN)) { @@ -4472,10 +4520,12 @@ qemuDomainObjStart(virConnectPtr conn, ret = qemuDomainObjRestore(conn, driver, vm, managed_save, bypass_cache);
- if ((ret == 0) && (unlink(managed_save) < 0)) + if (ret == 0 && unlink(managed_save) < 0) VIR_WARN("Failed to remove the managed state %s", managed_save); - - goto cleanup; + if (ret > 0) + VIR_WARN("Ignoring incomplete managed state %s", managed_save); + else + goto cleanup; } }
Looks and sounds okay, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 09/01/2011 09:48 PM, Daniel Veillard wrote:
On Tue, Aug 30, 2011 at 03:06:36PM -0600, Eric Blake wrote:
Several users have reported problems with 'virsh start' failing because it was encountering a managed save situation where the managed save file was incomplete. Be more robust to this by using two different magic numbers, so that newer libvirt can gracefully handle an incomplete file differently than a complete one, while older libvirt will at least fail up front rather than trying to load only to have qemu fail at the end.
Looks and sounds okay, ACK,
Thanks; I've now pushed 3/1 and 4/1. And thanks to Osier for first suggesting the idea that turned into these patches. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake