From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44594) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAfWL-0005Qk-6v for qemu-devel@nongnu.org; Thu, 02 Jul 2015 10:31:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAfWH-0004fD-4N for qemu-devel@nongnu.org; Thu, 02 Jul 2015 10:31:21 -0400 Message-ID: <55954B1A.9030302@gmail.com> Date: Thu, 02 Jul 2015 22:30:50 +0800 From: Wen Congyang MIME-Version: 1.0 References: <1435635285-5804-1-git-send-email-wency@cn.fujitsu.com> <1435635285-5804-3-git-send-email-wency@cn.fujitsu.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-BLOCK v7 02/17] quorum: implement block driver interfaces add/delete a BDS's child List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , Wen Congyang , qemu devel , Fam Zheng , Max Reitz , Paolo Bonzini Cc: Kevin Wolf , Lai Jiangshan , qemu block , Jiang Yunhong , Dong Eddie , "Dr. David Alan Gilbert" , Gonglei , Stefan Hajnoczi , Yang Hongyang , zhanghailiang At 2015/7/2 22:02, Alberto Garcia Wrote: > On Tue 30 Jun 2015 05:34:30 AM CEST, Wen Congyang wrote: > >> +static void quorum_add_child(BlockDriverState *bs, QDict *options, Error **errp) >> +{ >> + BDRVQuorumState *s = bs->opaque; >> + int ret; >> + Error *local_err = NULL; >> + >> + bdrv_drain(bs); >> + >> + if (s->num_children == s->max_children) { >> + if (s->max_children >= INT_MAX / 2) { >> + error_setg(errp, "Too many children"); >> + return; >> + } >> + >> + s->bs = g_renew(BlockDriverState *, s->bs, s->max_children * 2); >> + memset(&s->bs[s->max_children], 0, s->max_children * sizeof(void *)); >> + s->max_children *= 2; >> + } >> + >> + ret = bdrv_open_image(&s->bs[s->num_children], NULL, options, "child", bs, >> + &child_format, false, &local_err); >> + if (ret < 0) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + s->num_children++; >> + >> + /* TODO: Update vote_threshold */ >> +} > > A few comments: > > 1) Is there any reason why you grow the array exponentially instead of > adding a fixed number of elements when the limit is reached? I guess > in practice the number of children is never going to be too high > anyway... Yes, will do it in the next version. > > 2) Did you think of any API to update vote_threshold? Currently it > cannot be higher than num_children, so it's effectively limited by > the number of children that are attached when the quorum device is > created. The things I think about it is: if vote_threshold-- when deleting a child, vote_threshold will be less than 0. If we don't do vote_threshold-- when it is 1, the vote_threshold will be changed when all children are added again. Which behavior is better? > > 3) I don't think it's necessary to set to NULL the pointers in s->bs[i] > when i >= num_children. There's no way to access those pointers > anyway. Same for the ' s->bs[s->num_children] = NULL; ' bit in > quorum_del_child(). I also think that using memset() for setting NULL > pointers is not portable, although QEMU is already doing this in a > few places. OK, will remove it in the next version. Just a question: why is using memset() for setting NULL pointers is not prtable? Thanks Wen Congyang > > Otherwise the code looks good to me. Thanks! > > Berto > >