[libvirt] [PATCH v3 1/2] qemu: Resolve data loss and data corruption of domain restoring.
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. (This problem exists for "virsh managedsave dom; virsh start dom"). And keeping the saved state will cause data corruption if the user modified his disks and restore the domain second time from the saved state. (Problem exists for "virsh save dom; virsh restore dom"). The fix is to: * Don't unlink()s the managed saved state if the restoring fails. * Remove the saved state if restoring succeeded. --- src/qemu/qemu_driver.c | 17 ++++++++++++----- 1 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..a618df4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3171,6 +3171,9 @@ qemuDomainRestore(virConnectPtr conn, vm = NULL; } + if ((ret == 0) && (unlink(path) < 0)) + VIR_WARN("Failed to remove the saved state %s", path); + cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); @@ -3423,18 +3426,22 @@ static int qemudDomainObjStart(virConnectPtr conn, /* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); - if (unlink(managed_save) < 0) { - VIR_WARN("Failed to remove the managed state %s", managed_save); + if (ret == 0) { + if (unlink(managed_save) < 0) + VIR_WARN("Failed to remove the managed state %s", managed_save); + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restore from the managed state %s"), + managed_save); } - if (ret == 0) - goto cleanup; + goto cleanup; } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, -- 1.7.4
--- tools/virsh.pod | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/tools/virsh.pod b/tools/virsh.pod index f4bd294..507686c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -546,7 +546,8 @@ I<on_reboot> parameter in the domain's XML definition. =item B<restore> I<state-file> -Restores a domain from an B<virsh save> state file. See I<save> for more info. +Restores a domain from an B<virsh save> state file. If restoring +succeeded, the state file will be removed. See I<save> for more info. =item B<save> I<domain-id> I<state-file> -- 1.7.4
On 04/06/2011 12:17 AM, Osier Yang wrote:
--- tools/virsh.pod | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.pod b/tools/virsh.pod index f4bd294..507686c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -546,7 +546,8 @@ I<on_reboot> parameter in the domain's XML definition.
=item B<restore> I<state-file>
-Restores a domain from an B<virsh save> state file. See I<save> for more info. +Restores a domain from an B<virsh save> state file. If restoring +succeeded, the state file will be removed. See I<save> for more info.
NACK. Based on subsequent discussion on v2, this should instead be something like: To avoid corrupting file system contents within the guest, you should not reuse a saved state file after a successful restore unless you have also taken steps to revert the guest's storage volumes back to the contents they had when the domain state file was created. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 04/06/2011 12:17 AM, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. (This problem exists for "virsh managedsave dom; virsh start dom").
And keeping the saved state will cause data corruption if the user modified his disks and restore the domain second time from the saved state. (Problem exists for "virsh save dom; virsh restore dom").
Based on subsequent conversation on v2, we need v4.
The fix is to: * Don't unlink()s the managed saved state if the restoring fails.
Good
* Remove the saved state if restoring succeeded.
Bad
+++ b/src/qemu/qemu_driver.c @@ -3171,6 +3171,9 @@ qemuDomainRestore(virConnectPtr conn, vm = NULL; }
+ if ((ret == 0) && (unlink(path) < 0)) + VIR_WARN("Failed to remove the saved state %s", path);
Drop this hunk.
+ cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); @@ -3423,18 +3426,22 @@ static int qemudDomainObjStart(virConnectPtr conn,
/* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save) && (virFileExists(managed_save))) {
If managed_save is NULL, then we should be skipping to cleanup (qemuDomainManagedSavePath already reported OOM), rather than silently falling back to normal startup.
ret = qemuDomainObjRestore(conn, driver, vm, managed_save);
- if (unlink(managed_save) < 0) { - VIR_WARN("Failed to remove the managed state %s", managed_save); + if (ret == 0) { + if (unlink(managed_save) < 0) + VIR_WARN("Failed to remove the managed state %s", managed_save); + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restore from the managed state %s"), + managed_save);
This overwrites the error message from qemuDomainObjRestore, possibly losing useful information. I think you can just drop this else clause.
}
- if (ret == 0) - goto cleanup; + goto cleanup; }
ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, -- 1.7.4
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年04月07日 03:30, Eric Blake 写道:
On 04/06/2011 12:17 AM, Osier Yang wrote:
Both "qemuDomainStartWithFlags" and "qemuAutostartDomain" try to restore the domain from managedsave'ed image if it exists (by invoking "qemuDomainObjRestore"), but it unlinks the image even if restoring fails, which causes data loss. (This problem exists for "virsh managedsave dom; virsh start dom").
And keeping the saved state will cause data corruption if the user modified his disks and restore the domain second time from the saved state. (Problem exists for "virsh save dom; virsh restore dom").
Based on subsequent conversation on v2, we need v4.
The fix is to: * Don't unlink()s the managed saved state if the restoring fails.
Good
* Remove the saved state if restoring succeeded.
Bad
+++ b/src/qemu/qemu_driver.c @@ -3171,6 +3171,9 @@ qemuDomainRestore(virConnectPtr conn, vm = NULL; }
+ if ((ret == 0)&& (unlink(path)< 0)) + VIR_WARN("Failed to remove the saved state %s", path);
Drop this hunk.
Hum
+ cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); @@ -3423,18 +3426,22 @@ static int qemudDomainObjStart(virConnectPtr conn,
/* * If there is a managed saved state restore it instead of starting - * from scratch. In any case the old state is removed. + * from scratch. */ managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save)&& (virFileExists(managed_save))) {
If managed_save is NULL, then we should be skipping to cleanup (qemuDomainManagedSavePath already reported OOM), rather than silently falling back to normal startup.
No, qemuDomainObjStart is also used by qemuDomainStartWithFlags, skipping to cleanup when managed_save is NULL will break the starting of all domains which don't have managed state file. That's risky.
ret = qemuDomainObjRestore(conn, driver, vm, managed_save);
- if (unlink(managed_save)< 0) { - VIR_WARN("Failed to remove the managed state %s", managed_save); + if (ret == 0) { + if (unlink(managed_save)< 0) + VIR_WARN("Failed to remove the managed state %s", managed_save); + } else { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to restore from the managed state %s"), + managed_save);
This overwrites the error message from qemuDomainObjRestore, possibly losing useful information. I think you can just drop this else clause.
This makes sense. Thanks
}
- if (ret == 0) - goto cleanup; + goto cleanup; }
ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, -- 1.7.4
On 04/06/2011 08:01 PM, Osier Yang wrote:
managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save)&& (virFileExists(managed_save))) {
If managed_save is NULL, then we should be skipping to cleanup (qemuDomainManagedSavePath already reported OOM), rather than silently falling back to normal startup.
No, qemuDomainObjStart is also used by qemuDomainStartWithFlags, skipping to cleanup when managed_save is NULL will break the starting of all domains which don't have managed state file.
That's risky.
Ignoring OOM is risky. If managed_save is NULL, we are out of memory, and we should fail the command; that's the only time that qemuDomainManagedSavePath returns NULL. We don't know if there was a save file or not. And even if there was not a file, we'd probably run out of memory again if we attempt normal setup. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年04月07日 10:21, Eric Blake 写道:
On 04/06/2011 08:01 PM, Osier Yang wrote:
managed_save = qemuDomainManagedSavePath(driver, vm); if ((managed_save)&& (virFileExists(managed_save))) {
If managed_save is NULL, then we should be skipping to cleanup (qemuDomainManagedSavePath already reported OOM), rather than silently falling back to normal startup.
No, qemuDomainObjStart is also used by qemuDomainStartWithFlags, skipping to cleanup when managed_save is NULL will break the starting of all domains which don't have managed state file.
That's risky.
Ignoring OOM is risky. If managed_save is NULL, we are out of memory, and we should fail the command; that's the only time that qemuDomainManagedSavePath returns NULL. We don't know if there was a save file or not. And even if there was not a file, we'd probably run out of memory again if we attempt normal setup.
Oh, I misunderstand your meaning here, forget about it, v5 comes. :)
participants (2)
-
Eric Blake -
Osier Yang