Re: [Qemu-devel] [PATCH 11/14] qemu-io: Put flag changes in the options QDict in reopen_f()

[Max Reitz]

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 <address@hidden>
> ---
>  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
>

signature.asc
Description: OpenPGP digital signature