Re: [PATCH for-5.1] qcow2: Don't open images with a backing file and the data-file-raw bit

[Max Reitz]

On 18.06.20 14:03, Alberto Garcia wrote:
> On Wed 03 Jun 2020 03:53:03 PM CEST, Max Reitz wrote:
>> Sorry for the long delay. :/
> 
> And sorry for my long delay as well.
> 
>> The patch itself looks good, but I’m not sure whether it is extensive
>> enough.  Let me just jump straight to the problem:
>>
>> $ ./qemu-img create -f qcow2 \
>>     -o data_file=foo.qcow2.raw,data_file_raw=on \
>>     foo.qcow2 64M
>> (Create some file empty foo.qcow2 with external data file that’s raw)
>>
>> $ ./qemu-img create -f qcow2 backing.qcow2 64M
>> $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2
>> (Create some file filled with 42s)
>>
>> $ ./qemu-img compare foo.qcow2 foo.qcow2.raw
>> Images are identical.
>> (As expected, foo.qcow2 is identical to its raw data file)
>>
>> $ ./qemu-img compare --image-opts \
>>     file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \
>>     file.filename=foo.qcow2.raw
>> Content mismatch at offset 0!
>> (Oops.)
> 
> If two images have the same contents but then you compare them changing
> the backing file of one of them you can also get a content mismatch. How
> is this different?

It’s different in that files with data-file-raw can’t have backing files
at all.  So maybe users shouldn’t be allowed to give them backing files
at runtime either.

Or at least, if we have data-file-raw, *all* data visible on such an
image should be taken from the raw data file, never from any backing file.

> Regardless of how we should ideally handle bs->backing and data-file-raw
> (and yes I agree that it would be nice that QEMU would say "you cannot
> have a backing file and an external raw file" in all cases) I think that
> if the problem is that the user can override the backing file name and
> get different contents, then that's not a problem that we should be
> worried about.

Well, not really be worried about, but I do think it’s indicative of
some problem, though I’m not sure whether the problem is error
reporting.  I think it’s rather the fact that data-file-raw should imply
metadata preallocation.

With preallocation, we’d ensure that we always take all data from the
raw data file.  So we’d always ignore any potential backing file.

(It makes sense to guard users against giving images with data-file-raw
a backing file, and to consider such images corrupt, as done by this
patch.  But if users can force a backing file at runtime, I think
showing its contents is another bug.)

Max

signature.asc
Description: OpenPGP digital signature