[libvirt] [PATCH v5] qemu: Remove the managed state file only if restoring succeeded
1) 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"). The fix for is to unlink the managed state file only if restoring succeeded. 2) For "virsh save dom; virsh restore dom;", it can cause data corruption if one reuse the saved state file for restoring. Add doc to tell user about it. 3) In "qemuDomainObjStart", if "managed_save" is NULL, we shouldn't fallback to start the domain, skipping it to cleanup as a incidental fix. Discovered by Eric. --- src/qemu/qemu_driver.c | 12 +++++++----- tools/virsh.pod | 6 +++++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..a84780b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3423,18 +3423,20 @@ 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. The old state is removed once the restoring succeeded. */ managed_save = qemuDomainManagedSavePath(driver, vm); + + if (!managed_save) + goto cleanup; + if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); - if (unlink(managed_save) < 0) { + if ((ret == 0) && (unlink(managed_save) < 0)) VIR_WARN("Failed to remove the managed state %s", managed_save); - } - if (ret == 0) - goto cleanup; + goto cleanup; } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, diff --git a/tools/virsh.pod b/tools/virsh.pod index f4bd294..6319373 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -546,7 +546,11 @@ 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. See I<save> for more info. + +B<Note>: To avoid corrupting file system contents within the domain, you +should not reuse the saved state file to B<restore> unless you are convinced +with reverting the domain to the previous state. =item B<save> I<domain-id> I<state-file> -- 1.7.4
On Thu, Apr 07, 2011 at 10:31:52AM +0800, Osier Yang wrote:
1) 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").
The fix for is to unlink the managed state file only if restoring succeeded.
2) For "virsh save dom; virsh restore dom;", it can cause data corruption if one reuse the saved state file for restoring. Add doc to tell user about it.
3) In "qemuDomainObjStart", if "managed_save" is NULL, we shouldn't fallback to start the domain, skipping it to cleanup as a incidental fix. Discovered by Eric.
--- src/qemu/qemu_driver.c | 12 +++++++----- tools/virsh.pod | 6 +++++- 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..a84780b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3423,18 +3423,20 @@ 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. The old state is removed once the restoring succeeded. */ managed_save = qemuDomainManagedSavePath(driver, vm); + + if (!managed_save) + goto cleanup; + if ((managed_save) && (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save);
- if (unlink(managed_save) < 0) { + if ((ret == 0) && (unlink(managed_save) < 0)) VIR_WARN("Failed to remove the managed state %s", managed_save); - }
- if (ret == 0) - goto cleanup; + goto cleanup; }
ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, diff --git a/tools/virsh.pod b/tools/virsh.pod index f4bd294..6319373 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -546,7 +546,11 @@ 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. See I<save> for more info. + +B<Note>: To avoid corrupting file system contents within the domain, you +should not reuse the saved state file to B<restore> unless you are convinced +with reverting the domain to the previous state.
=item B<save> I<domain-id> I<state-file>
ACK, 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/
于 2011年04月07日 15:21, Daniel Veillard 写道:
On Thu, Apr 07, 2011 at 10:31:52AM +0800, Osier Yang wrote:
1) 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").
The fix for is to unlink the managed state file only if restoring succeeded.
2) For "virsh save dom; virsh restore dom;", it can cause data corruption if one reuse the saved state file for restoring. Add doc to tell user about it.
3) In "qemuDomainObjStart", if "managed_save" is NULL, we shouldn't fallback to start the domain, skipping it to cleanup as a incidental fix. Discovered by Eric.
--- src/qemu/qemu_driver.c | 12 +++++++----- tools/virsh.pod | 6 +++++- 2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48fe266..a84780b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3423,18 +3423,20 @@ 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. The old state is removed once the restoring succeeded. */ managed_save = qemuDomainManagedSavePath(driver, vm); + + if (!managed_save) + goto cleanup; + if ((managed_save)&& (virFileExists(managed_save))) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save);
- if (unlink(managed_save)< 0) { + if ((ret == 0)&& (unlink(managed_save)< 0)) VIR_WARN("Failed to remove the managed state %s", managed_save); - }
- if (ret == 0) - goto cleanup; + goto cleanup; }
ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, -1, NULL, diff --git a/tools/virsh.pod b/tools/virsh.pod index f4bd294..6319373 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -546,7 +546,11 @@ 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. See I<save> for more info. + +B<Note>: To avoid corrupting file system contents within the domain, you +should not reuse the saved state file to B<restore> unless you are convinced +with reverting the domain to the previous state.
=item B<save> I<domain-id> I<state-file>
ACK, thanks !
Daniel
Thanks, applied. Osier
On 04/06/2011 08:31 PM, Osier Yang wrote:
managed_save = qemuDomainManagedSavePath(driver, vm); + + if (!managed_save) + goto cleanup; + if ((managed_save) && (virFileExists(managed_save))) {
This second check for non-NULL managed_save is now redundant.
+++ b/tools/virsh.pod @@ -546,7 +546,11 @@ 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. See I<save> for more info.
Sorry for not noticing sooner, but we could do some grammar cleanups. Does this look like a reasonable followup? diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index a84780b..0734a76 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -3430,7 +3430,7 @@ static int qemudDomainObjStart(virConnectPtr conn, if (!managed_save) goto cleanup; - if ((managed_save) && (virFileExists(managed_save))) { + if (virFileExists(managed_save)) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save); if ((ret == 0) && (unlink(managed_save) < 0)) diff --git i/tools/virsh.pod w/tools/virsh.pod index 6319373..9ce6905 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -546,11 +546,12 @@ 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 a B<virsh save> state file. See I<save> for more info. B<Note>: To avoid corrupting file system contents within the domain, you -should not reuse the saved state file to B<restore> unless you are convinced -with reverting the domain to the previous state. +should not reuse the saved state file for a second B<restore> unless +you have also reverted all storage volumes back to the same contents +as when the state file was created. =item B<save> I<domain-id> I<state-file> -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年04月07日 22:56, Eric Blake 写道:
On 04/06/2011 08:31 PM, Osier Yang wrote:
managed_save = qemuDomainManagedSavePath(driver, vm); + + if (!managed_save) + goto cleanup; + if ((managed_save)&& (virFileExists(managed_save))) {
This second check for non-NULL managed_save is now redundant.
Argh, it my fault.
+++ b/tools/virsh.pod @@ -546,7 +546,11 @@ 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. See I<save> for more info.
Sorry for not noticing sooner, but we could do some grammar cleanups.
Does this look like a reasonable followup?
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index a84780b..0734a76 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -3430,7 +3430,7 @@ static int qemudDomainObjStart(virConnectPtr conn, if (!managed_save) goto cleanup;
- if ((managed_save)&& (virFileExists(managed_save))) { + if (virFileExists(managed_save)) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save);
if ((ret == 0)&& (unlink(managed_save)< 0)) diff --git i/tools/virsh.pod w/tools/virsh.pod index 6319373..9ce6905 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -546,11 +546,12 @@ 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 a B<virsh save> state file. See I<save> for more info.
B<Note>: To avoid corrupting file system contents within the domain, you -should not reuse the saved state file to B<restore> unless you are convinced -with reverting the domain to the previous state. +should not reuse the saved state file for a second B<restore> unless +you have also reverted all storage volumes back to the same contents +as when the state file was created.
Doesn't reuse implies the "second, third, ...." B <restore>? :) But anyway, it looks more clear, good to me.
=item B<save> I<domain-id> I<state-file>
On 04/07/2011 07:09 PM, Osier Yang wrote:
于 2011年04月07日 22:56, Eric Blake 写道:
Sorry for not noticing sooner, but we could do some grammar cleanups.
Does this look like a reasonable followup?
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index a84780b..0734a76 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -3430,7 +3430,7 @@ static int qemudDomainObjStart(virConnectPtr conn, if (!managed_save) goto cleanup;
- if ((managed_save)&& (virFileExists(managed_save))) { + if (virFileExists(managed_save)) { ret = qemuDomainObjRestore(conn, driver, vm, managed_save);
+Restores a domain from a B<virsh save> state file. See I<save> for more info.
B<Note>: To avoid corrupting file system contents within the domain, you -should not reuse the saved state file to B<restore> unless you are convinced -with reverting the domain to the previous state. +should not reuse the saved state file for a second B<restore> unless +you have also reverted all storage volumes back to the same contents +as when the state file was created.
Doesn't reuse implies the "second, third, ...." B <restore>? :)
But anyway, it looks more clear, good to me.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Osier Yang