Re: [Qemu-devel] [PATCH 3/4] Block: readonly changes

[Kevin Wolf]

Am 04.02.2010 10:36, schrieb Naphtali Sprei:
> Jamie Lokier wrote:
>> Naphtali Sprei wrote:
>>> Open backing file for read-only
>>> During commit upgrade to read-write and back at end to read-only
>>
>>> +    if (ro) { /* re-open as RO */
>>> +        bs_ro = bdrv_new("");
>>> +        ret = bdrv_open2(bs_ro, bs->backing_hd->filename,  
>>> bs->backing_hd->open_flags & ~BDRV_O_RDWR, NULL);
>>> +        if (ret < 0) {
>>> +            bdrv_delete(bs_ro);
>>> +            return -EACCES;
>>> +        }
>>> +        bdrv_close(bs->backing_hd);
>>> +        qemu_free(bs->backing_hd);
>>> +        bs->backing_hd = bs_ro;
>>> +        bs->backing_hd->keep_read_only = 0;
>>> +    }
>>
>> I think the general idea is perfect.
>>
>> A couple of concerns come to mind.
>>
>> 1. When changing read-write to read-only, if the backing file is a complex
>>    format like qcow2 (or any others), is it possible for this bdrv_open2()
>>    to read metadata such as format indexes, and even data, _before_
>>    all changes maintained by bs->backing_hd have been written to the file?
>>
>>    (If the complex formats were like real filesystems and had a "mounted"
>>    flags as real filesystems tend to, then it would be an issue, but I'm
>>    not aware of any of them doing that.)
>>
>>    Are there any such issues when switching from read-only to read-write
>>    earlier?  (It seems unlikely).
>>
> 
> Good question. I looked at some of the formats (qcow, qcow2, vmdk) and didn't 
> see
> anything problematic, since in the close function I didn't see any changes to 
> the real file,
> only in-memory data and memory free.
> But an answer from an expert would help.

It would probably work currently, but I think it's still an invalid
assumption. To make this code safe in terms of avoiding corruption, we
should first close the old bs and then open the new one.

Unfortunately, this means in turn that if re-opening fails, it's just
bad luck and the VM has lost its image. In this case I'm not sure if
there is any possible error recovery, so we might end up abort()ing.

>> 2. Secondly, what if the re-open gets a different file (testable with
>>    fstat()).  I know, you get what you deserve if you rename files, but
>>    still, do any of the formats which use backing files have a UUID check
>>    or something to confirm they are using the right backing file, which
>>    might be subverted by this?

qcow and qcow2 don't. I'm not sure about VMDK, but I don't think there
is any such check either. And even if such checks were in place we would
have the same problem as above: If we detected an error, what to do?
abort()?

Kevin