From 3d9330ece5c5cb7b5e8fec1bef4da8fe78134fc2 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 4 Mar 2021 13:35:03 +0300 Subject: [PATCH 01/17] MAINTAINERS: add Vladimir as co-maintainer of NBD Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210304103503.21008-1-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 26c9454823..c0c36648ab 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3033,6 +3033,7 @@ F: block/iscsi-opts.c Network Block Device (NBD) M: Eric Blake +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Maintained F: block/nbd* @@ -3043,6 +3044,7 @@ F: blockdev-nbd.c F: docs/interop/nbd.txt F: docs/interop/qemu-nbd.rst T: git https://repo.or.cz/qemu/ericb.git nbd +T: git https://src.openvz.org/scm/~vsementsov/qemu.git nbd NFS M: Peter Lieven From 0da9856851dcca09222a1467e16ddd05dc66e460 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Fri, 19 Feb 2021 18:07:52 +0200 Subject: [PATCH 02/17] nbd: server: Report holes for raw images When querying image extents for raw image, qemu-nbd reports holes as zero: $ qemu-nbd -t -r -f raw empty-6g.raw $ qemu-img map --output json nbd://localhost [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": true, "offset": 0}] $ qemu-img map --output json empty-6g.raw [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, "offset": 0}] Turns out that qemu-img map reports a hole based on BDRV_BLOCK_DATA, but nbd server reports a hole based on BDRV_BLOCK_ALLOCATED. The NBD protocol says: NBD_STATE_HOLE (bit 0): if set, the block represents a hole (and future writes to that area may cause fragmentation or encounter an NBD_ENOSPC error); if clear, the block is allocated or the server could not otherwise determine its status. qemu-img manual says: whether the sectors contain actual data or not (boolean field data; if false, the sectors are either unallocated or stored as optimized all-zero clusters); To me, data=false looks compatible with NBD_STATE_HOLE. From user point of view, getting same results from qemu-nbd and qemu-img is more important than being more correct about allocation status. Changing nbd server to report holes using BDRV_BLOCK_DATA makes qemu-nbd results compatible with qemu-img map: $ qemu-img map --output json nbd://localhost [{ "start": 0, "length": 6442450944, "depth": 0, "zero": true, "data": false, "offset": 0}] Signed-off-by: Nir Soffer Message-Id: <20210219160752.1826830-1-nsoffer@redhat.com> Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake --- nbd/server.c | 4 ++-- tests/qemu-iotests/241.out | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 7229f487d2..86a44a9b41 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2087,8 +2087,8 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset, return ret; } - flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) | - (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0); + flags = (ret & BDRV_BLOCK_DATA ? 0 : NBD_STATE_HOLE) | + (ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0); if (nbd_extent_array_add(ea, num, flags) < 0) { return 0; diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out index 75f9f465e5..3f8c173cc8 100644 --- a/tests/qemu-iotests/241.out +++ b/tests/qemu-iotests/241.out @@ -5,7 +5,7 @@ QA output created by 241 size: 1024 min block: 1 [{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, "offset": OFFSET}] +{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": false, "offset": OFFSET}] 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0) === Exporting unaligned raw image, forced server sector alignment === @@ -23,6 +23,6 @@ WARNING: Image format was not specified for 'TEST_DIR/t.raw' and probing guessed size: 1024 min block: 1 [{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, "offset": OFFSET}, -{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, "offset": OFFSET}] +{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": false, "offset": OFFSET}] 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0) *** done From 1657ba44b449471c665bc5d358ca33af411710f3 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 11 Feb 2021 14:44:35 -0600 Subject: [PATCH 03/17] utils: Enhance testsuite for do_strtosz() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enhance our testsuite coverage of do_strtosz() to cover some things we know that existing users want to continue working (hex bytes), as well as some things that accidentally work but shouldn't (hex fractions) or accidentally fail but that users want to work (64-bit precision on byte values). This includes fixing a typo in the comment regarding our parsing near 2^64. Signed-off-by: Eric Blake Message-Id: <20210211204438.1184395-2-eblake@redhat.com> Reviewed-by: Daniel P. Berrangé --- tests/test-cutils.c | 154 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 143 insertions(+), 11 deletions(-) diff --git a/tests/test-cutils.c b/tests/test-cutils.c index 1aa8351520..6d9802e00b 100644 --- a/tests/test-cutils.c +++ b/tests/test-cutils.c @@ -1960,11 +1960,19 @@ static void test_qemu_strtosz_simple(void) g_assert_cmpint(res, ==, 0); g_assert(endptr == str + 1); - str = "12345"; + /* Leading 0 gives decimal results, not octal */ + str = "08"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 8); + g_assert(endptr == str + 2); + + /* Leading space is ignored */ + str = " 12345"; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); g_assert_cmpint(res, ==, 12345); - g_assert(endptr == str + 5); + g_assert(endptr == str + 6); err = qemu_strtosz(str, NULL, &res); g_assert_cmpint(err, ==, 0); @@ -1984,7 +1992,7 @@ static void test_qemu_strtosz_simple(void) g_assert_cmpint(res, ==, 0x20000000000000); g_assert(endptr == str + 16); - str = "9007199254740993"; /* 2^53+1 */ + str = "9007199254740993"; /* 2^53+1 FIXME loss of precision is a bug */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); g_assert_cmpint(res, ==, 0x20000000000000); /* rounded to 53 bits */ @@ -1996,14 +2004,42 @@ static void test_qemu_strtosz_simple(void) g_assert_cmpint(res, ==, 0xfffffffffffff800); g_assert(endptr == str + 20); - str = "18446744073709550591"; /* 0xfffffffffffffbff */ + str = "18446744073709550591"; /* 0xfffffffffffffbff FIXME */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); g_assert_cmpint(res, ==, 0xfffffffffffff800); /* rounded to 53 bits */ g_assert(endptr == str + 20); - /* 0x7ffffffffffffe00..0x7fffffffffffffff get rounded to - * 0x8000000000000000, thus -ERANGE; see test_qemu_strtosz_erange() */ + /* + * FIXME 0xfffffffffffffe00..0xffffffffffffffff get rounded to + * 2^64, thus -ERANGE; see test_qemu_strtosz_erange() + */ +} + +static void test_qemu_strtosz_hex(void) +{ + const char *str; + const char *endptr; + int err; + uint64_t res = 0xbaadf00d; + + str = "0x0"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 0); + g_assert(endptr == str + 3); + + str = "0xab"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 171); + g_assert(endptr == str + 4); + + str = "0xae"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 174); + g_assert(endptr == str + 4); } static void test_qemu_strtosz_units(void) @@ -2064,14 +2100,36 @@ static void test_qemu_strtosz_units(void) static void test_qemu_strtosz_float(void) { - const char *str = "12.345M"; + const char *str; int err; const char *endptr; uint64_t res = 0xbaadf00d; + str = "0.5E"; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, 12.345 * MiB); + g_assert_cmpint(res, ==, EiB / 2); + g_assert(endptr == str + 4); + + /* For convenience, a fraction of 0 is tolerated even on bytes */ + str = "1.0B"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 1); + g_assert(endptr == str + 4); + + /* An empty fraction is tolerated */ + str = "1.k"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 1024); + g_assert(endptr == str + 3); + + /* For convenience, we permit values that are not byte-exact */ + str = "12.345M"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB)); g_assert(endptr == str + 7); } @@ -2106,6 +2164,47 @@ static void test_qemu_strtosz_invalid(void) err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); g_assert(endptr == str); + + /* Fractional values require scale larger than bytes */ + /* FIXME endptr shouldn't move on -EINVAL */ + str = "1.1B"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert(endptr == str + 4); + + /* FIXME endptr shouldn't move on -EINVAL */ + str = "1.1"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert(endptr == str + 3); + + /* FIXME we should reject floating point exponents */ + str = "1.5e1k"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 15360); + g_assert(endptr == str + 6); + + /* FIXME we should reject floating point exponents */ + str = "1.5E+0k"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 1536); + g_assert(endptr == str + 7); + + /* FIXME we should reject hex fractions */ + str = "0x1.8k"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 1536); + g_assert(endptr == str + 6); + + /* FIXME we reject all other attempts at negative, why not -0 */ + str = "-0"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 0); + g_assert(endptr == str + 2); } static void test_qemu_strtosz_trailing(void) @@ -2131,6 +2230,30 @@ static void test_qemu_strtosz_trailing(void) err = qemu_strtosz(str, NULL, &res); g_assert_cmpint(err, ==, -EINVAL); + + str = "0x"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(res, ==, 0); + g_assert(endptr == str + 1); + + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); + + str = "0.NaN"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert(endptr == str + 2); + + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); + + str = "123-45"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(res, ==, 123); + g_assert(endptr == str + 3); + + err = qemu_strtosz(str, NULL, &res); + g_assert_cmpint(err, ==, -EINVAL); } static void test_qemu_strtosz_erange(void) @@ -2145,12 +2268,12 @@ static void test_qemu_strtosz_erange(void) g_assert_cmpint(err, ==, -ERANGE); g_assert(endptr == str + 2); - str = "18446744073709550592"; /* 0xfffffffffffffc00 */ + str = "18446744073709550592"; /* 0xfffffffffffffc00 FIXME accept this */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -ERANGE); g_assert(endptr == str + 20); - str = "18446744073709551615"; /* 2^64-1 */ + str = "18446744073709551615"; /* 2^64-1 FIXME accept this */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -ERANGE); g_assert(endptr == str + 20); @@ -2168,15 +2291,22 @@ static void test_qemu_strtosz_erange(void) static void test_qemu_strtosz_metric(void) { - const char *str = "12345k"; + const char *str; int err; const char *endptr; uint64_t res = 0xbaadf00d; + str = "12345k"; err = qemu_strtosz_metric(str, &endptr, &res); g_assert_cmpint(err, ==, 0); g_assert_cmpint(res, ==, 12345000); g_assert(endptr == str + 6); + + str = "12.345M"; + err = qemu_strtosz_metric(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 12345000); + g_assert(endptr == str + 7); } int main(int argc, char **argv) @@ -2443,6 +2573,8 @@ int main(int argc, char **argv) g_test_add_func("/cutils/strtosz/simple", test_qemu_strtosz_simple); + g_test_add_func("/cutils/strtosz/hex", + test_qemu_strtosz_hex); g_test_add_func("/cutils/strtosz/units", test_qemu_strtosz_units); g_test_add_func("/cutils/strtosz/float", From cf923b783efd565787e9ab006fb5608bb2a7297b Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 11 Feb 2021 14:44:36 -0600 Subject: [PATCH 04/17] utils: Improve qemu_strtosz() to have 64 bits of precision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have multiple clients of qemu_strtosz (qemu-io, the opts visitor, the keyval visitor), and it gets annoying that edge-case testing is impacted by implicit rounding to 53 bits of precision due to parsing with strtod(). As an example posted by Rich Jones: $ nbdkit memory $(( 2**63 - 2**30 )) --run \ 'build/qemu-io -f raw "$uri" -c "w -P 3 $(( 2**63 - 2**30 - 512 )) 512" ' write failed: Input/output error because 9223372035781033472 got rounded to 0x7fffffffc0000000 which is out of bounds. It is also worth noting that our existing parser, by virtue of using strtod(), accepts decimal AND hex numbers, even though test-cutils previously lacked any coverage of the latter until the previous patch. We do have existing clients that expect a hex parse to work (for example, iotest 33 using qemu-io -c "write -P 0xa 0x200 0x400"), but strtod() parses "08" as 8 rather than as an invalid octal number, so we know there are no clients that depend on octal. Our use of strtod() also means that "0x1.8k" would actually parse as 1536 (the fraction is 8/16), rather than 1843 (if the fraction were 8/10); but as this was not covered in the testsuite, I have no qualms forbidding hex fractions as invalid, so this patch declares that the use of fractions is only supported with decimal input, and enhances the testsuite to document that. Our previous use of strtod() meant that -1 parsed as a negative; now that we parse with strtoull(), negative values can wrap around modulo 2^64, so we have to explicitly check whether the user passed in a '-'; and make it consistent to also reject '-0'. This has the minor effect of treating negative values as EINVAL (with no change to endptr) rather than ERANGE (with endptr advanced to what was parsed), visible in the updated iotest output. We also had no testsuite coverage of "1.1e0k", which happened to parse under strtod() but is unlikely to occur in practice; as long as we are making things more robust, it is easy enough to reject the use of exponents in a strtod parse. The fix is done by breaking the parse into an integer prefix (no loss in precision), rejecting negative values (since we can no longer rely on strtod() to do that), determining if a decimal or hexadecimal parse was intended (with the new restriction that a fractional hex parse is not allowed), and where appropriate, using a floating point fractional parse (where we also scan to reject use of exponents in the fraction). The bulk of the patch is then updates to the testsuite to match our new precision, as well as adding new cases we reject (whether they were rejected or inadvertently accepted before). Signed-off-by: Eric Blake Message-Id: <20210211204438.1184395-3-eblake@redhat.com> Reviewed-by: Daniel P. Berrangé --- tests/qemu-iotests/049.out | 14 ++-- tests/qemu-iotests/178.out.qcow2 | 3 +- tests/qemu-iotests/178.out.raw | 3 +- tests/test-cutils.c | 74 ++++++++------------- tests/test-keyval.c | 35 +++++++--- tests/test-qemu-opts.c | 33 ++++++---- util/cutils.c | 106 ++++++++++++++++++++++--------- 7 files changed, 166 insertions(+), 102 deletions(-) diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out index b1d8fd9107..01f7b1f240 100644 --- a/tests/qemu-iotests/049.out +++ b/tests/qemu-iotests/049.out @@ -92,16 +92,22 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off comp == 3. Invalid sizes == qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024 -qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. +qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for +qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2 -qemu-img: TEST_DIR/t.qcow2: Value '-1024' is out of range for parameter 'size' +qemu-img: TEST_DIR/t.qcow2: Parameter 'size' expects a non-negative number below 2^64 +Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta- +and exabytes, respectively. qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k -qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. +qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for +qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. qemu-img create -f qcow2 -o size=-1k TEST_DIR/t.qcow2 -qemu-img: TEST_DIR/t.qcow2: Value '-1k' is out of range for parameter 'size' +qemu-img: TEST_DIR/t.qcow2: Parameter 'size' expects a non-negative number below 2^64 +Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta- +and exabytes, respectively. qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for diff --git a/tests/qemu-iotests/178.out.qcow2 b/tests/qemu-iotests/178.out.qcow2 index fe193fd5f4..0d51fe401e 100644 --- a/tests/qemu-iotests/178.out.qcow2 +++ b/tests/qemu-iotests/178.out.qcow2 @@ -13,7 +13,8 @@ qemu-img: Invalid option list: , qemu-img: Invalid parameter 'snapshot.foo' qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar' qemu-img: --output must be used with human or json as argument. -qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. +qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for +qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. qemu-img: Unknown file format 'foo' == Size calculation for a new file (human) == diff --git a/tests/qemu-iotests/178.out.raw b/tests/qemu-iotests/178.out.raw index 445e460fad..116241ddef 100644 --- a/tests/qemu-iotests/178.out.raw +++ b/tests/qemu-iotests/178.out.raw @@ -13,7 +13,8 @@ qemu-img: Invalid option list: , qemu-img: Invalid parameter 'snapshot.foo' qemu-img: Failed in parsing snapshot param 'snapshot.foo=bar' qemu-img: --output must be used with human or json as argument. -qemu-img: Invalid image size specified. Must be between 0 and 9223372036854775807. +qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for +qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes. qemu-img: Unknown file format 'foo' == Size calculation for a new file (human) == diff --git a/tests/test-cutils.c b/tests/test-cutils.c index 6d9802e00b..bad3a60993 100644 --- a/tests/test-cutils.c +++ b/tests/test-cutils.c @@ -1978,8 +1978,6 @@ static void test_qemu_strtosz_simple(void) g_assert_cmpint(err, ==, 0); g_assert_cmpint(res, ==, 12345); - /* Note: precision is 53 bits since we're parsing with strtod() */ - str = "9007199254740991"; /* 2^53-1 */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); @@ -1992,10 +1990,10 @@ static void test_qemu_strtosz_simple(void) g_assert_cmpint(res, ==, 0x20000000000000); g_assert(endptr == str + 16); - str = "9007199254740993"; /* 2^53+1 FIXME loss of precision is a bug */ + str = "9007199254740993"; /* 2^53+1 */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, 0x20000000000000); /* rounded to 53 bits */ + g_assert_cmpint(res, ==, 0x20000000000001); g_assert(endptr == str + 16); str = "18446744073709549568"; /* 0xfffffffffffff800 (53 msbs set) */ @@ -2004,16 +2002,17 @@ static void test_qemu_strtosz_simple(void) g_assert_cmpint(res, ==, 0xfffffffffffff800); g_assert(endptr == str + 20); - str = "18446744073709550591"; /* 0xfffffffffffffbff FIXME */ + str = "18446744073709550591"; /* 0xfffffffffffffbff */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, 0xfffffffffffff800); /* rounded to 53 bits */ + g_assert_cmpint(res, ==, 0xfffffffffffffbff); g_assert(endptr == str + 20); - /* - * FIXME 0xfffffffffffffe00..0xffffffffffffffff get rounded to - * 2^64, thus -ERANGE; see test_qemu_strtosz_erange() - */ + str = "18446744073709551615"; /* 0xffffffffffffffff */ + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, 0); + g_assert_cmpint(res, ==, 0xffffffffffffffff); + g_assert(endptr == str + 20); } static void test_qemu_strtosz_hex(void) @@ -2166,45 +2165,43 @@ static void test_qemu_strtosz_invalid(void) g_assert(endptr == str); /* Fractional values require scale larger than bytes */ - /* FIXME endptr shouldn't move on -EINVAL */ str = "1.1B"; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); - g_assert(endptr == str + 4); + g_assert(endptr == str); - /* FIXME endptr shouldn't move on -EINVAL */ str = "1.1"; err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL); - g_assert(endptr == str + 3); + g_assert(endptr == str); - /* FIXME we should reject floating point exponents */ + /* No floating point exponents */ str = "1.5e1k"; err = qemu_strtosz(str, &endptr, &res); - g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, 15360); - g_assert(endptr == str + 6); + g_assert_cmpint(err, ==, -EINVAL); + g_assert(endptr == str); - /* FIXME we should reject floating point exponents */ str = "1.5E+0k"; err = qemu_strtosz(str, &endptr, &res); - g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, 1536); - g_assert(endptr == str + 7); + g_assert_cmpint(err, ==, -EINVAL); + g_assert(endptr == str); - /* FIXME we should reject hex fractions */ + /* No hex fractions */ str = "0x1.8k"; err = qemu_strtosz(str, &endptr, &res); - g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, 1536); - g_assert(endptr == str + 6); + g_assert_cmpint(err, ==, -EINVAL); + g_assert(endptr == str); - /* FIXME we reject all other attempts at negative, why not -0 */ + /* No negative values */ str = "-0"; err = qemu_strtosz(str, &endptr, &res); - g_assert_cmpint(err, ==, 0); - g_assert_cmpint(res, ==, 0); - g_assert(endptr == str + 2); + g_assert_cmpint(err, ==, -EINVAL); + g_assert(endptr == str); + + str = "-1"; + err = qemu_strtosz(str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); + g_assert(endptr == str); } static void test_qemu_strtosz_trailing(void) @@ -2263,22 +2260,7 @@ static void test_qemu_strtosz_erange(void) int err; uint64_t res = 0xbaadf00d; - str = "-1"; - err = qemu_strtosz(str, &endptr, &res); - g_assert_cmpint(err, ==, -ERANGE); - g_assert(endptr == str + 2); - - str = "18446744073709550592"; /* 0xfffffffffffffc00 FIXME accept this */ - err = qemu_strtosz(str, &endptr, &res); - g_assert_cmpint(err, ==, -ERANGE); - g_assert(endptr == str + 20); - - str = "18446744073709551615"; /* 2^64-1 FIXME accept this */ - err = qemu_strtosz(str, &endptr, &res); - g_assert_cmpint(err, ==, -ERANGE); - g_assert(endptr == str + 20); - - str = "18446744073709551616"; /* 2^64 */ + str = "18446744073709551616"; /* 2^64; see strtosz_simple for 2^64-1 */ err = qemu_strtosz(str, &endptr, &res); g_assert_cmpint(err, ==, -ERANGE); g_assert(endptr == str + 20); diff --git a/tests/test-keyval.c b/tests/test-keyval.c index ee927fe4e4..e20c07cf3e 100644 --- a/tests/test-keyval.c +++ b/tests/test-keyval.c @@ -445,9 +445,9 @@ static void test_keyval_visit_size(void) visit_end_struct(v, NULL); visit_free(v); - /* Note: precision is 53 bits since we're parsing with strtod() */ + /* Note: full 64 bits of precision */ - /* Around limit of precision: 2^53-1, 2^53, 2^53+1 */ + /* Around double limit of precision: 2^53-1, 2^53, 2^53+1 */ qdict = keyval_parse("sz1=9007199254740991," "sz2=9007199254740992," "sz3=9007199254740993", @@ -460,22 +460,25 @@ static void test_keyval_visit_size(void) visit_type_size(v, "sz2", &sz, &error_abort); g_assert_cmphex(sz, ==, 0x20000000000000); visit_type_size(v, "sz3", &sz, &error_abort); - g_assert_cmphex(sz, ==, 0x20000000000000); + g_assert_cmphex(sz, ==, 0x20000000000001); visit_check_struct(v, &error_abort); visit_end_struct(v, NULL); visit_free(v); - /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */ - qdict = keyval_parse("sz1=9223372036854774784," /* 7ffffffffffffc00 */ - "sz2=9223372036854775295", /* 7ffffffffffffdff */ + /* Close to signed integer limit 2^63 */ + qdict = keyval_parse("sz1=9223372036854775807," /* 7fffffffffffffff */ + "sz2=9223372036854775808," /* 8000000000000000 */ + "sz3=9223372036854775809", /* 8000000000000001 */ NULL, NULL, &error_abort); v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); qobject_unref(qdict); visit_start_struct(v, NULL, NULL, 0, &error_abort); visit_type_size(v, "sz1", &sz, &error_abort); - g_assert_cmphex(sz, ==, 0x7ffffffffffffc00); + g_assert_cmphex(sz, ==, 0x7fffffffffffffff); visit_type_size(v, "sz2", &sz, &error_abort); - g_assert_cmphex(sz, ==, 0x7ffffffffffffc00); + g_assert_cmphex(sz, ==, 0x8000000000000000); + visit_type_size(v, "sz3", &sz, &error_abort); + g_assert_cmphex(sz, ==, 0x8000000000000001); visit_check_struct(v, &error_abort); visit_end_struct(v, NULL); visit_free(v); @@ -490,14 +493,26 @@ static void test_keyval_visit_size(void) visit_type_size(v, "sz1", &sz, &error_abort); g_assert_cmphex(sz, ==, 0xfffffffffffff800); visit_type_size(v, "sz2", &sz, &error_abort); - g_assert_cmphex(sz, ==, 0xfffffffffffff800); + g_assert_cmphex(sz, ==, 0xfffffffffffffbff); + visit_check_struct(v, &error_abort); + visit_end_struct(v, NULL); + visit_free(v); + + /* Actual limit 2^64-1*/ + qdict = keyval_parse("sz1=18446744073709551615", /* ffffffffffffffff */ + NULL, NULL, &error_abort); + v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); + qobject_unref(qdict); + visit_start_struct(v, NULL, NULL, 0, &error_abort); + visit_type_size(v, "sz1", &sz, &error_abort); + g_assert_cmphex(sz, ==, 0xffffffffffffffff); visit_check_struct(v, &error_abort); visit_end_struct(v, NULL); visit_free(v); /* Beyond limits */ qdict = keyval_parse("sz1=-1," - "sz2=18446744073709550592", /* fffffffffffffc00 */ + "sz2=18446744073709551616", /* 2^64 */ NULL, NULL, &error_abort); v = qobject_input_visitor_new_keyval(QOBJECT(qdict)); qobject_unref(qdict); diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c index 8bbb17b1c7..6568e31a72 100644 --- a/tests/test-qemu-opts.c +++ b/tests/test-qemu-opts.c @@ -654,9 +654,9 @@ static void test_opts_parse_size(void) g_assert_cmpuint(opts_count(opts), ==, 1); g_assert_cmpuint(qemu_opt_get_size(opts, "size1", 1), ==, 0); - /* Note: precision is 53 bits since we're parsing with strtod() */ + /* Note: full 64 bits of precision */ - /* Around limit of precision: 2^53-1, 2^53, 2^54 */ + /* Around double limit of precision: 2^53-1, 2^53, 2^53+1 */ opts = qemu_opts_parse(&opts_list_02, "size1=9007199254740991," "size2=9007199254740992," @@ -668,18 +668,21 @@ static void test_opts_parse_size(void) g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1), ==, 0x20000000000000); g_assert_cmphex(qemu_opt_get_size(opts, "size3", 1), - ==, 0x20000000000000); + ==, 0x20000000000001); - /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */ + /* Close to signed int limit: 2^63-1, 2^63, 2^63+1 */ opts = qemu_opts_parse(&opts_list_02, - "size1=9223372036854774784," /* 7ffffffffffffc00 */ - "size2=9223372036854775295", /* 7ffffffffffffdff */ + "size1=9223372036854775807," /* 7fffffffffffffff */ + "size2=9223372036854775808," /* 8000000000000000 */ + "size3=9223372036854775809", /* 8000000000000001 */ false, &error_abort); - g_assert_cmpuint(opts_count(opts), ==, 2); + g_assert_cmpuint(opts_count(opts), ==, 3); g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1), - ==, 0x7ffffffffffffc00); + ==, 0x7fffffffffffffff); g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1), - ==, 0x7ffffffffffffc00); + ==, 0x8000000000000000); + g_assert_cmphex(qemu_opt_get_size(opts, "size3", 1), + ==, 0x8000000000000001); /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */ opts = qemu_opts_parse(&opts_list_02, @@ -690,14 +693,22 @@ static void test_opts_parse_size(void) g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1), ==, 0xfffffffffffff800); g_assert_cmphex(qemu_opt_get_size(opts, "size2", 1), - ==, 0xfffffffffffff800); + ==, 0xfffffffffffffbff); + + /* Actual limit, 2^64-1 */ + opts = qemu_opts_parse(&opts_list_02, + "size1=18446744073709551615", /* ffffffffffffffff */ + false, &error_abort); + g_assert_cmpuint(opts_count(opts), ==, 1); + g_assert_cmphex(qemu_opt_get_size(opts, "size1", 1), + ==, 0xffffffffffffffff); /* Beyond limits */ opts = qemu_opts_parse(&opts_list_02, "size1=-1", false, &err); error_free_or_abort(&err); g_assert(!opts); opts = qemu_opts_parse(&opts_list_02, - "size1=18446744073709550592", /* fffffffffffffc00 */ + "size1=18446744073709551616", /* 2^64 */ false, &err); error_free_or_abort(&err); g_assert(!opts); diff --git a/util/cutils.c b/util/cutils.c index 70c7d6efbd..189a184859 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -241,52 +241,100 @@ static int64_t suffix_mul(char suffix, int64_t unit) } /* - * Convert string to bytes, allowing either B/b for bytes, K/k for KB, - * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned - * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on - * other error. + * Convert size string to bytes. + * + * The size parsing supports the following syntaxes + * - 12345 - decimal, scale determined by @default_suffix and @unit + * - 12345{bBkKmMgGtTpPeE} - decimal, scale determined by suffix and @unit + * - 12345.678{kKmMgGtTpPeE} - decimal, scale determined by suffix, and + * fractional portion is truncated to byte + * - 0x7fEE - hexadecimal, unit determined by @default_suffix + * + * The following are intentionally not supported + * - octal, such as 08 + * - fractional hex, such as 0x1.8 + * - floating point exponents, such as 1e3 + * + * The end pointer will be returned in *end, if not NULL. If there is + * no fraction, the input can be decimal or hexadecimal; if there is a + * fraction, then the input must be decimal and there must be a suffix + * (possibly by @default_suffix) larger than Byte, and the fractional + * portion may suffer from precision loss or rounding. The input must + * be positive. + * + * Return -ERANGE on overflow (with *@end advanced), and -EINVAL on + * other error (with *@end left unchanged). */ static int do_strtosz(const char *nptr, const char **end, const char default_suffix, int64_t unit, uint64_t *result) { int retval; - const char *endptr; + const char *endptr, *f; unsigned char c; - int mul_required = 0; - double val, mul, integral, fraction; + bool mul_required = false; + uint64_t val; + int64_t mul; + double fraction = 0.0; - retval = qemu_strtod_finite(nptr, &endptr, &val); + /* Parse integral portion as decimal. */ + retval = qemu_strtou64(nptr, &endptr, 10, &val); if (retval) { goto out; } - fraction = modf(val, &integral); - if (fraction != 0) { - mul_required = 1; - } - c = *endptr; - mul = suffix_mul(c, unit); - if (mul >= 0) { - endptr++; - } else { - mul = suffix_mul(default_suffix, unit); - assert(mul >= 0); - } - if (mul == 1 && mul_required) { + if (memchr(nptr, '-', endptr - nptr) != NULL) { + endptr = nptr; retval = -EINVAL; goto out; } - /* - * Values near UINT64_MAX overflow to 2**64 when converting to double - * precision. Compare against the maximum representable double precision - * value below 2**64, computed as "the next value after 2**64 (0x1p64) in - * the direction of 0". - */ - if ((val * mul > nextafter(0x1p64, 0)) || val < 0) { + if (val == 0 && (*endptr == 'x' || *endptr == 'X')) { + /* Input looks like hex, reparse, and insist on no fraction. */ + retval = qemu_strtou64(nptr, &endptr, 16, &val); + if (retval) { + goto out; + } + if (*endptr == '.') { + endptr = nptr; + retval = -EINVAL; + goto out; + } + } else if (*endptr == '.') { + /* + * Input looks like a fraction. Make sure even 1.k works + * without fractional digits. If we see an exponent, treat + * the entire input as invalid instead. + */ + f = endptr; + retval = qemu_strtod_finite(f, &endptr, &fraction); + if (retval) { + fraction = 0.0; + endptr++; + } else if (memchr(f, 'e', endptr - f) || memchr(f, 'E', endptr - f)) { + endptr = nptr; + retval = -EINVAL; + goto out; + } else if (fraction != 0) { + mul_required = true; + } + } + c = *endptr; + mul = suffix_mul(c, unit); + if (mul > 0) { + endptr++; + } else { + mul = suffix_mul(default_suffix, unit); + assert(mul > 0); + } + if (mul == 1 && mul_required) { + endptr = nptr; + retval = -EINVAL; + goto out; + } + if (val > (UINT64_MAX - ((uint64_t) (fraction * mul))) / mul) { retval = -ERANGE; goto out; } - *result = val * mul; + *result = val * mul + (uint64_t) (fraction * mul); retval = 0; out: From f174cd3350c5e97db000e7383be974c66046b8f5 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 11 Feb 2021 14:44:37 -0600 Subject: [PATCH 05/17] utils: Deprecate hex-with-suffix sizes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supporting '0x20M' looks odd, particularly since we have a 'B' suffix that is ambiguous for bytes, as well as a less-frequently-used 'E' suffix for extremely large exibytes. In practice, people using hex inputs are specifying values in bytes (and would have written 0x2000000, or possibly relied on default_suffix in the case of qemu_strtosz_MiB), and the use of scaling suffixes makes the most sense for inputs in decimal (where the user would write 32M). But rather than outright dropping support for hex-with-suffix, let's follow our deprecation policy. Sadly, since qemu_strtosz() does not have an Err** parameter, and plumbing that in would be a much larger task, we instead go with just directly emitting the deprecation warning to stderr. Signed-off-by: Eric Blake Message-Id: <20210211204438.1184395-4-eblake@redhat.com> Reviewed-by: Daniel P. Berrangé --- docs/system/deprecated.rst | 8 ++++++++ util/cutils.c | 10 +++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index cfabe69846..ecff6bf8c6 100644 --- a/docs/system/deprecated.rst +++ b/docs/system/deprecated.rst @@ -166,6 +166,14 @@ Using ``-M kernel-irqchip=off`` with x86 machine types that include a local APIC is deprecated. The ``split`` setting is supported, as is using ``-M kernel-irqchip=off`` with the ISA PC machine type. +hexadecimal sizes with scaling multipliers (since 6.0) +'''''''''''''''''''''''''''''''''''''''''''''''''''''' + +Input parameters that take a size value should only use a size suffix +(such as 'k' or 'M') when the base is written in decimal, and not when +the value is hexadecimal. That is, '0x20M' is deprecated, and should +be written either as '32M' or as '0x2000000'. + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/util/cutils.c b/util/cutils.c index 189a184859..d89a40a8c3 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -250,6 +250,9 @@ static int64_t suffix_mul(char suffix, int64_t unit) * fractional portion is truncated to byte * - 0x7fEE - hexadecimal, unit determined by @default_suffix * + * The following cause a deprecation warning, and may be removed in the future + * - 0xabc{kKmMgGtTpP} - hex with scaling suffix + * * The following are intentionally not supported * - octal, such as 08 * - fractional hex, such as 0x1.8 @@ -272,7 +275,7 @@ static int do_strtosz(const char *nptr, const char **end, int retval; const char *endptr, *f; unsigned char c; - bool mul_required = false; + bool mul_required = false, hex = false; uint64_t val; int64_t mul; double fraction = 0.0; @@ -298,6 +301,7 @@ static int do_strtosz(const char *nptr, const char **end, retval = -EINVAL; goto out; } + hex = true; } else if (*endptr == '.') { /* * Input looks like a fraction. Make sure even 1.k works @@ -320,6 +324,10 @@ static int do_strtosz(const char *nptr, const char **end, c = *endptr; mul = suffix_mul(c, unit); if (mul > 0) { + if (hex) { + warn_report("Using a multiplier suffix on hex numbers " + "is deprecated: %s", nptr); + } endptr++; } else { mul = suffix_mul(default_suffix, unit); From bc520249595845d387aa5b5e4eeeade673931a98 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:45 +0300 Subject: [PATCH 06/17] block: check return value of bdrv_open_child and drop error propagation This patch is generated by cocci script: @@ symbol bdrv_open_child, errp, local_err; expression file; @@ file = bdrv_open_child(..., - &local_err + errp ); - if (local_err) + if (!file) { ... - error_propagate(errp, local_err); ... } with command spatch --sp-file x.cocci --macro-file scripts/cocci-macro-file.h \ --in-place --no-show-diff --max-width 80 --use-gitgrep block Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-4-vsementsov@virtuozzo.com> [eblake: fix qcow2_do_open() to use ERRP_GUARD, necessary as the only caller to pass allow_none=true] Signed-off-by: Eric Blake --- block/blkdebug.c | 6 ++---- block/blklogwrites.c | 10 ++++------ block/blkreplay.c | 6 ++---- block/blkverify.c | 11 ++++------- block/qcow2.c | 6 +++--- block/quorum.c | 6 ++---- 6 files changed, 17 insertions(+), 28 deletions(-) diff --git a/block/blkdebug.c b/block/blkdebug.c index 7eaa8a28bf..2c0b9b0ee8 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -464,7 +464,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, { BDRVBlkdebugState *s = bs->opaque; QemuOpts *opts; - Error *local_err = NULL; int ret; uint64_t align; @@ -494,10 +493,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - false, &local_err); - if (local_err) { + false, errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto out; } diff --git a/block/blklogwrites.c b/block/blklogwrites.c index 13ae63983b..b7579370a3 100644 --- a/block/blklogwrites.c +++ b/block/blklogwrites.c @@ -157,19 +157,17 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags, /* Open the file */ bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false, - &local_err); - if (local_err) { + errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } /* Open the log file */ s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_of_bds, - BDRV_CHILD_METADATA, false, &local_err); - if (local_err) { + BDRV_CHILD_METADATA, false, errp); + if (!s->log_file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } diff --git a/block/blkreplay.c b/block/blkreplay.c index 30a0f5d57a..4a247752fd 100644 --- a/block/blkreplay.c +++ b/block/blkreplay.c @@ -23,16 +23,14 @@ typedef struct Request { static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { - Error *local_err = NULL; int ret; /* Open the image file */ bs->file = bdrv_open_child(NULL, options, "image", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - false, &local_err); - if (local_err) { + false, errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } diff --git a/block/blkverify.c b/block/blkverify.c index 943e62be9c..188d7632fa 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -112,7 +112,6 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, { BDRVBlkverifyState *s = bs->opaque; QemuOpts *opts; - Error *local_err = NULL; int ret; opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); @@ -125,20 +124,18 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - false, &local_err); - if (local_err) { + false, errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } /* Open the test file */ s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "test", bs, &child_of_bds, BDRV_CHILD_DATA, - false, &local_err); - if (local_err) { + false, errp); + if (!s->test_file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } diff --git a/block/qcow2.c b/block/qcow2.c index d9f49a52e7..c5266424bb 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp) static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { + ERRP_GUARD(); BDRVQcow2State *s = bs->opaque; unsigned int len, i; int ret = 0; @@ -1611,9 +1612,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, /* Open external data file */ s->data_file = bdrv_open_child(NULL, options, "data-file", bs, &child_of_bds, BDRV_CHILD_DATA, - true, &local_err); - if (local_err) { - error_propagate(errp, local_err); + true, errp); + if (*errp) { ret = -EINVAL; goto fail; } diff --git a/block/quorum.c b/block/quorum.c index 0bd75450de..cfc1436abb 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -929,7 +929,6 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { BDRVQuorumState *s = bs->opaque; - Error *local_err = NULL; QemuOpts *opts = NULL; const char *pattern_str; bool *opened; @@ -1007,9 +1006,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags, s->children[i] = bdrv_open_child(NULL, options, indexstr, bs, &child_of_bds, BDRV_CHILD_DATA, false, - &local_err); - if (local_err) { - error_propagate(errp, local_err); + errp); + if (!s->children[i]) { ret = -EINVAL; goto close_exit; } From 5a11a1ca0d0ed5be52070f1da8de89ef85941183 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:46 +0300 Subject: [PATCH 07/17] blockdev: fix drive_backup_prepare() missed error We leak local_err and don't report failure to the caller. It's definitely wrong, let's fix. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-5-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- blockdev.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/blockdev.c b/blockdev.c index cd438e60e3..65884a2826 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1825,9 +1825,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) aio_context_acquire(aio_context); if (set_backing_hd) { - bdrv_set_backing_hd(target_bs, source, &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (bdrv_set_backing_hd(target_bs, source, errp) < 0) { goto unref; } } From dc9c10a1f42235a7f8411feca28984c4e7da3177 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:47 +0300 Subject: [PATCH 08/17] block: drop extra error propagation for bdrv_set_backing_hd bdrv_set_backing_hd now returns status, let's use it. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-6-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/block.c b/block.c index a1f3cecd75..933ff49b10 100644 --- a/block.c +++ b/block.c @@ -2995,11 +2995,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, /* Hook up the backing file link; drop our reference, bs owns the * backing_hd reference now */ - bdrv_set_backing_hd(bs, backing_hd, &local_err); + ret = bdrv_set_backing_hd(bs, backing_hd, errp); bdrv_unref(backing_hd); - if (local_err) { - error_propagate(errp, local_err); - ret = -EINVAL; + if (ret < 0) { goto free_exit; } From eb5becc18fff6ba43922a169a64029e7e26ef86a Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:48 +0300 Subject: [PATCH 09/17] block/mirror: drop extra error propagation in commit_active_start() Let's check return value of mirror_start_job to check for failure instead of local_err. Rename ret to job, as ret is usually integer variable. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-7-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/mirror.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index 1803c6988b..6af02a57c4 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1860,8 +1860,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, bool auto_complete, Error **errp) { bool base_read_only; - Error *local_err = NULL; - BlockJob *ret; + BlockJob *job; base_read_only = bdrv_is_read_only(base); @@ -1871,19 +1870,18 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs, } } - ret = mirror_start_job( + job = mirror_start_job( job_id, bs, creation_flags, base, NULL, speed, 0, 0, MIRROR_LEAVE_BACKING_CHAIN, false, on_error, on_error, true, cb, opaque, &commit_active_job_driver, false, base, auto_complete, filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND, - &local_err); - if (local_err) { - error_propagate(errp, local_err); + errp); + if (!job) { goto error_restore_flags; } - return ret; + return job; error_restore_flags: /* ignore error and errp for bdrv_reopen, because we want to propagate From 775d0c050866c3571b8599291b3ff65fdbd63ed8 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:49 +0300 Subject: [PATCH 10/17] blockjob: return status from block_job_set_speed() Better to return status together with setting errp. It allows to avoid error propagation in the caller. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-8-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- blockjob.c | 18 ++++++++---------- include/block/blockjob.h | 2 +- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/blockjob.c b/blockjob.c index f2feff051d..9e0ffd8dc9 100644 --- a/blockjob.c +++ b/blockjob.c @@ -258,18 +258,18 @@ static bool job_timer_pending(Job *job) return timer_pending(&job->sleep_timer); } -void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) { const BlockJobDriver *drv = block_job_driver(job); int64_t old_speed = job->speed; - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp)) { - return; + if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { + return false; } if (speed < 0) { error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed", "a non-negative value"); - return; + return false; } ratelimit_set_speed(&job->limit, speed, BLOCK_JOB_SLICE_TIME); @@ -281,11 +281,13 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) } if (speed && speed <= old_speed) { - return; + return true; } /* kick only if a timer is pending */ job_enter_cond(&job->job, job_timer_pending); + + return true; } int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) @@ -458,12 +460,8 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, /* Only set speed when necessary to avoid NotSupported error */ if (speed != 0) { - Error *local_err = NULL; - - block_job_set_speed(job, speed, &local_err); - if (local_err) { + if (!block_job_set_speed(job, speed, errp)) { job_early_fail(&job->job); - error_propagate(errp, local_err); return NULL; } } diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 35faa3aa26..d200f33c10 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -139,7 +139,7 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); * Set a rate-limiting parameter for the job; the actual meaning may * vary depending on the job type. */ -void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); /** * block_job_query: From 83bad8cbf5c1276499cee13710db56c0101faa69 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:50 +0300 Subject: [PATCH 11/17] block/qcow2: qcow2_get_specific_info(): drop error propagation Don't use error propagation in qcow2_get_specific_info(). For this refactor qcow2_get_bitmap_info_list, its current interface is rather weird. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210202124956.63146-9-vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia [eblake: separate local 'tail' variable from 'info_list' parameter] Signed-off-by: Eric Blake --- block/qcow2-bitmap.c | 26 ++++++++++++++------------ block/qcow2.c | 10 +++------- block/qcow2.h | 4 ++-- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 5eef82fa55..3c9dcaf0a2 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1089,30 +1089,32 @@ static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags) /* * qcow2_get_bitmap_info_list() * Returns a list of QCOW2 bitmap details. - * In case of no bitmaps, the function returns NULL and - * the @errp parameter is not set. - * When bitmap information can not be obtained, the function returns - * NULL and the @errp parameter is set. + * On success return true with info_list set (note, that if there are no + * bitmaps, info_list is set to NULL). + * On failure return false with errp set. */ -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, - Error **errp) +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, + Qcow2BitmapInfoList **info_list, Error **errp) { BDRVQcow2State *s = bs->opaque; Qcow2BitmapList *bm_list; Qcow2Bitmap *bm; - Qcow2BitmapInfoList *list = NULL; - Qcow2BitmapInfoList **tail = &list; + Qcow2BitmapInfoList **tail; if (s->nb_bitmaps == 0) { - return NULL; + *info_list = NULL; + return true; } bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, errp); - if (bm_list == NULL) { - return NULL; + if (!bm_list) { + return false; } + *info_list = NULL; + tail = info_list; + QSIMPLEQ_FOREACH(bm, bm_list, entry) { Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1); info->granularity = 1U << bm->granularity_bits; @@ -1123,7 +1125,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, bitmap_list_free(bm_list); - return list; + return true; } int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp) diff --git a/block/qcow2.c b/block/qcow2.c index c5266424bb..289e3dbc97 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -5066,12 +5066,10 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs, BDRVQcow2State *s = bs->opaque; ImageInfoSpecific *spec_info; QCryptoBlockInfo *encrypt_info = NULL; - Error *local_err = NULL; if (s->crypto != NULL) { - encrypt_info = qcrypto_block_get_info(s->crypto, &local_err); - if (local_err) { - error_propagate(errp, local_err); + encrypt_info = qcrypto_block_get_info(s->crypto, errp); + if (!encrypt_info) { return NULL; } } @@ -5088,9 +5086,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs, }; } else if (s->qcow_version == 3) { Qcow2BitmapInfoList *bitmaps; - bitmaps = qcow2_get_bitmap_info_list(bs, &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!qcow2_get_bitmap_info_list(bs, &bitmaps, errp)) { qapi_free_ImageInfoSpecific(spec_info); qapi_free_QCryptoBlockInfo(encrypt_info); return NULL; diff --git a/block/qcow2.h b/block/qcow2.h index 0678073b74..a6bf2881bb 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -979,8 +979,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, void **refcount_table, int64_t *refcount_table_size); bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); -Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs, - Error **errp); +bool qcow2_get_bitmap_info_list(BlockDriverState *bs, + Qcow2BitmapInfoList **info_list, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, From 0c1e9d2a9a3b6dd5ce74092019882fbce691d081 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:51 +0300 Subject: [PATCH 12/17] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface It's recommended for bool functions with errp to return true on success and false on failure. Non-standard interfaces don't help to understand the code. The change is also needed to reduce error propagation. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Reviewed-by: Greg Kurz Message-Id: <20210202124956.63146-10-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/qcow2-bitmap.c | 26 +++++++++++++++----------- block/qcow2.c | 6 ++---- block/qcow2.h | 3 ++- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 3c9dcaf0a2..9452e9fe76 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -962,25 +962,27 @@ static void set_readonly_helper(gpointer bitmap, gpointer value) bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value); } -/* qcow2_load_dirty_bitmaps() - * Return value is a hint for caller: true means that the Qcow2 header was - * updated. (false doesn't mean that the header should be updated by the - * caller, it just means that updating was not needed or the image cannot be - * written to). - * On failure the function returns false. +/* + * Return true on success, false on failure. + * If header_updated is not NULL then it is set appropriately regardless of + * the return value. */ -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated, + Error **errp) { BDRVQcow2State *s = bs->opaque; Qcow2BitmapList *bm_list; Qcow2Bitmap *bm; GSList *created_dirty_bitmaps = NULL; - bool header_updated = false; bool needs_update = false; + if (header_updated) { + *header_updated = false; + } + if (s->nb_bitmaps == 0) { /* No bitmaps - nothing to do */ - return false; + return true; } bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, @@ -1036,7 +1038,9 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) error_setg_errno(errp, -ret, "Can't update bitmap directory"); goto fail; } - header_updated = true; + if (header_updated) { + *header_updated = true; + } } if (!can_write(bs)) { @@ -1047,7 +1051,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) g_slist_free(created_dirty_bitmaps); bitmap_list_free(bm_list); - return header_updated; + return true; fail: g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs); diff --git a/block/qcow2.c b/block/qcow2.c index 289e3dbc97..31042b87a1 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1297,7 +1297,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, unsigned int len, i; int ret = 0; QCowHeader header; - Error *local_err = NULL; uint64_t ext_end; uint64_t l1_vm_state_index; bool update_header = false; @@ -1785,9 +1784,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) { /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */ - bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err); - if (local_err != NULL) { - error_propagate(errp, local_err); + bool header_updated; + if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) { ret = -EINVAL; goto fail; } diff --git a/block/qcow2.h b/block/qcow2.h index a6bf2881bb..d19c883206 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -978,7 +978,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table); int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res, void **refcount_table, int64_t *refcount_table_size); -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp); +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated, + Error **errp); bool qcow2_get_bitmap_info_list(BlockDriverState *bs, Qcow2BitmapInfoList **info_list, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); From 526e31de993431569ad3b8bdf461ef26d03c404d Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:52 +0300 Subject: [PATCH 13/17] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps It's better to return status together with setting errp. It makes possible to avoid error propagation. While being here, put ERRP_GUARD() to fix error_prepend(errp, ...) usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment above ERRP_GUARD() definition in include/qapi/error.h) Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-11-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/qcow2-bitmap.c | 13 ++++++------- block/qcow2.h | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index 9452e9fe76..f417f9ccb1 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1531,9 +1531,10 @@ out: * readonly to begin with, and whether we opened directly or reopened to that * state shouldn't matter for the state we get afterward. */ -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored, Error **errp) { + ERRP_GUARD(); BdrvDirtyBitmap *bitmap; BDRVQcow2State *s = bs->opaque; uint32_t new_nb_bitmaps = s->nb_bitmaps; @@ -1553,7 +1554,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bm_list = bitmap_list_load(bs, s->bitmap_directory_offset, s->bitmap_directory_size, errp); if (bm_list == NULL) { - return; + return false; } } @@ -1668,7 +1669,7 @@ success: } bitmap_list_free(bm_list); - return; + return true; fail: QSIMPLEQ_FOREACH(bm, bm_list, entry) { @@ -1686,16 +1687,14 @@ fail: } bitmap_list_free(bm_list); + return false; } int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp) { BdrvDirtyBitmap *bitmap; - Error *local_err = NULL; - qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err); - if (local_err != NULL) { - error_propagate(errp, local_err); + if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) { return -EINVAL; } diff --git a/block/qcow2.h b/block/qcow2.h index d19c883206..0fe5f74ed3 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -984,7 +984,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs, Qcow2BitmapInfoList **info_list, Error **errp); int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp); int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp); -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored, Error **errp); int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs, From 772c4cad13b97b8e8ef8592228707f7a5557f939 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:53 +0300 Subject: [PATCH 14/17] block/qcow2: read_cache_sizes: return status value It's better to return status together with setting errp. It allows to reduce error propagation. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Greg Kurz Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-12-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/qcow2.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 31042b87a1..4aa0890166 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -868,7 +868,7 @@ static void qcow2_attach_aio_context(BlockDriverState *bs, cache_clean_timer_init(bs, new_context); } -static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, +static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, uint64_t *l2_cache_size, uint64_t *l2_cache_entry_size, uint64_t *refcount_cache_size, Error **errp) @@ -906,16 +906,16 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE " and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set " "at the same time"); - return; + return false; } else if (l2_cache_size_set && (l2_cache_max_setting > combined_cache_size)) { error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed " QCOW2_OPT_CACHE_SIZE); - return; + return false; } else if (*refcount_cache_size > combined_cache_size) { error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed " QCOW2_OPT_CACHE_SIZE); - return; + return false; } if (l2_cache_size_set) { @@ -954,8 +954,10 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts, error_setg(errp, "L2 cache entry size must be a power of two " "between %d and the cluster size (%d)", 1 << MIN_CLUSTER_BITS, s->cluster_size); - return; + return false; } + + return true; } typedef struct Qcow2ReopenState { @@ -982,7 +984,6 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, int i; const char *encryptfmt; QDict *encryptopts = NULL; - Error *local_err = NULL; int ret; qdict_extract_subqdict(options, &encryptopts, "encrypt."); @@ -995,10 +996,8 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, } /* get L2 table/refcount block cache size from command line options */ - read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size, - &refcount_cache_size, &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size, + &refcount_cache_size, errp)) { ret = -EINVAL; goto fail; } From e6247c9c9f9e4d01b00036f017da53d130981727 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:54 +0300 Subject: [PATCH 15/17] block/qcow2: simplify qcow2_co_invalidate_cache() qcow2_do_open correctly sets errp on each failure path. So, we can simplify code in qcow2_co_invalidate_cache() and drop explicit error propagation. Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h so that error_prepend() is actually called even if errp is &error_fatal. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Reviewed-by: Greg Kurz Message-Id: <20210202124956.63146-13-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/qcow2.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 4aa0890166..a1dee95dea 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2716,11 +2716,11 @@ static void qcow2_close(BlockDriverState *bs) static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, Error **errp) { + ERRP_GUARD(); BDRVQcow2State *s = bs->opaque; int flags = s->flags; QCryptoBlock *crypto = NULL; QDict *options; - Error *local_err = NULL; int ret; /* @@ -2738,16 +2738,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, flags &= ~BDRV_O_INACTIVE; qemu_co_mutex_lock(&s->lock); - ret = qcow2_do_open(bs, options, flags, &local_err); + ret = qcow2_do_open(bs, options, flags, errp); qemu_co_mutex_unlock(&s->lock); qobject_unref(options); - if (local_err) { - error_propagate_prepend(errp, local_err, - "Could not reopen qcow2 layer: "); - bs->drv = NULL; - return; - } else if (ret < 0) { - error_setg_errno(errp, -ret, "Could not reopen qcow2 layer"); + if (ret < 0) { + error_prepend(errp, "Could not reopen qcow2 layer: "); bs->drv = NULL; return; } From 15ce94a68ca6730466c565c3d29971aab3087bf1 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:55 +0300 Subject: [PATCH 16/17] block/qed: bdrv_qed_do_open: deal with errp Always set errp on failure. The generic bdrv_open_driver supports driver functions which can return a negative value but forget to set errp. That's a strange thing. Let's improve bdrv_qed_do_open to not behave this way. This allows the simplification of code in bdrv_qed_co_invalidate_cache(). Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Reviewed-by: Greg Kurz Message-Id: <20210202124956.63146-14-vsementsov@virtuozzo.com> [eblake: commit message grammar tweak] Signed-off-by: Eric Blake --- block/qed.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/block/qed.c b/block/qed.c index b27e7546ca..f45c640513 100644 --- a/block/qed.c +++ b/block/qed.c @@ -393,6 +393,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header)); if (ret < 0) { + error_setg(errp, "Failed to read QED header"); return ret; } qed_header_le_to_cpu(&le_header, &s->header); @@ -408,25 +409,30 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, return -ENOTSUP; } if (!qed_is_cluster_size_valid(s->header.cluster_size)) { + error_setg(errp, "QED cluster size is invalid"); return -EINVAL; } /* Round down file size to the last cluster */ file_size = bdrv_getlength(bs->file->bs); if (file_size < 0) { + error_setg(errp, "Failed to get file length"); return file_size; } s->file_size = qed_start_of_cluster(s, file_size); if (!qed_is_table_size_valid(s->header.table_size)) { + error_setg(errp, "QED table size is invalid"); return -EINVAL; } if (!qed_is_image_size_valid(s->header.image_size, s->header.cluster_size, s->header.table_size)) { + error_setg(errp, "QED image size is invalid"); return -EINVAL; } if (!qed_check_table_offset(s, s->header.l1_table_offset)) { + error_setg(errp, "QED table offset is invalid"); return -EINVAL; } @@ -438,6 +444,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, /* Header size calculation must not overflow uint32_t */ if (s->header.header_size > UINT32_MAX / s->header.cluster_size) { + error_setg(errp, "QED header size is too large"); return -EINVAL; } @@ -445,6 +452,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, if ((uint64_t)s->header.backing_filename_offset + s->header.backing_filename_size > s->header.cluster_size * s->header.header_size) { + error_setg(errp, "QED backing filename offset is invalid"); return -EINVAL; } @@ -453,6 +461,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, bs->auto_backing_file, sizeof(bs->auto_backing_file)); if (ret < 0) { + error_setg(errp, "Failed to read backing filename"); return ret; } pstrcpy(bs->backing_file, sizeof(bs->backing_file), @@ -475,6 +484,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, ret = qed_write_header_sync(s); if (ret) { + error_setg(errp, "Failed to update header"); return ret; } @@ -487,6 +497,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, ret = qed_read_l1_table_sync(s); if (ret) { + error_setg(errp, "Failed to read L1 table"); goto out; } @@ -503,6 +514,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options, ret = qed_check(s, &result, true); if (ret) { + error_setg(errp, "Image corrupted"); goto out; } } @@ -1537,22 +1549,16 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs, Error **errp) { BDRVQEDState *s = bs->opaque; - Error *local_err = NULL; int ret; bdrv_qed_close(bs); bdrv_qed_init_state(bs); qemu_co_mutex_lock(&s->table_lock); - ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err); + ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp); qemu_co_mutex_unlock(&s->table_lock); - if (local_err) { - error_propagate_prepend(errp, local_err, - "Could not reopen qed layer: "); - return; - } else if (ret < 0) { - error_setg_errno(errp, -ret, "Could not reopen qed layer"); - return; + if (ret < 0) { + error_prepend(errp, "Could not reopen qed layer: "); } } From 1184b411016bce7590723170aa6b5984518707ec Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:56 +0300 Subject: [PATCH 17/17] block/qcow2: refactor qcow2_update_options_prepare error paths Keep setting ret close to setting errp and don't merge different error paths into one. This way it's more obvious that we don't return error without setting errp. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-15-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block/qcow2.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index a1dee95dea..0db1227ac9 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1158,6 +1158,10 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, } qdict_put_str(encryptopts, "format", "qcow"); r->crypto_opts = block_crypto_open_opts_init(encryptopts, errp); + if (!r->crypto_opts) { + ret = -EINVAL; + goto fail; + } break; case QCOW_CRYPT_LUKS: @@ -1170,14 +1174,15 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, } qdict_put_str(encryptopts, "format", "luks"); r->crypto_opts = block_crypto_open_opts_init(encryptopts, errp); + if (!r->crypto_opts) { + ret = -EINVAL; + goto fail; + } break; default: error_setg(errp, "Unsupported encryption method %d", s->crypt_method_header); - break; - } - if (s->crypt_method_header != QCOW_CRYPT_NONE && !r->crypto_opts) { ret = -EINVAL; goto fail; }