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

[Changlong Xie]

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