On 19.09.18 16:47, Alberto Garcia wrote:
> When reopen_f() puts a block device in the reopen queue, some of the
> new options are passed using a QDict, but others ("read-only" and the
> cache options) are passed as flags.
>
> This patch puts those flags in the QDict. This way the flags parameter
> becomes redundant and we'll be able to get rid of it in a subsequent
> patch.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> qemu-io-cmds.c | 27 ++++++++++++++++++++++++++-
> tests/qemu-iotests/133 | 9 +++++++++
> tests/qemu-iotests/133.out | 8 ++++++++
> 3 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index db0b3ee5ef..4ad5269abc 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -10,6 +10,7 @@
>
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> +#include "qapi/qmp/qdict.h"
> #include "qemu-io.h"
> #include "sysemu/block-backend.h"
> #include "block/block.h"
> @@ -1978,6 +1979,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
> int flags = bs->open_flags;
> bool writethrough = !blk_enable_write_cache(blk);
> bool has_rw_option = false;
> + bool has_cache_option = false;
>
> BlockReopenQueue *brq;
> Error *local_err = NULL;
> @@ -1989,6 +1991,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
> error_report("Invalid cache option: %s", optarg);
> return -EINVAL;
> }
> + has_cache_option = true;
> break;
> case 'o':
> if (!qemu_opts_parse_noisily(&reopen_opts, optarg, 0)) {
> @@ -2046,9 +2049,31 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)
> }
>
> qopts = qemu_opts_find(&reopen_opts, NULL);
> - opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
> + opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : qdict_new();
> qemu_opts_reset(&reopen_opts);
>
> + if (qdict_haskey(opts, BDRV_OPT_READ_ONLY)) {
> + if (has_rw_option) {
> + error_report("Can't set both -r/-w and '" BDRV_OPT_READ_ONLY "'");
I'm not a fan of elision ("can't") in user interfaces. (In fact, I'm
not a fan of elision anywhere in the code base or commit logs, but I
don't put up a struggle there.)
> + qobject_unref(opts);
> + return -EINVAL;
> + }
> + } else {
> + qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !(flags & BDRV_O_RDWR));
> + }
> +
> + if (qdict_haskey(opts, BDRV_OPT_CACHE_DIRECT) ||
> + qdict_haskey(opts, BDRV_OPT_CACHE_NO_FLUSH)) {
> + if (has_cache_option) {
> + error_report("Can't set both -c and the cache options");
> + qobject_unref(opts);
> + return -EINVAL;
> + }
> + } else {
> + qdict_put_bool(opts, BDRV_OPT_CACHE_DIRECT, flags & BDRV_O_NOCACHE);
> + qdict_put_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, flags & BDRV_O_NO_FLUSH);
> + }
> +
> bdrv_subtree_drained_begin(bs);
> brq = bdrv_reopen_queue(NULL, bs, opts, flags);
> bdrv_reopen_multiple(bdrv_get_aio_context(bs), brq, &local_err);
> diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
> index b9f17c996f..a58130c5b4 100755
> --- a/tests/qemu-iotests/133
> +++ b/tests/qemu-iotests/133
> @@ -98,6 +98,15 @@ echo
>
> $QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG
>
> +echo
> +echo "=== Check that mixing -c/-r/-w and their equivalent options is forbidden ==="
> +echo
> +
> +$QEMU_IO -c 'reopen -r -o read-only=on' $TEST_IMG
> +$QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG
The equivalent option however would be read-only=off.
> +$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG
> +$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG
But the equivalent option would be cache.direct=off.
> +$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG
And cache.direct=on here. Or rather, the equivalent -c value for
cache.no-flush=on would be "unsafe".
It doesn't really matter, but if you don't care, it shouldn't say
"equivalent options" but maybe "corresponding options".
Overall, the patch looks good.
Max
> # success, all done
> echo "*** done"
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
> index 570f623b6b..4d84401688 100644
> --- a/tests/qemu-iotests/133.out
> +++ b/tests/qemu-iotests/133.out
> @@ -28,4 +28,12 @@ format name: null-co
> === Check that invalid options are handled correctly ===
>
> Parameter 'read-only' expects 'on' or 'off'
> +
> +=== Check that mixing -c/-r/-w and their equivalent options is forbidden ===
> +
> +Can't set both -r/-w and 'read-only'
> +Can't set both -r/-w and 'read-only'
> +Can't set both -c and the cache options
> +Can't set both -c and the cache options
> +Can't set both -c and the cache options
> *** done
>