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

[Changlong Xie <xiecl.fnst@cn.fujitsu.com>]

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