From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55776) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b01bJ-0004HT-L8 for qemu-devel@nongnu.org; Tue, 10 May 2016 02:57:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b01bH-0006VF-6s for qemu-devel@nongnu.org; Tue, 10 May 2016 02:57:00 -0400 Message-ID: <573186EB.1090008@cn.fujitsu.com> Date: Tue, 10 May 2016 14:59:55 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1460536389-9161-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1460536389-9161-3-git-send-email-xiecl.fnst@cn.fujitsu.com> In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v13 2/3] 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 , Kevin Wolf , Max Reitz , Stefan Hajnoczi Cc: Markus Armbruster , "Dr. David Alan Gilbert" , Dong Eddie , Jiang Yunhong , Wen Congyang , qemu block On 05/09/2016 11:52 PM, Alberto Garcia wrote: > On Wed 13 Apr 2016 10:33:08 AM CEST, Changlong Xie wrote: > > Sorry for the late reply! > Never mind : ) > The patch looks good, I have some additional comments on top of what Max > Wrote, nothing serious though :) > >> @@ -67,6 +68,9 @@ typedef struct QuorumVotes { >> typedef struct BDRVQuorumState { >> BdrvChild **children; /* children BlockDriverStates */ >> int num_children; /* children count */ >> + uint64_t last_index; /* indicate the child role name of the last >> + * element of children array >> + */ > > I think you can use a regular 'unsigned' here, it's simpler and more > efficient. We're not going to have 2^32 Quorum children, are we? :) > Actually, i tried to use 'unsinged' here in my first version. But thinking of if someone did crazy child add/delete test(add 10 children per second), it will overflow in about 2^32/(60*60*24*365*10) = 13 years, so i choiced uint64_t(2^64 is big enough) here. Now, i argree with you, it's overwrought. Will use 'unsigned' in next version. >> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, >> + Error **errp) >> +{ >> + BDRVQuorumState *s = bs->opaque; >> + BdrvChild *child; >> + char indexstr[32]; >> + int ret; >> + >> + assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) && >> + s->last_index <= UINT64_MAX); > > That last condition has no practical effect. last_index is a uint64_t so > s->last_index <= UINT64_MAX is always going to be true. Yes, it's redundant code. > >> + s->children = g_renew(BdrvChild *, s->children, s->num_children + 1); >> + s->children[s->num_children++] = child; > > Slightly simpler way: > > s->children = g_renew(BdrvChild *, s->children, ++s->num_children); > s->children[s->num_children] = child; Overflow arrays, should (s->num_children - 1) here. I'll keep my original one. > > But this is not very important, so you can leave it as it is now if you > prefer. > >> + /* child->name is "children.%d" */ >> + assert(!strncmp(child->name, "children.", 9)); > > I actually don't think there's anything wrong with this assertion, but > if you decide to keep it you can use g_str_has_prefix() instead, which > is a bit easier and more readable. > Just as Max said, it's extra check, and will remove it. >> + /* We can safely remove this child now */ >> + memmove(&s->children[i], &s->children[i + 1], >> + (s->num_children - i - 1) * sizeof(BdrvChild *)); >> + s->children = g_renew(BdrvChild *, s->children, --s->num_children); >> + bdrv_unref_child(bs, child); > > Question: do we want to decrement last_index if 'i' is the last > children? Something like: > I agree with Max, it seems no benifit(although will save number resources if (i == s->num_children - 1)) here. Thanks -Xie > if (i == s->num_children - 1) { > s->last_index--; > } else { > memmove(...) > } > s->children = g_renew(...) > > Berto > > > . >