Message ID | 2534f867ea9be5b666dfce19744b7d4e2b96c976.1626619393.git.lukasstraub2@web.de |
---|---|
State | New |
Headers | show |
Series | replication: Bugfix and properly attach children | expand |
> -----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
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;