From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51092) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ0TN-0008MG-9W for qemu-devel@nongnu.org; Mon, 23 Feb 2015 16:23:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQ0TH-0000VM-Eh for qemu-devel@nongnu.org; Mon, 23 Feb 2015 16:23:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38714) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQ0TH-0000VD-6i for qemu-devel@nongnu.org; Mon, 23 Feb 2015 16:23:19 -0500 Message-ID: <54EB9A26.5060002@redhat.com> Date: Mon, 23 Feb 2015 16:22:46 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423710438-14377-1-git-send-email-wency@cn.fujitsu.com> <1423710438-14377-6-git-send-email-wency@cn.fujitsu.com> In-Reply-To: <1423710438-14377-6-git-send-email-wency@cn.fujitsu.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 05/14] quorom: implement block driver interfaces for block replication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wen Congyang , qemu devel , Kevin Wolf , Stefan Hajnoczi , Paolo Bonzini Cc: Lai Jiangshan , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Gonglei , Yang Hongyang , zhanghailiang On 2015-02-11 at 22:07, Wen Congyang wrote: > Signed-off-by: Wen Congyang > Signed-off-by: zhanghailiang > Signed-off-by: Gonglei > --- > block/quorum.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/block/quorum.c b/block/quorum.c > index e6aff5f..c8479b4 100644 > --- a/block/quorum.c > +++ b/block/quorum.c > @@ -1070,6 +1070,71 @@ static void quorum_refresh_filename(BlockDriverState *bs) > bs->full_open_options = opts; > } > > +static int quorum_stop_replication(BlockDriverState *bs); > +static int quorum_start_replication(BlockDriverState *bs, int mode) > +{ > + BDRVQuorumState *s = bs->opaque; > + int ret = -1, i; Again, I'd prefer it if you used -errno (or the Error API). > + > + /* > + * TODO: support COLO_SECONDARY_MODE if we allow secondary > + * QEMU becoming primary QEMU. > + */ > + if (mode != COLO_PRIMARY_MODE) { > + return -1; > + } > + > + if (s->read_pattern != QUORUM_READ_PATTERN_FIRST) { > + return -1; > + } > + > + /* NBD client should not be the first child */ > + if (bdrv_start_replication(s->bs[0], mode) == 0) { If you allow the NBD client to be the first child you can probably drop this block (and start from "i = 0" in the for loop). > + bdrv_stop_replication(s->bs[0]); > + return -1; > + } > + > + for (i = 1; i < s->num_children; i++) { > + if (bdrv_start_replication(s->bs[i], mode) == 0) { > + ret++; > + } > + } > + > + if (ret > 0) { > + quorum_stop_replication(bs); > + } I think it would be easier to read if you had an additional "count" variable which is set to 0 before the for loop and then incremented (instead of ret). This would then be "if (count > 1)". > + > + return ret ? -1 : 0; And this would be "return count == 0 ? 0 : -ENOTSUP" or something. But apart from that, what's so bad about having multiple children which support bdrv_start_replication()? I mean, other than "It's not what we intended". Max > +} > + > +static int quorum_do_checkpoint(BlockDriverState *bs) > +{ > + BDRVQuorumState *s = bs->opaque; > + int i; > + > + for (i = 1; i < s->num_children; i++) { > + if (bdrv_do_checkpoint(s->bs[i]) == 0) { > + return 0; > + } > + } > + > + return -1; > +} > + > +static int quorum_stop_replication(BlockDriverState *bs) > +{ > + BDRVQuorumState *s = bs->opaque; > + int ret = -1, i; > + > + for (i = 0; i < s->num_children; i++) { > + if (bdrv_stop_replication(s->bs[i]) == 0) { > + ret++; > + } > + } > + > + return ret ? -1 : 0; > +} > + > static BlockDriver bdrv_quorum = { > .format_name = "quorum", > .protocol_name = "quorum", > @@ -1093,6 +1158,10 @@ static BlockDriver bdrv_quorum = { > > .is_filter = true, > .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter, > + > + .bdrv_start_replication = quorum_start_replication, > + .bdrv_do_checkpoint = quorum_do_checkpoint, > + .bdrv_stop_replication = quorum_stop_replication, > }; > > static void bdrv_quorum_init(void)