Re: [Qemu-devel] [PATCH 01/14] block: Don't call update_flags_from_options() if the options are wrong

Re: [Qemu-devel] [PATCH 01/14] block: Don't call update_flags_from_options() if the options are wrong

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> If qemu_opts_absorb_qdict() fails and we carry on and call
> update_flags_from_options() then that can result on a failed
> assertion:
> 
>    $ qemu-io -c 'reopen -o read-only=foo' hd.qcow2
>    block.c:1101: update_flags_from_options: Assertion `qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)' failed.
>    Aborted
> 
> This only happens in bdrv_reopen_queue_child(). Although this function
> doesn't return errors, we can simply keep the wrong options in the
> reopen queue and later bdrv_reopen_prepare() will fail.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c                    | 11 +++++++++--
>  tests/qemu-iotests/133     |  6 ++++++
>  tests/qemu-iotests/133.out |  4 ++++
>  3 files changed, 19 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 02/14] block: Add bdrv_reopen_set_read_only()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> Most callers of bdrv_reopen() only use it to switch a BlockDriverState
> between read-only and read-write, so this patch adds a new function
> that does just that.
> 
> We also want to get rid of the flags parameter in the bdrv_reopen()
> API, so this function sets the "read-only" option and passes the
> original flags (which will then be updated in bdrv_reopen_prepare()).
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               | 17 +++++++++++++++++
>  include/block/block.h |  2 ++
>  2 files changed, 19 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 03/14] block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 04/14] block: Use bdrv_reopen_set_read_only() in commit_start/complete()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/commit.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)

Will need a rebase on 22dffcbec62ba918db690ed44beba4bd4e970bb9, but that
looks like a rather simple job, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 05/14] block: Use bdrv_reopen_set_read_only() in bdrv_commit()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/commit.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 06/14] block: Use bdrv_reopen_set_read_only() in stream_start/complete()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/stream.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

In anticipation of a rebase on 1b57488acf1:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 07/14] block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 375 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 08/14] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> This patch replaces the bdrv_reopen() calls that set and remove the
> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.

Ha!  Got you!  It's just one call this time, not "calls"! :-)

> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 08/14] block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()

[Alberto Garcia]

On Mon 08 Oct 2018 03:31:08 AM CEST, Max Reitz wrote:
> On 19.09.18 16:47, Alberto Garcia wrote:
>> This patch replaces the bdrv_reopen() calls that set and remove the
>> BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
>
> Ha!  Got you!  It's just one call this time, not "calls"! :-)

:-D

Berto

Re: [Qemu-devel] [PATCH 09/14] block: Use bdrv_reopen_set_read_only() in the mirror driver

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> The 'block-commit' QMP command is implemented internally using two
> different drivers. If the source image is the active layer then the
> mirror driver is used (commit_active_start()), otherwise the commit
> driver is used (commit_start()).
> 
> In both cases the destination image must be put temporarily in
> read-write mode. This is done correctly in the latter case, but what
> commit_active_start() does is copy all flags instead.

Well, not only commit_active_start().  mirror_exit() does exactly the
same.  It does seem on purpose to let the target have exactly the same
flags as the source eventually.

Then again, none of the other flags seem to make any sense to me here.
Therefore, I tend to agree that it is a bug fix, even though I wouldn't
say it was an oversight.

Reviewed-by: Max Reitz <mreitz@redhat.com>

...eagerly awaiting rebase on 737efc1eda2.

> This patch replaces the bdrv_reopen() calls in that function with
> bdrv_reopen_set_read_only() so that only the read-only status is
> changed.
> 
> A similar change is made in mirror_exit(), which is also used by the
> 'drive-mirror' and 'blockdev-mirror' commands.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/mirror.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 10/14] block: Drop bdrv_reopen()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> No one is using this function anymore, so we can safely remove it

Is this such a minor action to you that you think it doesn't need a full
stop? ;-)

> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               | 21 ---------------------
>  include/block/block.h |  1 -
>  2 files changed, 22 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 12/14] block: Clean up reopen_backing_file() in block/replication.c

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5041 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> This function is used to put the hidden and secondary disks in
> read-write mode before launching the backup job, and back in read-only
> mode afterwards.
> 
> This patch does the following changes:
> 
>   - Use an options QDict with the "read-only" option instead of
>     passing the changes as flags only.
> 
>   - Simplify the code (it was unnecessarily complicated and verbose).
> 
>   - Fix a bug due to which the secondary disk was not being put back
>     in read-only mode when writable=false (because in this case
>     orig_secondary_flags always had the BDRV_O_RDWR flag set).
> 
>   - Stop clearing the BDRV_O_INACTIVE flag.

Why?  Sorry, but I don't know much about replication.  Do you know this
change is OK?  (Since the code deliberately clears it right now.)

(Well, it makes sense not to re-set it afterwards...)

> The flags parameter to bdrv_reopen_queue() becomes redundant and we'll
> be able to get rid of it in a subsequent patch.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/replication.c | 42 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/block/replication.c b/block/replication.c
> index 6349d6958e..1ad0ef2ef8 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -20,6 +20,7 @@
>  #include "block/block_backup.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/error.h"
> +#include "qapi/qmp/qdict.h"
>  #include "replication.h"
>  
>  typedef enum {
> @@ -39,8 +40,8 @@ typedef struct BDRVReplicationState {
>      char *top_id;
>      ReplicationState *rs;
>      Error *blocker;
> -    int orig_hidden_flags;
> -    int orig_secondary_flags;
> +    bool orig_hidden_read_only;
> +    bool orig_secondary_read_only;
>      int error;
>  } BDRVReplicationState;
>  
> @@ -376,39 +377,32 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
>  {
>      BDRVReplicationState *s = bs->opaque;
>      BlockReopenQueue *reopen_queue = NULL;
> -    int orig_hidden_flags, orig_secondary_flags;
> -    int new_hidden_flags, new_secondary_flags;
>      Error *local_err = NULL;
>  
>      if (writable) {

I'd like to request a comment that "writable" is true the first time
this function is called, and false the second time.  That'd make it
clear why this rewrite makes sense.

(And that writable=false means reverting to the original state.)

((And no, it doesn't make more sense before this patch.))

> -        orig_hidden_flags = s->orig_hidden_flags =
> -                                bdrv_get_flags(s->hidden_disk->bs);
> -        new_hidden_flags = (orig_hidden_flags | BDRV_O_RDWR) &
> -                                                    ~BDRV_O_INACTIVE;
> -        orig_secondary_flags = s->orig_secondary_flags =
> -                                bdrv_get_flags(s->secondary_disk->bs);
> -        new_secondary_flags = (orig_secondary_flags | BDRV_O_RDWR) &
> -                                                     ~BDRV_O_INACTIVE;
> -    } else {
> -        orig_hidden_flags = (s->orig_hidden_flags | BDRV_O_RDWR) &
> -                                                    ~BDRV_O_INACTIVE;
> -        new_hidden_flags = s->orig_hidden_flags;
> -        orig_secondary_flags = (s->orig_secondary_flags | BDRV_O_RDWR) &
> -                                                    ~BDRV_O_INACTIVE;
> -        new_secondary_flags = s->orig_secondary_flags;
> +        s->orig_hidden_read_only =
> +            !(bdrv_get_flags(s->hidden_disk->bs) & BDRV_O_RDWR);
> +        s->orig_secondary_read_only =
> +            !(bdrv_get_flags(s->secondary_disk->bs) & BDRV_O_RDWR);

Why not use bdrv_is_read_only()?

Max

>      }
>  
>      bdrv_subtree_drained_begin(s->hidden_disk->bs);
>      bdrv_subtree_drained_begin(s->secondary_disk->bs);
>  
> -    if (orig_hidden_flags != new_hidden_flags) {
> -        reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs, NULL,
> -                                         new_hidden_flags);
> +    if (s->orig_hidden_read_only) {
> +        int flags = bdrv_get_flags(s->hidden_disk->bs);
> +        QDict *opts = qdict_new();
> +        qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
> +        reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
> +                                         opts, flags);
>      }
>  
> -    if (!(orig_secondary_flags & BDRV_O_RDWR)) {
> +    if (s->orig_secondary_read_only) {
> +        int flags = bdrv_get_flags(s->secondary_disk->bs);
> +        QDict *opts = qdict_new();
> +        qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
>          reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
> -                                         NULL, new_secondary_flags);
> +                                         opts, flags);
>      }
>  
>      if (reopen_queue) {
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 12/14] block: Clean up reopen_backing_file() in block/replication.c

[Alberto Garcia]

On Mon 08 Oct 2018 04:34:24 AM CEST, Max Reitz wrote:
>> This patch does the following changes:
>> 
>>   - Use an options QDict with the "read-only" option instead of
>>     passing the changes as flags only.
>> 
>>   - Simplify the code (it was unnecessarily complicated and verbose).
>> 
>>   - Fix a bug due to which the secondary disk was not being put back
>>     in read-only mode when writable=false (because in this case
>>     orig_secondary_flags always had the BDRV_O_RDWR flag set).
>> 
>>   - Stop clearing the BDRV_O_INACTIVE flag.
>
> Why?  Sorry, but I don't know much about replication.  Do you know
> this change is OK?  (Since the code deliberately clears it right now.)

I don't think the current code is ok, the BDRV_O_INACTIVE flag should be
cleared with bdrv_co_invalidate_cache() so I'm inclined to think that
it's a bug.

>> @@ -376,39 +377,32 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,
>>  {
>>      BDRVReplicationState *s = bs->opaque;
>>      BlockReopenQueue *reopen_queue = NULL;
>> -    int orig_hidden_flags, orig_secondary_flags;
>> -    int new_hidden_flags, new_secondary_flags;
>>      Error *local_err = NULL;
>>  
>>      if (writable) {
>
> I'd like to request a comment that "writable" is true the first time
> this function is called, and false the second time.  That'd make it
> clear why this rewrite makes sense.

Yes, I think it's probably a good idea to do that.

>> +        s->orig_hidden_read_only =
>> +            !(bdrv_get_flags(s->hidden_disk->bs) & BDRV_O_RDWR);
>> +        s->orig_secondary_read_only =
>> +            !(bdrv_get_flags(s->secondary_disk->bs) & BDRV_O_RDWR);
>
> Why not use bdrv_is_read_only()?

No reason, I overlooked this. I'll fix it.

Berto

Re: [Qemu-devel] [PATCH 13/14] block: Remove flags parameter from bdrv_reopen_queue()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> Now that all callers are passing all flag changes as QDict options,
> the flags parameter is no longer necessary, so we can get rid of it.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               | 5 +++--
>  block/replication.c   | 6 ++----
>  include/block/block.h | 3 +--
>  qemu-io-cmds.c        | 2 +-
>  4 files changed, 7 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6302 bytes --]

On 19.09.18 16:47, Alberto Garcia wrote:
> Now that all callers are passing the new options using the QDict we no
> longer need the 'flags' parameter.
> 
> This patch makes the following changes:
> 
>    1) The update_options_from_flags() call is no longer necessary
>       so it can be removed.
> 
>    2) The update_flags_from_options() call is now used in all cases,
>       and is moved down a few lines so it happens after the options
>       QDict contains the final set of values.
> 
>    3) The flags parameter is removed. Now the flags are initialized
>       using the current value (for the top-level node) or the parent
>       flags (after inherit_options()). In both cases the initial
>       values are updated to reflect the new options in the QDict. This
>       happens in bdrv_reopen_queue_child() (as explained above) and in
>       bdrv_reopen_prepare().
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 50 +++++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
> 
> diff --git a/block.c b/block.c
> index fd27b204d9..26c6a4e58a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2884,7 +2884,6 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,
>  static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>                                                   BlockDriverState *bs,
>                                                   QDict *options,
> -                                                 int flags,
>                                                   const BdrvChildRole *role,
>                                                   QDict *parent_options,
>                                                   int parent_flags)
> @@ -2894,6 +2893,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>      BlockReopenQueueEntry *bs_entry;
>      BdrvChild *child;
>      QDict *old_options, *explicit_options;
> +    int flags;
>  
>      /* Make sure that the caller remembered to use a drained section. This is
>       * important to avoid graph changes between the recursive queuing here and
> @@ -2919,22 +2919,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>      /*
>       * Precedence of options:
>       * 1. Explicitly passed in options (highest)
> -     * 2. Set in flags (only for top level)
> -     * 3. Retained from explicitly set options of bs
> -     * 4. Inherited from parent node
> -     * 5. Retained from effective options of bs
> +     * 2. Retained from explicitly set options of bs
> +     * 3. Inherited from parent node
> +     * 4. Retained from effective options of bs
>       */
>  
> -    if (!parent_options) {
> -        /*
> -         * Any setting represented by flags is always updated. If the
> -         * corresponding QDict option is set, it takes precedence. Otherwise
> -         * the flag is translated into a QDict option. The old setting of bs is
> -         * not considered.
> -         */
> -        update_options_from_flags(options, flags);
> -    }
> -
>      /* Old explicitly set values (don't overwrite by inherited value) */
>      if (bs_entry) {
>          old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
> @@ -2948,12 +2937,22 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>  
>      /* Inherit from parent node */
>      if (parent_options) {
> -        QemuOpts *opts;
> -        QDict *options_copy;
> -        Error *local_err = NULL;
> -        assert(!flags);
> +        flags = 0;
>          role->inherit_options(&flags, options, parent_flags, parent_options);
> -        options_copy = qdict_clone_shallow(options);
> +    } else {
> +        flags = bdrv_get_flags(bs);
> +    }
> +
> +    /* Old values are used for options that aren't set yet */
> +    old_options = qdict_clone_shallow(bs->options);
> +    bdrv_join_options(bs, options, old_options);
> +    qobject_unref(old_options);
> +
> +    /* We have the final set of options so let's update the flags */
> +    {
> +        Error *local_err = NULL;
> +        QemuOpts *opts;
> +        QDict *options_copy = qdict_clone_shallow(options);

I-I'm not sure this conforms to our coding style.

While I applaud your effort to keep the patch size small, I know for
sure I don't want to read code like this.  (Unless it's necessary
because of some variables' lifetimes.)

I agree with the changes, however.

Max

>          opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
>          qemu_opts_absorb_qdict(opts, options_copy, &local_err);
>          /* Don't call update_flags_from_options() if the options are wrong.
> @@ -2967,11 +2966,6 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>          qobject_unref(options_copy);
>      }
>  
> -    /* Old values are used for options that aren't set yet */
> -    old_options = qdict_clone_shallow(bs->options);
> -    bdrv_join_options(bs, options, old_options);
> -    qobject_unref(old_options);
> -
>      /* bdrv_open_inherit() sets and clears some additional flags internally */
>      flags &= ~BDRV_O_PROTOCOL;
>      if (flags & BDRV_O_RDWR) {
> @@ -3011,7 +3005,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
>          qdict_extract_subqdict(options, &new_child_options, child_key_dot);
>          g_free(child_key_dot);
>  
> -        bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
> +        bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options,
>                                  child->role, options, flags);
>      }
>  
> @@ -3022,9 +3016,7 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>                                      BlockDriverState *bs,
>                                      QDict *options)
>  {
> -    int flags = bdrv_get_flags(bs);
> -    return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
> -                                   NULL, NULL, 0);
> +    return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, NULL, 0);
>  }
>  
>  /*
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()

[Alberto Garcia]

On Mon 08 Oct 2018 04:48:50 AM CEST, Max Reitz wrote:

>> +    /* Old values are used for options that aren't set yet */
>> +    old_options = qdict_clone_shallow(bs->options);
>> +    bdrv_join_options(bs, options, old_options);
>> +    qobject_unref(old_options);
>> +
>> +    /* We have the final set of options so let's update the flags */
>> +    {
>> +        Error *local_err = NULL;
>> +        QemuOpts *opts;
>> +        QDict *options_copy = qdict_clone_shallow(options);
>
> I-I'm not sure this conforms to our coding style.
>
> While I applaud your effort to keep the patch size small, I know for
> sure I don't want to read code like this.  (Unless it's necessary
> because of some variables' lifetimes.)

I actually think it makes the code more readable if there are variables
with a very limited scope (like in this case). But I can also rewrite it
using a more traditional style.

Berto

Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On 08.10.18 20:13, Alberto Garcia wrote:
> On Mon 08 Oct 2018 04:48:50 AM CEST, Max Reitz wrote:
> 
>>> +    /* Old values are used for options that aren't set yet */
>>> +    old_options = qdict_clone_shallow(bs->options);
>>> +    bdrv_join_options(bs, options, old_options);
>>> +    qobject_unref(old_options);
>>> +
>>> +    /* We have the final set of options so let's update the flags */
>>> +    {
>>> +        Error *local_err = NULL;
>>> +        QemuOpts *opts;
>>> +        QDict *options_copy = qdict_clone_shallow(options);
>>
>> I-I'm not sure this conforms to our coding style.
>>
>> While I applaud your effort to keep the patch size small, I know for
>> sure I don't want to read code like this.  (Unless it's necessary
>> because of some variables' lifetimes.)
> 
> I actually think it makes the code more readable if there are variables
> with a very limited scope (like in this case). But I can also rewrite it
> using a more traditional style.

I think it should be a function call then.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 14/14] block: Stop passing flags to bdrv_reopen_queue_child()

[Alberto Garcia]

On Mon 08 Oct 2018 08:46:57 PM CEST, Max Reitz wrote:
> On 08.10.18 20:13, Alberto Garcia wrote:
>> On Mon 08 Oct 2018 04:48:50 AM CEST, Max Reitz wrote:
>> 
>>>> +    /* Old values are used for options that aren't set yet */
>>>> +    old_options = qdict_clone_shallow(bs->options);
>>>> +    bdrv_join_options(bs, options, old_options);
>>>> +    qobject_unref(old_options);
>>>> +
>>>> +    /* We have the final set of options so let's update the flags */
>>>> +    {
>>>> +        Error *local_err = NULL;
>>>> +        QemuOpts *opts;
>>>> +        QDict *options_copy = qdict_clone_shallow(options);
>>>
>>> I-I'm not sure this conforms to our coding style.
>>>
>>> While I applaud your effort to keep the patch size small, I know for
>>> sure I don't want to read code like this.  (Unless it's necessary
>>> because of some variables' lifetimes.)
>> 
>> I actually think it makes the code more readable if there are variables
>> with a very limited scope (like in this case). But I can also rewrite it
>> using a more traditional style.
>
> I think it should be a function call then.

Nah, I don't think this deserves its own function. I'd rather keep
everything in the same function. I'll send an updated patch.

Berto