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

[Alberto Garcia]

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;

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.

> +    /* 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:

if (i == s->num_children - 1) {
   s->last_index--;
} else {
   memmove(...)
}
s->children = g_renew(...)

Berto