[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 12/14] block: Clean up reopen_backing_file() in
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH 12/14] block: Clean up reopen_backing_file() in block/replication.c |
Date: |
Mon, 8 Oct 2018 04:34:24 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
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 <address@hidden>
> ---
> 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) {
>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 12/14] block: Clean up reopen_backing_file() in block/replication.c,
Max Reitz <=