On 08/29/2014 02:33 AM, Hu Tao wrote: > Signed-off-by: Hu Tao <hu...@cn.fujitsu.com> > --- > block/archipelago.c | 3 ++- > block/cow.c | 3 ++- > block/gluster.c | 4 +-- > block/iscsi.c | 4 +-- > block/nfs.c | 3 ++- > block/qcow.c | 3 ++- > block/qcow2.c | 3 ++- > block/qed.c | 3 ++- > block/raw-posix.c | 8 +++--- > block/raw-win32.c | 4 +-- > block/rbd.c | 3 ++- > block/sheepdog.c | 3 ++- > block/ssh.c | 3 ++- > block/vdi.c | 3 ++- > block/vhdx.c | 3 ++- > block/vmdk.c | 3 ++- > block/vpc.c | 3 ++- > tests/qemu-iotests/104 | 57 > ++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/104.out | 12 +++++++++ > tests/qemu-iotests/common.filter | 21 +++++++++++++++ > tests/qemu-iotests/group | 1 + > 21 files changed, 127 insertions(+), 23 deletions(-) > create mode 100755 tests/qemu-iotests/104 > create mode 100644 tests/qemu-iotests/104.out
Code looks correct. However, I have a possible design concern: > +++ b/block/qcow2.c > @@ -1921,7 +1921,8 @@ static int qcow2_create(const char *filename, QemuOpts > *opts, Error **errp) > int ret; > > /* Read out options */ > - sectors = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / 512; > + sectors = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), > + BDRV_SECTOR_SIZE); At least the qcow2 file format describes virtual file size in bytes (offsets 24-31 in the header). It's nice that we are guaranteeing at least the size requested by the user, but by doing the division, we are unable to write the user's actual requested size into the file. I'm okay if all files _we_ create are aligned in size. But do we have any lurking bugs if a qcow2 file created by some other means has a non-aligned size? The recent work on the image fuzzer should be able to easily create such images. Meanwhile, I don't know of any physical hardware that has an unaligned size. Should we tighten the qcow2 spec to forbid a size that is not a multiple of the sector size, and teach qemu to forcefully reject such images as invalid? Does the qcow2 format need a way to flag whether an image has 512 vs. 4096-byte sectors (that is, what happens with an image that is 512-aligned but not 4096-aligned when used in an emulation that only exposes 4096-byte alignment to the guest)? On the other hand, if we _do_ have code that gracefully handles unaligned size from externally-created sources, would it be worth allowing qemu-img to also create unaligned images directly, instead of having to resort to the image fuzzer to create such images, all so that it is easier to test our code and ensure it doesn't bit-rot? It might be lower-maintenance to just require that such images be rejected as broken. > +++ b/block/raw-posix.c > @@ -1369,8 +1369,8 @@ static int raw_create(const char *filename, QemuOpts > *opts, Error **errp) > strstart(filename, "file:", &filename); > > /* Read out options */ > - total_size = > - qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0) / BDRV_SECTOR_SIZE; > + total_size = DIV_ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), > + BDRV_SECTOR_SIZE); > nocow = qemu_opt_get_bool(opts, BLOCK_OPT_NOCOW, false); Again, I'm okay with the idea of always treating guest images as aligned multiples of sectors. But it's very easy to create an unaligned raw file, and pass that to qemu. So even though we don't create such files via qemu-img, it's still worth ensuring that the code behaves correctly when such an image is used directly or as a backing file (as a backing file of a larger qcow2 image, bytes beyond the actual size must read as padded to 0; as a direct file, I'm not sure that the guest OS can do a partial sector read, so I'd lean more towards an I/O error when trying to read a full sector when only a partial sector is available the same as when trying to read a sector completely beyond the disk bounds, but I could also live with a successful read with the tail being padded to 0; similarly, we'd have to ensure that writes either error out, or guarantee that the tail is ignored and only the valid portion of the sector is written). Meanwhile, although we own the qcow2 spec, and can therefore choose to reject unaligned size in qcow2 files as invalid format, we are not in charge of the spec for several other file formats, but probably ought to handle those the same way as other vendors. For example, is it possible to create a VMDK file with unaligned size, and if so, how does it behave? > > +_filter_img_info() > +{ > + sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \ This works, so I'm not asking for a change; but I prefer | over # when writing a sed expression that includes a filename expansion, as in: sed -e "s|$IMGPROTO:$TEST_DIR|TEST_DIR|g" Why? Because it's easy (even if uncommon) to create file names with # embedded in them (if $TEST_DIR is absolute, I just have to have /path/to/my#odd/directory with no shell quoting required as the place where I unpack and configure qemu), but file names with | can only be created with shell quoting, so they are less common. (The complaint gets more relevant for people doing s,,, with file names, because such files are more common [anyone remember CVS?]) At any rate, if there are still design issues to resolve (such as tightening the qcow2 spec), they can be separate patches. So I'm okay with: Reviewed-by: Eric Blake <ebl...@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature