On 15.04.20 21:02, Alberto Garcia wrote: > Although we cannot create these images with qemu-img it is still > possible to do it using an external tool. QEMU should refuse to open > them until the data-file-raw bit is cleared with 'qemu-img check'. > > Signed-off-by: Alberto Garcia <be...@igalia.com> > --- > block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/244 | 13 +++++++++++++ > tests/qemu-iotests/244.out | 14 ++++++++++++++ > 3 files changed, 66 insertions(+)
Sorry for the long delay. :/ 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.) So when the user manually gives a backing file without one having been given by the image file, we run into the same problem. Now I’m not quite sure what the problem is here. We could make this patch more extensive and also forbid this case. But I think there actually shouldn’t be a problem. The qcow2 driver shouldn’t fall back to a backing file for raw external data files. But how exactly should that be implemented? I think the correct way would be to preallocate all metadata whenever data_file_raw=on – the qcow2 spec doesn’t say to ignore the metadata with data_file_raw=on, it just says that the data read from the qcow2 file must match that read from the external data file. (I seem to remember I proposed this before, but I don’t know exactly...) (In contrast, I don’t think it would be correct to just treat unallocated clusters as zero whenever data_file_raw=on.) What do you think? Should we force preallocation with data_file_raw=on, and then just take this patch, even though it still lets users give backing files to a qcow2 file at runtime without error? (Except the backing file wouldn’t have an effect, then.)
signature.asc
Description: OpenPGP digital signature