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

[Osier Yang]

于 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
>>
>>
>