[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Changlong Xie
Subject: Re: [Qemu-devel] [PATCH v13 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()
Date: Tue, 10 May 2016 14:59:55 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

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

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


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]