Re: [libvirt] [PATCH v3 1/2] qemu: Resolve data loss and data corruption of domain restoring.
于 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
>
>