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

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(-)

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
>