From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:45149)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <wency@cn.fujitsu.com>) id 1ZNBfD-00069a-Lg
	for qemu-devel@nongnu.org; Wed, 05 Aug 2015 23:16:16 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <wency@cn.fujitsu.com>) id 1ZNBfC-0007eJ-Ga
	for qemu-devel@nongnu.org; Wed, 05 Aug 2015 23:16:15 -0400
References: <1436258596-1253-1-git-send-email-wency@cn.fujitsu.com>
	<1436258596-1253-3-git-send-email-wency@cn.fujitsu.com>
	<w51zj26lygd.fsf@maestria.local.igalia.com>
From: Wen Congyang <wency@cn.fujitsu.com>
Message-ID: <55C2D164.3080307@cn.fujitsu.com>
Date: Thu, 6 Aug 2015 11:15:48 +0800
MIME-Version: 1.0
In-Reply-To: <w51zj26lygd.fsf@maestria.local.igalia.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH COLO-BLOCK v8 02/18] quorum: implement
 block driver interfaces add/delete a BDS's child
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alberto Garcia <berto@igalia.com>, qemu devel <qemu-devel@nongnu.org>, Fam Zheng <famz@redhat.com>, Max Reitz <mreitz@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, zhanghailiang <zhang.zhanghailiang@huawei.com>, qemu block <qemu-block@nongnu.org>, Jiang Yunhong <yunhong.jiang@intel.com>, Dong Eddie <eddie.dong@intel.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, "Michael R. Hines" <mrhines@linux.vnet.ibm.com>, Gonglei <arei.gonglei@huawei.com>, Yang Hongyang <yanghy@cn.fujitsu.com>

On 08/05/2015 08:19 PM, Alberto Garcia wrote:
> On Tue 07 Jul 2015 10:43:00 AM CEST, Wen Congyang wrote:
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Cc: Alberto Garcia <berto@igalia.com>
> 
> Sorry for being so late with this, I was on holidays during July.
> 
>> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    int i;
>> +
>> +    for (i = 0; i < s->num_children; i++) {
>> +        if (s->bs[i] == child_bs) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (i == s->num_children) {
>> +        error_setg(errp, "Invalid child");
>> +        return;
>> +    }
>> +
>> +    if (s->num_children <= s->threshold) {
>> +        error_setg(errp, "Cannot remove any more child");
>> +        return;
>> +    }
> 
> I think that should be "Cannot remove any more children".
> 
> You could also make it a bit more explanatory by saying "The number of
> children cannot be lower than the vote threshold" or something like
> that.
> 
>> +    bdrv_drain(bs);
>> +    /* We can safe remove this child now */
>> +    memmove(&s->bs[i], &s->bs[i+1], (s->num_children - i - 1) * sizeof(void *));
> 
> s/safe/safely/
> 
> Apart from that, the code looks good to me, so
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>

Thanks, I have send add/delete a quorum child separately. This patch is changed one line:
unref child_bs when it is removed.

I will update that patch and add your reviewed-by in that patchset.

Thanks
Wen Congyang

> 
> Berto
> .
>