| Message ID | d213fb4eb326353eccde0ea1c5a05bba2da706e0.1541595424.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Don't pass flags to bdrv_reopen_queue() | expand |
On 07.11.18 13:59, Alberto Garcia wrote: > This function takes three options (cache.direct, cache.no-flush and > read-only) from a QemuOpts object and updates the flags accordingly. and auto-read-only now > > If any of those options is not set (because it was missing from the > original QDict or because it had an invalid value) then the function > aborts with a failed assertion: > > $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2 > block.c:1126: update_flags_from_options: Assertion `qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)' failed. > Aborted > > This assertion is unnecessary, and it forces any caller of > bdrv_reopen() to pass all the aforementioned three options. This may *four > have made sense in order to remove ambiguity when bdrv_reopen() was > taking both flags and options, but that's not the case anymore. > > It's also unnecessary if we want to validate the option values, > because bdrv_reopen_prepare() already takes care of that, as we can > see if we remove the assertions: > > $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2 > Parameter 'read-only' expects 'on' or 'off' > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 4 ---- > tests/qemu-iotests/133 | 8 ++++++++ > tests/qemu-iotests/133.out | 6 ++++++ > 3 files changed, 14 insertions(+), 4 deletions(-) Hm, seems like one way to solve it and I can't really find issue with it. So, let's first give a Reviewed-by: Max Reitz <mreitz@redhat.com> However, I wonder why you dropped your patch from v1 for this. It seemed more reasonable to me. You're basically trading half-updating the flags for just not touching them at all (and the latter seems better, even though it's all an error in the end anyway). > diff --git a/block.c b/block.c > index 8bc808d6f3..68f1e3b45e 100644 > --- a/block.c > +++ b/block.c > @@ -1139,24 +1139,20 @@ static void update_flags_from_options(int *flags, QemuOpts *opts) > { > *flags &= ~BDRV_O_CACHE_MASK; > > - assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH)); > if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) { > *flags |= BDRV_O_NO_FLUSH; > } > > - assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)); > if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_DIRECT, false)) { > *flags |= BDRV_O_NOCACHE; > } > > *flags &= ~BDRV_O_RDWR; Unrelated to this patch, but isn't BDRV_O_AUTO_RDONLY missing here? Max > > - assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY)); > if (!qemu_opt_get_bool_del(opts, BDRV_OPT_READ_ONLY, false)) { > *flags |= BDRV_O_RDWR; > } > > - assert(qemu_opt_find(opts, BDRV_OPT_AUTO_READ_ONLY)); > if (qemu_opt_get_bool_del(opts, BDRV_OPT_AUTO_READ_ONLY, false)) { > *flags |= BDRV_O_AUTO_RDONLY; > } > diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133 > index 14e6b3b972..59d5e2ea25 100755 > --- a/tests/qemu-iotests/133 > +++ b/tests/qemu-iotests/133 > @@ -101,6 +101,14 @@ $QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG > $QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG > $QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG > $QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG > + > +echo > +echo "=== Check that invalid options are handled correctly ===" > +echo > + > +$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG > +$QEMU_IO -c 'reopen -o cache.no-flush=bar' $TEST_IMG > +$QEMU_IO -c 'reopen -o cache.direct=baz' $TEST_IMG > # success, all done > echo "*** done" > rm -f $seq.full > diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out > index 48a9d087f0..551096a9c4 100644 > --- a/tests/qemu-iotests/133.out > +++ b/tests/qemu-iotests/133.out > @@ -32,4 +32,10 @@ Cannot set both -r/-w and 'read-only' > Cannot set both -c and the cache options > Cannot set both -c and the cache options > Cannot set both -c and the cache options > + > +=== Check that invalid options are handled correctly === > + > +Parameter 'read-only' expects 'on' or 'off' > +Parameter 'cache.no-flush' expects 'on' or 'off' > +Parameter 'cache.direct' expects 'on' or 'off' > *** done >
On Sun 11 Nov 2018 10:01:05 PM CET, Max Reitz wrote: > On 07.11.18 13:59, Alberto Garcia wrote: >> This function takes three options (cache.direct, cache.no-flush and >> read-only) from a QemuOpts object and updates the flags accordingly. > > and auto-read-only now Oops, will update. > Hm, seems like one way to solve it and I can't really find issue with > it. So, let's first give a > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > However, I wonder why you dropped your patch from v1 for this. It > seemed more reasonable to me. You're basically trading half-updating > the flags for just not touching them at all (and the latter seems > better, even though it's all an error in the end anyway). The main reason why I'm doing this is because if we keep the assertions then we're forced to have these four options always set, and I don't see any reason why they would need to be. It's not a problem now but it will be later on. Have a look at this early implementation of qmp_x_blockdev_reopen(): https://lists.gnu.org/archive/html/qemu-block/2018-06/msg00795.html Here we need to explicitly set those options to false if they're unset. 'false' is already the default value of all of them, so this shouldn't be necessary, but if we don't do it we'd hit the assertions that I'm removing in this patch. Berto
On Sun 11 Nov 2018 10:01:05 PM CET, Max Reitz wrote: >> - assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)); >> if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_DIRECT, false)) { >> *flags |= BDRV_O_NOCACHE; >> } >> >> *flags &= ~BDRV_O_RDWR; > > Unrelated to this patch, but isn't BDRV_O_AUTO_RDONLY missing here? I forgot to mention, but I think you're right here. I'll include this fix in the next version of the series. Berto
On 12.11.18 11:26, Alberto Garcia wrote: > On Sun 11 Nov 2018 10:01:05 PM CET, Max Reitz wrote: >> On 07.11.18 13:59, Alberto Garcia wrote: >>> This function takes three options (cache.direct, cache.no-flush and >>> read-only) from a QemuOpts object and updates the flags accordingly. >> >> and auto-read-only now > > Oops, will update. > >> Hm, seems like one way to solve it and I can't really find issue with >> it. So, let's first give a >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> However, I wonder why you dropped your patch from v1 for this. It >> seemed more reasonable to me. You're basically trading half-updating >> the flags for just not touching them at all (and the latter seems >> better, even though it's all an error in the end anyway). > > The main reason why I'm doing this is because if we keep the assertions > then we're forced to have these four options always set, and I don't see > any reason why they would need to be. > > It's not a problem now but it will be later on. Have a look at this > early implementation of qmp_x_blockdev_reopen(): > > https://lists.gnu.org/archive/html/qemu-block/2018-06/msg00795.html > > Here we need to explicitly set those options to false if they're > unset. 'false' is already the default value of all of them, so this > shouldn't be necessary, but if we don't do it we'd hit the assertions > that I'm removing in this patch. OK, that makes sense. Max
diff --git a/block.c b/block.c index 8bc808d6f3..68f1e3b45e 100644 --- a/block.c +++ b/block.c @@ -1139,24 +1139,20 @@ static void update_flags_from_options(int *flags, QemuOpts *opts) { *flags &= ~BDRV_O_CACHE_MASK; - assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH)); if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) { *flags |= BDRV_O_NO_FLUSH; } - assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)); if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_DIRECT, false)) { *flags |= BDRV_O_NOCACHE; } *flags &= ~BDRV_O_RDWR; - assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY)); if (!qemu_opt_get_bool_del(opts, BDRV_OPT_READ_ONLY, false)) { *flags |= BDRV_O_RDWR; } - assert(qemu_opt_find(opts, BDRV_OPT_AUTO_READ_ONLY)); if (qemu_opt_get_bool_del(opts, BDRV_OPT_AUTO_READ_ONLY, false)) { *flags |= BDRV_O_AUTO_RDONLY; } diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133 index 14e6b3b972..59d5e2ea25 100755 --- a/tests/qemu-iotests/133 +++ b/tests/qemu-iotests/133 @@ -101,6 +101,14 @@ $QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG $QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG $QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG $QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG + +echo +echo "=== Check that invalid options are handled correctly ===" +echo + +$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG +$QEMU_IO -c 'reopen -o cache.no-flush=bar' $TEST_IMG +$QEMU_IO -c 'reopen -o cache.direct=baz' $TEST_IMG # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out index 48a9d087f0..551096a9c4 100644 --- a/tests/qemu-iotests/133.out +++ b/tests/qemu-iotests/133.out @@ -32,4 +32,10 @@ Cannot set both -r/-w and 'read-only' Cannot set both -c and the cache options Cannot set both -c and the cache options Cannot set both -c and the cache options + +=== Check that invalid options are handled correctly === + +Parameter 'read-only' expects 'on' or 'off' +Parameter 'cache.no-flush' expects 'on' or 'off' +Parameter 'cache.direct' expects 'on' or 'off' *** done
This function takes three options (cache.direct, cache.no-flush and read-only) from a QemuOpts object and updates the flags accordingly. If any of those options is not set (because it was missing from the original QDict or because it had an invalid value) then the function aborts with a failed assertion: $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2 block.c:1126: update_flags_from_options: Assertion `qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)' failed. Aborted This assertion is unnecessary, and it forces any caller of bdrv_reopen() to pass all the aforementioned three options. This may have made sense in order to remove ambiguity when bdrv_reopen() was taking both flags and options, but that's not the case anymore. It's also unnecessary if we want to validate the option values, because bdrv_reopen_prepare() already takes care of that, as we can see if we remove the assertions: $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2 Parameter 'read-only' expects 'on' or 'off' Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 4 ---- tests/qemu-iotests/133 | 8 ++++++++ tests/qemu-iotests/133.out | 6 ++++++ 3 files changed, 14 insertions(+), 4 deletions(-)