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