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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list