Re: [Qemu-devel] [Patch for-2.5 v2 4/6] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang <wency@cn.fujitsu.com>]

On 09/01/2015 02:53 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/quorum.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/quorum.c b/block/quorum.c
>> index 2f6c45f..1305086 100644
>> --- a/block/quorum.c
>> +++ 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 will larger than maximum.
> 
> s/will/grows/
> 
> 
>> @@ -995,6 +999,70 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
>>      }
>>  }
>>  
>> +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) {
>> +            error_setg(errp, "Too many children");
>> +            return;
>> +        }
>> +
>> +        s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
>> +        s->bs[s->num_children] = NULL;
>> +        s->max_children += 1;
> 
> why not use ++?
> 
>> +    }
>> +
>> +    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++;
>> +}
>> +
>> +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;
>> +    }
> 
> The previous patch already assert()ed that the child was present; can't
> this one just assert(i < s->num_children)?
> 
>> +
>> +    if (s->num_children <= s->threshold) {
>> +        error_setg(errp,
>> +            "The number of children cannot be lower than the vote threshold");
>> +        return;
> 
> Might be nice to include the numeric value of that threshold in the
> error message.
> 
>> +    }
>> +
>> +    if (s->num_children == 1) {
>> +        error_setg(errp, "Cannot remove the last child");
>> +        return;
>> +    }
> 
> Isn't this dead code, as the vote threshold always has to be at least 1,
> so the previous 'if' already rejected an attempt to go lower than the
> threshold?

Yes, the vote threshold has to be at least 1. But current codes have a bug, and
vote threshold can be 0. The patch is queued in Max's tree. I will remove
it in the later version.

All other comments will be addressed in the next version.

Thanks
Wen Congyang

> 
>> +
>> +    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 *));
> 
> Spaces around '+'.
> 
>> +    s->num_children--;
>> +    s->bs[s->num_children] = NULL;
>> +    bdrv_unref(child_bs);
>> +}
>> +
>>  static void quorum_refresh_filename(BlockDriverState *bs)
>>  {
>>      BDRVQuorumState *s = bs->opaque;
>> @@ -1049,6 +1117,9 @@ static BlockDriver bdrv_quorum = {
>>      .bdrv_detach_aio_context            = quorum_detach_aio_context,
>>      .bdrv_attach_aio_context            = quorum_attach_aio_context,
>>  
>> +    .bdrv_add_child                     = quorum_add_child,
>> +    .bdrv_del_child                     = quorum_del_child,
>> +
>>      .is_filter                          = true,
>>      .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
>>  };
>>
> 
> Overall seems reasonable.
>