On Wed, Jan 8, 2020 at 7:52 PM Alberto Garcia <be...@igalia.com> wrote: > > The qcow2 header specifies the virtual size of the image in bytes, but > BlockDriverState stores it as a number of 512-byte sectors. > > If the user tries to create an image with a size that is not a > multiple of the sector size then this is fixed on creation by > silently rounding the image size up (see commit c2eb918e32). > qcow2_co_truncate() is more strict and returns an error instead. > > However when an image is opened the virtual size is rounded down, > which means that trying to access the last few advertised bytes will > result in an error. As seen above QEMU cannot create such images and > there's no good use case that would require us to try to handle them > so let's just treat them as unsupported.
Making error impossible is best. Can we require multiple of 4k to avoid unaligned read/write at the end of an image aligned to 512 bytes on storage with 4k sector size? > > Signed-off-by: Alberto Garcia <be...@igalia.com> > --- > block/qcow2.c | 7 +++++++ > docs/interop/qcow2.txt | 3 ++- > tests/qemu-iotests/080 | 7 +++++++ > tests/qemu-iotests/080.out | 4 ++++ > 4 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 7fbaac8457..92474849db 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1326,6 +1326,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState > *bs, QDict *options, > goto fail; > } > > + if (header.size % BDRV_SECTOR_SIZE) { > + error_setg(errp, "Virtual size is not a multiple of %u", > + (unsigned) BDRV_SECTOR_SIZE); > + ret = -EINVAL; > + goto fail; > + } > + > if (header.header_length > sizeof(header)) { > s->unknown_header_fields_size = header.header_length - > sizeof(header); > s->unknown_header_fields = g_malloc(s->unknown_header_fields_size); > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > index af5711e533..891f5662d8 100644 > --- a/docs/interop/qcow2.txt > +++ b/docs/interop/qcow2.txt > @@ -40,7 +40,8 @@ The first cluster of a qcow2 image contains the file header: > with larger cluster sizes. > > 24 - 31: size > - Virtual disk size in bytes. > + Virtual disk size in bytes. qemu can only handle > + sizes that are a multiple of 512 bytes. > > Note: qemu has an implementation limit of 32 MB as > the maximum L1 table size. With a 2 MB cluster > diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080 > index 4bcb5021e8..2563b2c052 100755 > --- a/tests/qemu-iotests/080 > +++ b/tests/qemu-iotests/080 > @@ -48,6 +48,7 @@ header_size=104 > > offset_backing_file_offset=8 > offset_backing_file_size=16 > +offset_virtual_size=24 > offset_l1_size=36 > offset_l1_table_offset=40 > offset_refcount_table_offset=48 > @@ -197,6 +198,12 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" > "\x10\x00\x00\x00" > { $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir > _check_test_img > > +echo > +echo "== Image size not a multiple of the sector size ==" > +_make_test_img 64k Logging the change here would make the test and the output more clear: echo "modifying virtual size to 65535" > +poke_file "$TEST_IMG" "$offset_virtual_size" > "\x00\x00\x00\x00\x00\x00\xff\xff" > +{ $QEMU_IO -c "write 65530 1" $TEST_IMG; } 2>&1 | _filter_qemu_io | > _filter_testdir > + > # success, all done > echo "*** done" > rm -f $seq.full > diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out > index 45ab01db8e..e1c969e2ba 100644 > --- a/tests/qemu-iotests/080.out > +++ b/tests/qemu-iotests/080.out > @@ -104,4 +104,8 @@ Data may be corrupted, or further writes to the image may > corrupt it. > > 3 leaked clusters were found on the image. > This means waste of disk space, but no harm to data. > + > +== Image size not a multiple of the sector size == > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 > +qemu-io: can't open device TEST_DIR/t.qcow2: Virtual size is not a multiple > of 512 The output is confusing, looks like we created image with aligned size, and on the next line it complains about the size. > *** done > -- > 2.20.1 > > Nir