[libvirt] [PATCH v3 1/2] qemu: Resolve data loss and data corruption of domain restoring.

[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.

> +
>  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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110406/efe7a972/attachment-0001.sig>