[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4 14/15] block: Remove assertions from update_f
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v4 14/15] block: Remove assertions from update_flags_from_options() |
Date: |
Sun, 11 Nov 2018 22:01:05 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
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
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v4 05/15] block: Use bdrv_reopen_set_read_only() in stream_start/complete(), (continued)
- [Qemu-block] [PATCH v4 05/15] block: Use bdrv_reopen_set_read_only() in stream_start/complete(), Alberto Garcia, 2018/11/07
- [Qemu-block] [PATCH v4 12/15] block: Remove flags parameter from bdrv_reopen_queue(), Alberto Garcia, 2018/11/07
- [Qemu-block] [PATCH v4 13/15] block: Stop passing flags to bdrv_reopen_queue_child(), Alberto Garcia, 2018/11/07
- [Qemu-block] [PATCH v4 03/15] block: Use bdrv_reopen_set_read_only() in commit_start/complete(), Alberto Garcia, 2018/11/07
- [Qemu-block] [PATCH v4 07/15] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit(), Alberto Garcia, 2018/11/07
- [Qemu-block] [PATCH v4 01/15] block: Add bdrv_reopen_set_read_only(), Alberto Garcia, 2018/11/07
- [Qemu-block] [PATCH v4 10/15] qemu-io: Put flag changes in the options QDict in reopen_f(), Alberto Garcia, 2018/11/07
- [Qemu-block] [PATCH v4 14/15] block: Remove assertions from update_flags_from_options(), Alberto Garcia, 2018/11/07