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

[Kevin Wolf]

Am 09.05.2016 um 17:52 hat Alberto Garcia geschrieben:
> On Wed 13 Apr 2016 10:33:08 AM CEST, Changlong Xie wrote:
> 
> Sorry for the late reply!
> 
> 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? :)
> 
> > +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.
> 
> > +    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;

Without having checked the context, this code is not equivalent. You
need to access s->children[s->num_children - 1] now in the second line.

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

There's also good old strstart() from cutils.c.

Kevin