Re: [Qemu-devel] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

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

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
> .
>