From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51239) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zk0fE-0001c3-Hk for qemu-devel@nongnu.org; Wed, 07 Oct 2015 22:10:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zk0fD-0001Dv-7V for qemu-devel@nongnu.org; Wed, 07 Oct 2015 22:10:36 -0400 References: <1442907862-21376-1-git-send-email-wency@cn.fujitsu.com> <1442907862-21376-3-git-send-email-wency@cn.fujitsu.com> From: Wen Congyang Message-ID: <5615D086.1090505@cn.fujitsu.com> Date: Thu, 8 Oct 2015 10:10:14 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu devel , Eric Blake , Markus Armbruster , Stefan Hajnoczi Cc: Kevin Wolf , zhanghailiang , qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Gonglei , Yang Hongyang On 10/07/2015 10:12 PM, Alberto Garcia wrote: > On Tue 22 Sep 2015 09:44:20 AM CEST, Wen Congyang wrote: > >> +++ b/block/quorum.c >> @@ -66,6 +66,9 @@ typedef struct QuorumVotes { >> typedef struct BDRVQuorumState { >> BlockDriverState **bs; /* children BlockDriverStates */ >> int num_children; /* children count */ >> + int max_children; /* The maximum children count, we need to reallocate >> + * bs if num_children grows larger than maximum. >> + */ >> int threshold; /* if less than threshold children reads gave the >> * same result a quorum error occurs. >> */ > > As you announce in the cover letter of this series, your code depends on > the parents list patch written by Kevin here: > > http://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04579.html > > As you might be aware, and as part of the same series by Kevin, > BDRVQuorumState will no longer hold a list of BlockDriverState but a > list of BdrvChild instead: > > https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04571.html I notice that, and I only one patch from Kevin now. I will fix it in the next version. > >> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, >> + Error **errp) >> +{ >> + BDRVQuorumState *s = bs->opaque; >> + >> + bdrv_drain(bs); >> + >> + if (s->num_children == s->max_children) { >> + if (s->max_children >= INT_MAX) { >> + error_setg(errp, "Too many children"); >> + return; >> + } > > max_children can never be greater than INT_MAX. Use == instead. > >> + s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1); >> + s->bs[s->num_children] = NULL; > > No need to set the pointer to NULL here, and you are anyway setting the > pointer to the new child a few lines afterwards. Yes, I will remove it in the next version. > >> + s->max_children++; >> + } >> + >> + bdrv_ref(child_bs); >> + bdrv_attach_child(bs, child_bs, &child_format); >> + s->bs[s->num_children++] = child_bs; >> +} >> + >> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs, >> + Error **errp) >> +{ >> + BDRVQuorumState *s = bs->opaque; >> + BdrvChild *child; >> + int i; >> + >> + for (i = 0; i < s->num_children; i++) { >> + if (s->bs[i] == child_bs) { >> + break; >> + } >> + } >> + >> + QLIST_FOREACH(child, &bs->children, next) { >> + if (child->bs == child_bs) { >> + break; >> + } >> + } >> + >> + /* we have checked it in bdrv_del_child() */ >> + assert(i < s->num_children && child); >> + >> + if (s->num_children <= s->threshold) { >> + error_setg(errp, >> + "The number of children cannot be lower than the vote threshold %d", >> + s->threshold); >> + return; >> + } >> + >> + bdrv_drain(bs); >> + /* We can safely remove this child now */ >> + memmove(&s->bs[i], &s->bs[i + 1], >> + (s->num_children - i - 1) * sizeof(void *)); >> + s->num_children--; >> + s->bs[s->num_children] = NULL; > > Same here, no one will check or use s->bs[s->num_children] so there's no > need to make it NULL. > > Apart from the issue of using only part of Kevin's series, the rest are > minor things. I will fix it in the next version. > > Thanks and sorry for the late review! Thanks for your review Wen Congyang > > Berto > . >