RE: [PATCH v6 1/4] replication: Remove s->active_disk

[Zhang, Chen]

> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Lukas Straub
> Sent: Sunday, July 18, 2021 10:48 PM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Kevin Wolf <kwolf@redhat.com>; Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com>; qemu-block <qemu-block@nongnu.org>;
> Wen Congyang <wencongyang2@huawei.com>; Xie Changlong
> <xiechanglong.d@gmail.com>; Max Reitz <mreitz@redhat.com>
> Subject: [PATCH v6 1/4] replication: Remove s->active_disk
> 
> s->active_disk is bs->file. Remove it and use local variables instead.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Looks good for me.
Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> ---
>  block/replication.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/block/replication.c b/block/replication.c index
> 774e15df16..9ad2dfdc69 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -35,7 +35,6 @@ typedef enum {
>  typedef struct BDRVReplicationState {
>      ReplicationMode mode;
>      ReplicationStage stage;
> -    BdrvChild *active_disk;
>      BlockJob *commit_job;
>      BdrvChild *hidden_disk;
>      BdrvChild *secondary_disk;
> @@ -307,8 +306,10 @@ out:
>      return ret;
>  }
> 
> -static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
> +static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
>  {
> +    BDRVReplicationState *s = bs->opaque;
> +    BdrvChild *active_disk = bs->file;
>      Error *local_err = NULL;
>      int ret;
> 
> @@ -323,13 +324,13 @@ static void
> secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
>          return;
>      }
> 
> -    if (!s->active_disk->bs->drv) {
> +    if (!active_disk->bs->drv) {
>          error_setg(errp, "Active disk %s is ejected",
> -                   s->active_disk->bs->node_name);
> +                   active_disk->bs->node_name);
>          return;
>      }
> 
> -    ret = bdrv_make_empty(s->active_disk, errp);
> +    ret = bdrv_make_empty(active_disk, errp);
>      if (ret < 0) {
>          return;
>      }
> @@ -458,6 +459,7 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>      BlockDriverState *bs = rs->opaque;
>      BDRVReplicationState *s;
>      BlockDriverState *top_bs;
> +    BdrvChild *active_disk;
>      int64_t active_length, hidden_length, disk_length;
>      AioContext *aio_context;
>      Error *local_err = NULL;
> @@ -495,15 +497,14 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>      case REPLICATION_MODE_PRIMARY:
>          break;
>      case REPLICATION_MODE_SECONDARY:
> -        s->active_disk = bs->file;
> -        if (!s->active_disk || !s->active_disk->bs ||
> -                                    !s->active_disk->bs->backing) {
> +        active_disk = bs->file;
> +        if (!active_disk || !active_disk->bs ||
> + !active_disk->bs->backing) {
>              error_setg(errp, "Active disk doesn't have backing file");
>              aio_context_release(aio_context);
>              return;
>          }
> 
> -        s->hidden_disk = s->active_disk->bs->backing;
> +        s->hidden_disk = active_disk->bs->backing;
>          if (!s->hidden_disk->bs || !s->hidden_disk->bs->backing) {
>              error_setg(errp, "Hidden disk doesn't have backing file");
>              aio_context_release(aio_context); @@ -518,7 +519,7 @@ static void
> replication_start(ReplicationState *rs, ReplicationMode mode,
>          }
> 
>          /* verify the length */
> -        active_length = bdrv_getlength(s->active_disk->bs);
> +        active_length = bdrv_getlength(active_disk->bs);
>          hidden_length = bdrv_getlength(s->hidden_disk->bs);
>          disk_length = bdrv_getlength(s->secondary_disk->bs);
>          if (active_length < 0 || hidden_length < 0 || disk_length < 0 || @@ -
> 530,9 +531,9 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>          }
> 
>          /* Must be true, or the bdrv_getlength() calls would have failed */
> -        assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
> +        assert(active_disk->bs->drv && s->hidden_disk->bs->drv);
> 
> -        if (!s->active_disk->bs->drv->bdrv_make_empty ||
> +        if (!active_disk->bs->drv->bdrv_make_empty ||
>              !s->hidden_disk->bs->drv->bdrv_make_empty) {
>              error_setg(errp,
>                         "Active disk or hidden disk doesn't support 
> make_empty"); @@ -
> 586,7 +587,7 @@ static void replication_start(ReplicationState *rs,
> ReplicationMode mode,
>      s->stage = BLOCK_REPLICATION_RUNNING;
> 
>      if (s->mode == REPLICATION_MODE_SECONDARY) {
> -        secondary_do_checkpoint(s, errp);
> +        secondary_do_checkpoint(bs, errp);
>      }
> 
>      s->error = 0;
> @@ -615,7 +616,7 @@ static void
> replication_do_checkpoint(ReplicationState *rs, Error **errp)
>      }
> 
>      if (s->mode == REPLICATION_MODE_SECONDARY) {
> -        secondary_do_checkpoint(s, errp);
> +        secondary_do_checkpoint(bs, errp);
>      }
>      aio_context_release(aio_context);
>  }
> @@ -652,7 +653,6 @@ static void replication_done(void *opaque, int ret)
>      if (ret == 0) {
>          s->stage = BLOCK_REPLICATION_DONE;
> 
> -        s->active_disk = NULL;
>          s->secondary_disk = NULL;
>          s->hidden_disk = NULL;
>          s->error = 0;
> @@ -705,7 +705,7 @@ static void replication_stop(ReplicationState *rs, bool
> failover, Error **errp)
>          }
> 
>          if (!failover) {
> -            secondary_do_checkpoint(s, errp);
> +            secondary_do_checkpoint(bs, errp);
>              s->stage = BLOCK_REPLICATION_DONE;
>              aio_context_release(aio_context);
>              return;
> @@ -713,7 +713,7 @@ static void replication_stop(ReplicationState *rs, bool
> failover, Error **errp)
> 
>          s->stage = BLOCK_REPLICATION_FAILOVER;
>          s->commit_job = commit_active_start(
> -                            NULL, s->active_disk->bs, s->secondary_disk->bs,
> +                            NULL, bs->file->bs, s->secondary_disk->bs,
>                              JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
>                              NULL, replication_done, bs, true, errp);
>          break;
> --
> 2.20.1