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
