block: Follow-up for .bdrv_co_create (part 1)
cover block: Follow-up for .bdrv_co_create (part 1)
This series adds qemu-iotests for a few more block drivers (yet more to come in another series) and fixes a few things that previous review and these tests brought up.
The only major design change is that I converted the vdi block driver from a boolean 'static' create option to the standard 'preallocation' one that other drivers are using. This seems like a good move to make while the interface isn't stable yet.
Kevin Wolf (12):
vdi: Change 'static' create option to 'preallocation' in QMP
vdi: Fix build with CONFIG_VDI_DEBUG
qemu-iotests: Test vdi image creation with QMP
qemu-iotests: Enable 025 for luks
luks: Turn another invalid assertion into check
qemu-iotests: Test invalid resize on luks
parallels: Check maximum cluster size on create
qemu-iotests: Test parallels image creation with QMP
vhdx: Require power-of-two block size on create
vhdx: Don't use error_setg_errno() with constant errno
vhdx: Check for 4 GB maximum log size on creation
qemu-iotests: Test vhdx image creation with QMP
qapi/block-core.json | 6 +- block/crypto.c | 6 +- block/parallels.c | 5 + block/vdi.c | 46 ++++-- block/vhdx.c | 17 ++- tests/qemu-iotests/025 | 9 +- tests/qemu-iotests/210 | 37 +++++ tests/qemu-iotests/211 | 256 +++++++++++++++++++++++++++++++++ tests/qemu-iotests/211.out | 98 +++++++++++++ tests/qemu-iotests/212 | 326 ++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/212.out | 111 ++++++++++++++ tests/qemu-iotests/213 | 349 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/213.out | 121 ++++++++++++++++ tests/qemu-iotests/group | 3 + 14 files changed, 1365 insertions(+), 25 deletions(-) create mode 100755 tests/qemu-iotests/211 create mode 100644 tests/qemu-iotests/211.out create mode 100755 tests/qemu-iotests/212 create mode 100644 tests/qemu-iotests/212.out create mode 100755 tests/qemu-iotests/213 create mode 100644 tests/qemu-iotests/213.out
Signature · 1 line
2.13.6
PATCH 1/12 vdi: Change 'static' create option to 'preallocation' in QMP
What static=on really does is what we call metadata preallocation for other block drivers. While we can still change the QMP interface, make it more consistent by using 'preallocation' for VDI, too.
This doesn't implement any new functionality, so the only supported preallocation modes are 'off' and 'metadata' for now.
--- qapi/block-core.json | 6 ++---- block/vdi.c | 24 ++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-)
qapi/block-core.json +2 -4
@@ -3834,16 +3834,14 @@ # # @file Node to create the image format on # @size Size of the virtual disk in bytes -# @static Whether to create a statically (true) or -# dynamically (false) allocated image -# (default: false, i.e. dynamic) +# @preallocation Preallocation mode for the new image (default: off) # # Since: 2.12 ## { 'struct': 'BlockdevCreateOptionsVdi', 'data': { 'file': 'BlockdevRef', 'size': 'size', - '*static': 'bool' } } + '*preallocation': 'PreallocMode' } } ## # @BlockdevVhdxSubformat:
block/vdi.c +22 -2
@@ -728,7 +728,7 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,int ret =0 ;uint64_t bytes =0 ;uint32_t blocks; -uint32_t image_type =VDI_TYPE_DYNAMIC ; +uint32_t image_type;VdiHeader header;size_t i;size_t bmap_size;
@@ -744,9 +744,22 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,/* Validate options and set default values */ bytes = vdi_opts->size; -if (vdi_opts->q_static) { + +if (!vdi_opts->has_preallocation) { + vdi_opts->preallocation =PREALLOC_MODE_OFF ; + } +switch (vdi_opts->preallocation) { +case PREALLOC_MODE_OFF : + image_type =VDI_TYPE_DYNAMIC ; +break ; +case PREALLOC_MODE_METADATA : image_type =VDI_TYPE_STATIC ; +break ; +default : +error_setg (errp,"Preallocation mode not supported for vdi" ); +return -EINVAL ; } +#ifndef CONFIG_VDI_STATIC_IMAGE if (image_type ==VDI_TYPE_STATIC ) { ret = -ENOTSUP ;
@@ -874,6 +887,7 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,BlockdevCreateOptions *create_options =NULL ;BlockDriverState *bs_file =NULL ;uint64_t block_size =DEFAULT_CLUSTER_SIZE ; +bool is_static = false;Visitor *v;Error *local_err =NULL ;int ret;
@@ -895,6 +909,9 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts, goto done; }#endif +if (qemu_opt_get_bool_del (opts,BLOCK_OPT_STATIC , false)) { + is_static = true; + } qdict =qemu_opts_to_qdict_filtered (opts,NULL , &vdi_create_opts, true);
@@ -913,6 +930,9 @@ static int coroutine_fn vdi_co_create_opts(const char *filename, QemuOpts *opts,qdict_put_str (qdict,"driver" ,"vdi" );qdict_put_str (qdict,"file" , bs_file->node_name); +if (is_static) { +qdict_put_str (qdict,"preallocation" ,"metadata" ); + }/* Get the QAPI object */ v =qobject_input_visitor_new_keyval (QOBJECT (qdict));
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 01/12] vdi: Change 'static' create option to 'preallocation' in QMP
Quoted 3 lines · Kevin Wolf
What static=on really does is what we call metadata preallocation for other block drivers. While we can still change the QMP interface, make it more consistent by using 'preallocation' for VDI, too.
The x- naming of the command means we don't HAVE to get this patch in for 2.12, but I agree that sooner is better than later and that doing this in 2.12 is worth the effort.
Quoted 13 lines
This doesn't implement any new functionality, so the only supported preallocation modes are 'off' and 'metadata' for now.
Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 6 ++---- block/vdi.c | 24 ++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 6 deletions(-)+++
b/qapi/block-core.json
qapi/block-core.json line 3837
Quoted 7 lines
qapi/block-core.json +1
-3@@ -3834,16 +3834,14 @@ # # @file Node to create the image format on # @size Size of the virtual disk in bytes -# @static Whether to create a statically (true) or -# dynamically (false) allocated image -# (default: false, i.e. dynamic) +# @preallocation Preallocation mode for the new image (default: off)
Do we want to document that 'full' is not supported yet? Or is just relying on the error message good enough?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 2/12 vdi: Fix build with CONFIG_VDI_DEBUG
In reply to Kevin WolfUse qemu_uuid_unparse() instead of uuid_unparse() to make vdi.c compile again when CONFIG_VDI_DEBUG is set. In order to prevent future bitrot, replace '#ifdef CONFIG_VDI_DEBUG' by 'if (VDI_DEBUG)' so that the compiler always sees the code.
--- block/vdi.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
block/vdi.c +10 -12
@@ -235,7 +235,6 @@ static void vdi_header_to_le(VdiHeader *header)qemu_uuid_bswap (&header->uuid_parent); } -#if defined(CONFIG_VDI_DEBUG )static void vdi_header_print (VdiHeader *header) {char uuid[37 ];
@@ -257,16 +256,15 @@ static void vdi_header_print(VdiHeader *header)logout ("block extra 0x%04x\n" , header->block_extra);logout ("blocks tot. 0x%04x\n" , header->blocks_in_image);logout ("blocks all. 0x%04x\n" , header->blocks_allocated); -uuid_unparse (header->uuid_image, uuid); +qemu_uuid_unparse (&header->uuid_image, uuid);logout ("uuid image %s\n" , uuid); -uuid_unparse (header->uuid_last_snap, uuid); +qemu_uuid_unparse (&header->uuid_last_snap, uuid);logout ("uuid snap %s\n" , uuid); -uuid_unparse (header->uuid_link, uuid); +qemu_uuid_unparse (&header->uuid_link, uuid);logout ("uuid link %s\n" , uuid); -uuid_unparse (header->uuid_parent, uuid); +qemu_uuid_unparse (&header->uuid_parent, uuid);logout ("uuid parent %s\n" , uuid); } -#endif static int coroutine_fn vdi_co_check(BlockDriverState *bs, BdrvCheckResult *res,BdrvCheckMode fix)
@@ -387,9 +385,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags, }vdi_header_to_cpu (&header); -#if defined(CONFIG_VDI_DEBUG ) -vdi_header_print (&header); -#endif +if (VDI_DEBUG ) { +vdi_header_print (&header); + }if (header.disk_size >VDI_DISK_SIZE_MAX ) { error_setg(errp, "Unsupported VDI image size (size is 0x%" PRIx64
@@ -825,9 +823,9 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,qemu_uuid_generate (&header.uuid_image);qemu_uuid_generate (&header.uuid_last_snap);/* There is no need to set header.uuid_link or header.uuid_parent here. */ -#if defined(CONFIG_VDI_DEBUG ) -vdi_header_print (&header); -#endif +if (VDI_DEBUG ) { +vdi_header_print (&header); + }vdi_header_to_le (&header); ret =blk_pwrite (blk, offset, &header,sizeof (header),0 );if (ret <0 ) {
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 02/12] vdi: Fix build with CONFIG_VDI_DEBUG
Quoted 10 lines · Kevin Wolf
Use
qemu_uuid_unparse()instead ofuuid_unparse()to makevdi.ccompile again whenCONFIG_VDI_DEBUGis set. In order to prevent future bitrot, replace '#ifdefCONFIG_VDI_DEBUG' by 'if (VDI_DEBUG)' so that the compiler always sees the code.Signed-off-by Kevin Wolf <[email protected]>--- block/vdi.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 3/12 qemu-iotests: Test vdi image creation with QMP
In reply to Kevin Wolf--- tests/qemu-iotests/211 | 256 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/211.out | 98 +++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 355 insertions(+) create mode 100755 tests/qemu-iotests/211 create mode 100644 tests/qemu-iotests/211.out
tests/qemu-iotests/211 new file +256 -0
@@ -0,0 +1,256 @@ +#!/bin/bash +# +# Test VDI and file image creation +# +# Copyright (C) 2018 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator [email protected] + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt vdi +_supported_proto file +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \ + | _filter_qemu | _filter_imgfmt \ + | _filter_actual_image_size +} + +echo +echo "=== Successful image creation (defaults) ===" +echo + +size=$((128 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "blockdev-add", + "arguments": { + "driver": "file", + "node-name": "imgfile", + "filename": "$TEST_IMG" + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "imgfile", + "size": $size + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific +$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map + +echo +echo "=== Successful image creation (explicit defaults) ===" +echo + +# Choose a different size to show that we got a new image +size=$((64 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_IMG" + }, + "size": $size, + "preallocation": "off" + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific +$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map + +echo +echo "=== Successful image creation (with non-default options) ===" +echo + +# Choose a different size to show that we got a new image +size=$((32 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_IMG" + }, + "size": $size, + "preallocation": "metadata" + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific +$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map + +echo +echo "=== Invalid BlockdevRef ===" +echo + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "this doesn't exist", + "size": $size + } +} +{ "execute": "quit" } +EOF + +echo +echo "=== Zero size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 0 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + +echo +echo "=== Maximum size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 562949819203584 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + +echo +echo "=== Invalid sizes ===" +echo + +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. 2^64 - 512 +# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 3. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits) +# 4. 0x1fffff8000000 (maximum image size for VDI) +# 5. (one byte more than maximum image size for VDI) + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 18446744073709551104 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 9223372036854775808 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 9223372036854775296 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 562949819203585 + } +} +{ "execute": "quit" } +EOF + +# success, all done +echo "*** done" +rm -f $seq.full +status=0
tests/qemu-iotests/211.out new file +98 -0
98 lines not on this page · raw message
tests/qemu-iotests/group +1 -0
@@ -209,3 +209,4 @@ 208 rw auto quick 209 rw auto quick 210 rw auto +211 rw auto quick
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 03/12] qemu-iotests: Test vdi image creation with QMP
Quoted 10 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/211 | 256 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/211.out | 98 +++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 355 insertions(+)create mode 100755 tests/qemu-iotests/211
create mode 100644 tests/qemu-iotests/211.out
tests/qemu-iotests/211 line 28
Quoted 9 lines
tests/qemu-iotests/211 new file +9
-0+ +# creator [email protected] + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default!
More of the useless boilerplate we've been meaning to clean up. Oh well.
tests/qemu-iotests/211 line 218
Quoted 10 lines
tests/qemu-iotests/211 new file +10
-0+# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. 2^64 - 512 +# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 3. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits) +# 4. 0x1fffff8000000 (maximum image size for VDI) +# 5. (one byte more than maximum image size for VDI)
You list 5 cases...
Quoted 20 lines
tests/qemu-iotests/211 new file +20
-0+ +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 18446744073709551104 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 9223372036854775808 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT",
tests/qemu-iotests/211 line 248
Quoted 10 lines
tests/qemu-iotests/211 new file +10
-0+ "file": "node0", + "size": 9223372036854775296 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 562949819203585 + }
...but only four x-blockdev-create. Why?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 4/12 qemu-iotests: Enable 025 for luks
In reply to Kevin WolfWe want to test resizing even for luks. The only change that is needed is to explicitly zero out new space for luks because it's undefined.
--- tests/qemu-iotests/025 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
tests/qemu-iotests/025 +8 -1
@@ -38,7 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.filter . ./common.pattern -_supported_fmt raw qcow2 qed +_supported_fmt raw qcow2 qed luks _supported_proto file sheepdog rbd nfs _supported_os Linux
@@ -62,6 +62,13 @@ length EOF _check_test_img +# bdrv_truncate() doesn't zero the new space, so we need to do that explicitly. +# We still want to test automatic zeroing for other formats even though +# bdrv_truncate() doesn't guarantee it. +if [ "$IMGFMT" == "luks" ]; then + $QEMU_IO -c "write -z $small_size $((big_size - small_size))" "$TEST_IMG" > /dev/null +fi + echo echo "=== Verifying image size after reopen" $QEMU_IO -c "length" "$TEST_IMG"
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 04/12] qemu-iotests: Enable 025 for luks
Quoted 8 lines · Kevin Wolf
We want to test resizing even for luks. The only change that is needed is to explicitly zero out new space for luks because it's undefined.
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/025 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 04/12] qemu-iotests: Enable 025 for luks
In reply to Kevin WolfQuoted 7 lines · Kevin Wolf
We want to test resizing even for luks. The only change that is needed is to explicitly zero out new space for luks because it's undefined.
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/025 | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 5/12 luks: Turn another invalid assertion into check
In reply to Kevin WolfCommit e39e959e fixed an invalid assertion in the .bdrv_length implementation, but left a similar assertion in place for .bdrv_truncate. Instead of crashing when the user requests a too large image size, fail gracefully.
A file size of exactly INT64_MAX caused failure before, but is actually legal.
--- block/crypto.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
block/crypto.c +5 -1
@@ -357,7 +357,11 @@ static int block_crypto_truncate(BlockDriverState *bs, int64_t offset,BlockCrypto *crypto = bs->opaque;uint64_t payload_offset =qcrypto_block_get_payload_offset (crypto->block); -assert (payload_offset < (INT64_MAX - offset)); + +if (payload_offset >INT64_MAX - offset) { +error_setg (errp,"The requested file size is too large" ); +return -EFBIG ; + } offset += payload_offset;
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 05/12] luks: Turn another invalid assertion into check
Quoted 5 lines · Kevin Wolf
Commit
e39e959efixed an invalid assertion in the .bdrv_length implementation, but left a similar assertion in place for .bdrv_truncate. Instead of crashing when the user requests a too large image size, fail gracefully.
Quoted 7 lines
A file size of exactly
INT64_MAXcaused failure before, but is actually legal.Signed-off-by Kevin Wolf <[email protected]>--- block/crypto.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 05/12] luks: Turn another invalid assertion into check
In reply to Kevin WolfQuoted 5 lines · Kevin Wolf
Commit
e39e959efixed an invalid assertion in the .bdrv_length implementation, but left a similar assertion in place for .bdrv_truncate. Instead of crashing when the user requests a too large image size, fail gracefully.
Quoted 7 lines
A file size of exactly
INT64_MAXcaused failure before, but is actually legal.Signed-off-by Kevin Wolf <[email protected]>--- block/crypto.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 6/12 qemu-iotests: Test invalid resize on luks
In reply to Kevin WolfThis tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.
--- tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
tests/qemu-iotests/210 +37 -0
@@ -204,6 +204,43 @@ run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \ { "execute": "quit" } EOF +echo +echo "=== Resize image with invalid sizes ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG_FILE",node-name=node0 \ + -blockdev driver=luks,file=node0,key-secret=keysec0,node-name=node1 \ + -object secret,id=keysec0,data="foo" <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "block_resize", + "arguments": { + "node-name": "node1", + "size": 9223372036854775296 + } +} +{ "execute": "block_resize", + "arguments": { + "node-name": "node1", + "size": 9223372036854775808 + } +} +{ "execute": "block_resize", + "arguments": { + "node-name": "node1", + "size": 18446744073709551104 + } +} +{ "execute": "block_resize", + "arguments": { + "node-name": "node1", + "size": -9223372036854775808 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + # success, all done echo "*** done" rm -f $seq.full
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks
Quoted 7 lines · Kevin Wolf
This tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
Missing a change to 210.out?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks
Am 20.03.2018 um 19:33 hat Eric Blake geschrieben:
Quoted 10 lines · Kevin Wolf
Quoted 7 lines
This tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)Missing a change to 210.out?
Obviously. I wonder how that happened... Thanks!
Kevin
Re: [Qemu-devel] [PATCH for-2.12 06/12] qemu-iotests: Test invalid resize on luks
In reply to Kevin WolfQuoted 7 lines · Kevin Wolf
This tests that the .bdrv_truncate implementation for luks doesn't crash for invalid image sizes.
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/210 | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
PATCH 7/12 parallels: Check maximum cluster size on create
In reply to Kevin WolfIt's unclear what the real maximum cluster size is for the Parallels format, but let's at least make sure that we don't get integer overflows in our .bdrv_co_create implementation.
--- block/parallels.c | 5 +++++ 1 file changed, 5 insertions(+)
block/parallels.c +5 -0
@@ -526,6 +526,11 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, cl_size =DEFAULT_CLUSTER_SIZE ; } +/* XXX What is the real limit here? This is an insanely large maximum. */ +if (cl_size >=UINT64_MAX /MAX_PARALLELS_IMAGE_FACTOR ) { +error_setg (errp,"Cluster size is too large" ); +return -EINVAL ; + }if (total_size >=MAX_PARALLELS_IMAGE_FACTOR * cl_size) {error_setg (errp,"Image size is too large for this cluster size" );return -E2BIG ;
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 07/12] parallels: Check maximum cluster size on create
Quoted 14 lines · Kevin Wolf
It's unclear what the real maximum cluster size is for the Parallels format, but let's at least make sure that we don't get integer overflows in our .bdrv_co_create implementation.
Signed-off-by Kevin Wolf <[email protected]>--- block/parallels.c | 5 +++++ 1 file changed, 5 insertions(+)block/parallels.c +0
-0
block/parallels.c static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, · line 530
Quoted 5 lines
block/parallels.c +2
-0@@ -526,6 +526,11 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, cl_size =DEFAULT_CLUSTER_SIZE ; } +/* XXX What is the real limit here? This is an insanely large maximum. */ +if (cl_size >=UINT64_MAX /MAX_PARALLELS_IMAGE_FACTOR ) {
INT64_MAX is probably a saner starting point for the division...
block/parallels.c static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, · line 534
Quoted 4 lines
block/parallels.c +3
-0+error_setg (errp,"Cluster size is too large" ); +return -EINVAL ; + }if (total_size >=MAX_PARALLELS_IMAGE_FACTOR * cl_size) {
since total_size still has to fit within off_t (63 bits, not 64)
block/parallels.c static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts, · line 536
Quoted 3 lines
block/parallels.c +0
-0error_setg (errp,"Image size is too large for this cluster size" );return -E2BIG ;
With that change,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 8/12 qemu-iotests: Test parallels image creation with QMP
In reply to Kevin Wolf--- tests/qemu-iotests/212 | 326 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/212.out | 111 +++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 438 insertions(+) create mode 100755 tests/qemu-iotests/212 create mode 100644 tests/qemu-iotests/212.out
tests/qemu-iotests/212 new file +326 -0
@@ -0,0 +1,326 @@ +#!/bin/bash +# +# Test parallels and file image creation +# +# Copyright (C) 2018 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator [email protected] + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt parallels +_supported_proto file +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \ + | _filter_qemu | _filter_imgfmt \ + | _filter_actual_image_size +} + +echo +echo "=== Successful image creation (defaults) ===" +echo + +size=$((128 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "blockdev-add", + "arguments": { + "driver": "file", + "node-name": "imgfile", + "filename": "$TEST_IMG" + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "imgfile", + "size": $size + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Successful image creation (explicit defaults) ===" +echo + +# Choose a different size to show that we got a new image +size=$((64 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_IMG" + }, + "size": $size, + "cluster-size": 1048576 + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Successful image creation (with non-default options) ===" +echo + +# Choose a different size to show that we got a new image +size=$((32 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_IMG" + }, + "size": $size, + "cluster-size": 65536 + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Invalid BlockdevRef ===" +echo + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "this doesn't exist", + "size": $size + } +} +{ "execute": "quit" } +EOF + +echo +echo "=== Zero size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 0 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + +echo +echo "=== Maximum size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 4503599627369984 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + +echo +echo "=== Invalid sizes ===" +echo + +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. Misaligned image size +# 2. 2^64 - 512 +# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 4. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits) +# 5. 2^52 (512 bytes more than maximum image size) + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 1234 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 18446744073709551104 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 9223372036854775808 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 9223372036854775296 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 4503599627370497 + } +} +{ "execute": "quit" } +EOF + +echo +echo "=== Invalid cluster size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "cluster-size": 1234 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "cluster-size": 128 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "cluster-size": 4294967296 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "cluster-size": 9223372036854775808 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "cluster-size": 18446744073709551104 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "cluster-size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 281474976710656, + "cluster-size": 512 + } +} +{ "execute": "quit" } +EOF + + +# success, all done +echo "*** done" +rm -f $seq.full +status=0
tests/qemu-iotests/212.out new file +111 -0
111 lines not on this page · raw message
tests/qemu-iotests/group +1 -0
@@ -210,3 +210,4 @@ 209 rw auto quick 210 rw auto 211 rw auto quick +212 rw auto quick
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 08/12] qemu-iotests: Test parallels image creation with QMP
Quoted 13 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/212 | 326 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/212.out | 111 +++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 438 insertions(+)create mode 100755 tests/qemu-iotests/212
create mode 100644 tests/qemu-iotests/212.outtests/qemu-iotests/212 new file +3
-0+echo +echo "=== Invalid sizes ===" +echo
tests/qemu-iotests/212 line 214
Quoted 10 lines
tests/qemu-iotests/212 new file +10
-0+ +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. Misaligned image size +# 2. 2^64 - 512 +# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 4. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits)
Is this part of the comment stale copy-and-paste? There's no crypto header, and the real max is much smaller...
Quoted 7 lines
tests/qemu-iotests/212 new file +6
-0+# 5. 2^52 (512 bytes more than maximum image size) + +echo +echo "=== Invalid cluster size ===" +echo +
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 08/12] qemu-iotests: Test parallels image creation with QMP
Am 20.03.2018 um 19:42 hat Eric Blake geschrieben:
Quoted 11 lines · Kevin Wolf
Quoted 9 lines
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/212 | 326 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/212.out | 111 +++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 438 insertions(+)create mode 100755 tests/qemu-iotests/212
create mode 100644 tests/qemu-iotests/212.out
Quoted 16 lines
tests/qemu-iotests/212 line 214
Quoted 13 lines
tests/qemu-iotests/212 new file +13
-0+echo +echo "=== Invalid sizes ===" +echo + +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. Misaligned image size +# 2. 2^64 - 512 +# 3. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 4. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits)Is this part of the comment stale copy-and-paste? There's no crypto header, and the real max is much smaller...
Yeah... Not sure if the case is all that useful, but it can't hurt, so I'll just s/crypto header/image header/ rather than removing it.
Kevin
PATCH 9/12 vhdx: Require power-of-two block size on create
In reply to Kevin WolfImages with a non-power-of-two block size are invalid and cannot be opened. Reject such block sizes when creating an image.
--- block/vhdx.c | 4 ++++ 1 file changed, 4 insertions(+)
block/vhdx.c +4 -0
@@ -1877,6 +1877,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,error_setg_errno (errp,EINVAL ,"Block size must be a multiple of 1 MB" );return -EINVAL ; } +if (!is_power_of_2 (block_size)) { +error_setg (errp,"Block size must be a power of two" ); +return -EINVAL ; + }if (block_size >VHDX_BLOCK_SIZE_MAX ) {error_setg_errno (errp,EINVAL ,"Block size must not exceed %d" ,VHDX_BLOCK_SIZE_MAX );
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 09/12] vhdx: Require power-of-two block size on create
Quoted 8 lines · Kevin Wolf
Images with a non-power-of-two block size are invalid and cannot be opened. Reject such block sizes when creating an image.
Signed-off-by Kevin Wolf <[email protected]>--- block/vhdx.c | 4 ++++ 1 file changed, 4 insertions(+)
Quoted 16 lines
block/vhdx.c +4
-0@@ -1877,6 +1877,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,error_setg_errno (errp,EINVAL ,"Block size must be a multiple of 1 MB" );return -EINVAL ; } +if (!is_power_of_2 (block_size)) { +error_setg (errp,"Block size must be a power of two" ); +return -EINVAL ; + }if (block_size >VHDX_BLOCK_SIZE_MAX ) {error_setg_errno (errp,EINVAL ,"Block size must not exceed %d" ,VHDX_BLOCK_SIZE_MAX );
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 09/12] vhdx: Require power-of-two block size on create
In reply to Kevin WolfQuoted 3 lines · Kevin Wolf
Images with a non-power-of-two block size are invalid and cannot be opened. Reject such block sizes when creating an image.
Good catch.
Quoted 23 lines
Signed-off-by Kevin Wolf <[email protected]>--- block/vhdx.c | 4 ++++ 1 file changed, 4 insertions(+)block/vhdx.c +4
-0@@ -1877,6 +1877,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,error_setg_errno (errp,EINVAL ,"Block size must be a multiple of 1 MB" );return -EINVAL ; } +if (!is_power_of_2 (block_size)) { +error_setg (errp,"Block size must be a power of two" ); +return -EINVAL ; + }if (block_size >VHDX_BLOCK_SIZE_MAX ) {error_setg_errno (errp,EINVAL ,"Block size must not exceed %d" ,VHDX_BLOCK_SIZE_MAX );Signature · 1 line
2.13.6
PATCH 10/12 vhdx: Don't use error_setg_errno() with constant errno
In reply to Kevin Wolferror_setg_errno() is meant for cases where we got an errno from the OS that can add useful extra information to an error message. It's pointless if we pass a constant errno, these cases should use plain error_setg().
--- block/vhdx.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
block/vhdx.c +4 -5
@@ -1822,7 +1822,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,/* Validate options and set default values */ image_size = vhdx_opts->size;if (image_size >VHDX_MAX_IMAGE_SIZE ) { -error_setg_errno (errp,EINVAL ,"Image size too large; max of 64TB" ); +error_setg (errp,"Image size too large; max of 64TB" );return -EINVAL ; }
@@ -1832,7 +1832,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts, log_size = vhdx_opts->log_size; }if (log_size < MiB || (log_size % MiB) !=0 ) { -error_setg_errno (errp,EINVAL ,"Log size must be a multiple of 1 MB" ); +error_setg (errp,"Log size must be a multiple of 1 MB" );return -EINVAL ; }
@@ -1874,7 +1874,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts, }if (block_size < MiB || (block_size % MiB) !=0 ) { -error_setg_errno (errp,EINVAL ,"Block size must be a multiple of 1 MB" ); +error_setg (errp,"Block size must be a multiple of 1 MB" );return -EINVAL ; }if (!is_power_of_2 (block_size)) {
@@ -1882,8 +1882,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,return -EINVAL ; }if (block_size >VHDX_BLOCK_SIZE_MAX ) { -error_setg_errno (errp,EINVAL ,"Block size must not exceed %d" , -VHDX_BLOCK_SIZE_MAX ); +error_setg (errp,"Block size must not exceed %d" ,VHDX_BLOCK_SIZE_MAX );return -EINVAL ; }
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 10/12] vhdx: Don't use error_setg_errno() with constant errno
Quoted 4 lines · Kevin Wolf
error_setg_errno()is meant for cases where we got an errno from the OS that can add useful extra information to an error message. It's pointless if we pass a constant errno, these cases should use plainerror_setg().
Well, it DOES let you get the libc-specific spelling of the strerror for that errno (not all libc report the same string) - but you're right that it didn't add much here.
Quoted 5 lines
Signed-off-by Kevin Wolf <[email protected]>--- block/vhdx.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 10/12] vhdx: Don't use error_setg_errno() with constant errno
In reply to Kevin WolfQuoted 54 lines · Kevin Wolf
error_setg_errno()is meant for cases where we got an errno from the OS that can add useful extra information to an error message. It's pointless if we pass a constant errno, these cases should use plainerror_setg().Signed-off-by Kevin Wolf <[email protected]>--- block/vhdx.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)block/vhdx.c +4
-5@@ -1822,7 +1822,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,/* Validate options and set default values */ image_size = vhdx_opts->size;if (image_size >VHDX_MAX_IMAGE_SIZE ) { -error_setg_errno (errp,EINVAL ,"Image size too large; max of 64TB" ); +error_setg (errp,"Image size too large; max of 64TB" );return -EINVAL ; }@@ -1832,7 +1832,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts, log_size = vhdx_opts->log_size; }if (log_size < MiB || (log_size % MiB) !=0 ) { -error_setg_errno (errp,EINVAL ,"Log size must be a multiple of 1 MB" ); +error_setg (errp,"Log size must be a multiple of 1 MB" );return -EINVAL ; }@@ -1874,7 +1874,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts, }if (block_size < MiB || (block_size % MiB) !=0 ) { -error_setg_errno (errp,EINVAL ,"Block size must be a multiple of 1 MB" ); +error_setg (errp,"Block size must be a multiple of 1 MB" );return -EINVAL ; }if (!is_power_of_2 (block_size)) {@@ -1882,8 +1882,7 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,return -EINVAL ; }if (block_size >VHDX_BLOCK_SIZE_MAX ) { -error_setg_errno (errp,EINVAL ,"Block size must not exceed %d" , -VHDX_BLOCK_SIZE_MAX ); +error_setg (errp,"Block size must not exceed %d" ,VHDX_BLOCK_SIZE_MAX );return -EINVAL ; }Signature · 1 line
2.13.6
PATCH 11/12 vhdx: Check for 4 GB maximum log size on creation
In reply to Kevin WolfIt's unclear what the real maximum is, but we use an uint32_t to store the log size in vhdx_co_create(), so we should check that the given value fits in 32 bits.
--- block/vhdx.c | 4 ++++ 1 file changed, 4 insertions(+)
block/vhdx.c +4 -0
@@ -1829,6 +1829,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,if (!vhdx_opts->has_log_size) { log_size =DEFAULT_LOG_SIZE ; }else { +if (vhdx_opts->log_size >UINT32_MAX ) { +error_setg (errp,"Log size must be smaller than 4 GB" ); +return -EINVAL ; + } log_size = vhdx_opts->log_size; }if (log_size < MiB || (log_size % MiB) !=0 ) {
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 11/12] vhdx: Check for 4 GB maximum log size on creation
Quoted 8 lines · Kevin Wolf
It's unclear what the real maximum is, but we use an
uint32_tto store the log size invhdx_co_create(), so we should check that the given value fits in 32 bits.Signed-off-by Kevin Wolf <[email protected]>--- block/vhdx.c | 4 ++++ 1 file changed, 4 insertions(+)
Quoted 17 lines
block/vhdx.c +4
-0@@ -1829,6 +1829,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,if (!vhdx_opts->has_log_size) { log_size =DEFAULT_LOG_SIZE ; }else { +if (vhdx_opts->log_size >UINT32_MAX ) { +error_setg (errp,"Log size must be smaller than 4 GB" ); +return -EINVAL ; + } log_size = vhdx_opts->log_size; }if (log_size < MiB || (log_size % MiB) !=0 ) {
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 11/12] vhdx: Check for 4 GB maximum log size on creation
In reply to Kevin WolfQuoted 4 lines · Kevin Wolf
It's unclear what the real maximum is, but we use an
uint32_tto store the log size invhdx_co_create(), so we should check that the given value fits in 32 bits.
It's a uint32 in the on-disk header per spec, so I agree the implied max is UINT32_MAX
Quoted 23 lines
Signed-off-by Kevin Wolf <[email protected]>--- block/vhdx.c | 4 ++++ 1 file changed, 4 insertions(+)block/vhdx.c +4
-0@@ -1829,6 +1829,10 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,if (!vhdx_opts->has_log_size) { log_size =DEFAULT_LOG_SIZE ; }else { +if (vhdx_opts->log_size >UINT32_MAX ) { +error_setg (errp,"Log size must be smaller than 4 GB" ); +return -EINVAL ; + } log_size = vhdx_opts->log_size; }if (log_size < MiB || (log_size % MiB) !=0 ) {Signature · 1 line
2.13.6
PATCH 12/12 qemu-iotests: Test vhdx image creation with QMP
In reply to Kevin Wolf--- tests/qemu-iotests/213 | 349 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/213.out | 121 ++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 471 insertions(+) create mode 100755 tests/qemu-iotests/213 create mode 100644 tests/qemu-iotests/213.out
tests/qemu-iotests/213 new file +349 -0
@@ -0,0 +1,349 @@ +#!/bin/bash +# +# Test vhdx and file image creation +# +# Copyright (C) 2018 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator [email protected] + +seq=`basename $0` +echo "QA output created by $seq" + +here=`pwd` +status=1 # failure is the default! + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt vhdx +_supported_proto file +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp \ + | _filter_qemu | _filter_imgfmt \ + | _filter_actual_image_size +} + +echo +echo "=== Successful image creation (defaults) ===" +echo + +size=$((128 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "blockdev-add", + "arguments": { + "driver": "file", + "node-name": "imgfile", + "filename": "$TEST_IMG" + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "imgfile", + "size": $size + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Successful image creation (explicit defaults) ===" +echo + +# Choose a different size to show that we got a new image +size=$((64 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_IMG" + }, + "size": $size, + "log-size": 1048576, + "block-size": 8388608, + "subformat": "dynamic", + "block-state-zero": true + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Successful image creation (with non-default options) ===" +echo + +# Choose a different size to show that we got a new image +size=$((32 * 1024 * 1024)) + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "file", + "filename": "$TEST_IMG", + "size": 0 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": { + "driver": "file", + "filename": "$TEST_IMG" + }, + "size": $size, + "log-size": 8388608, + "block-size": 268435456, + "subformat": "fixed", + "block-state-zero": false + } +} +{ "execute": "quit" } +EOF + +_img_info --format-specific | _filter_img_info --format-specific + +echo +echo "=== Invalid BlockdevRef ===" +echo + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "this doesn't exist", + "size": $size + } +} +{ "execute": "quit" } +EOF + +echo +echo "=== Zero size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 0 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + +echo +echo "=== Maximum size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 70368744177664 + } +} +{ "execute": "quit" } +EOF + +_img_info | _filter_img_info + +echo +echo "=== Invalid sizes ===" +echo + +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. 2^64 - 512 +# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 3. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits) +# 4. 2^46 + 1 (512 bytes more than maximum image size) + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 18446744073709551104 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 9223372036854775808 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 9223372036854775296 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 70368744177665 + } +} +{ "execute": "quit" } +EOF + +echo +echo "=== Invalid block size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "block-size": 1234567 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "block-size": 128 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "block-size": 3145728 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "block-size": 536870912 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "block-size": 0 + } +} +{ "execute": "quit" } +EOF + +echo +echo "=== Invalid log size ===" +echo + +run_qemu -blockdev driver=file,filename="$TEST_IMG",node-name=node0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "log-size": 1234567 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "log-size": 128 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "log-size": 4294967296 + } +} +{ "execute": "x-blockdev-create", + "arguments": { + "driver": "$IMGFMT", + "file": "node0", + "size": 67108864, + "log-size": 0 + } +} +{ "execute": "quit" } +EOF + + +# success, all done +echo "*** done" +rm -f $seq.full +status=0
tests/qemu-iotests/213.out new file +121 -0
121 lines not on this page · raw message
tests/qemu-iotests/group +1 -0
@@ -211,3 +211,4 @@ 210 rw auto 211 rw auto quick 212 rw auto quick +213 rw auto quick
Signature · 1 line
2.13.6
Re: [Qemu-devel] [PATCH for-2.12 12/12] qemu-iotests: Test vhdx image creation with QMP
Quoted 13 lines · Kevin Wolf
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/213 | 349 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/213.out | 121 ++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 471 insertions(+)create mode 100755 tests/qemu-iotests/213
create mode 100644 tests/qemu-iotests/213.outtests/qemu-iotests/213 new file +3
-0+ +echo +echo "=== Invalid sizes ==="
tests/qemu-iotests/213 line 219
Quoted 10 lines
tests/qemu-iotests/213 new file +10
-0+echo + +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. 2^64 - 512 +# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 3. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits)
Same comments as before on whether this comment is slightly stale after copy-and-paste.
Quoted 1 line
+# 4. 2^46 + 1 (512 bytes more than maximum image size)
Does this image format require 512-byte alignment? If so, are you missing a test of unaligned sizes? If not, why not just 1 byte more than the maximum?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-2.12 12/12] qemu-iotests: Test vhdx image creation with QMP
Am 20.03.2018 um 19:53 hat Eric Blake geschrieben:
Quoted 11 lines · Kevin Wolf
Quoted 9 lines
Signed-off-by Kevin Wolf <[email protected]>--- tests/qemu-iotests/213 | 349 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/213.out | 121 ++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 471 insertions(+)create mode 100755 tests/qemu-iotests/213
create mode 100644 tests/qemu-iotests/213.out
Quoted 16 lines
tests/qemu-iotests/213 line 219
Quoted 13 lines
tests/qemu-iotests/213 new file +13
-0+ +echo +echo "=== Invalid sizes ===" +echo + +# TODO Negative image sizes aren't handled correctly, but this is a problem +# with QAPI's implementation of the 'size' type and affects other commands as +# well. Once this is fixed, we may want to add a test case here. + +# 1. 2^64 - 512 +# 2. 2^63 = 8 EB (qemu-img enforces image sizes less than this) +# 3. 2^63 - 512 (generally valid, but with the crypto header the file will +# exceed 63 bits)Same comments as before on whether this comment is slightly stale after copy-and-paste.
Will do the same thing as there ("image header").
Quoted 4 lines
Quoted 1 line
+# 4. 2^46 + 1 (512 bytes more than maximum image size)
Does this image format require 512-byte alignment? If so, are you missing a test of unaligned sizes? If not, why not just 1 byte more than the maximum?
The comment is wrong, the code already does just 1 byte more.
Kevin