RE: [PATCH 1/5] block/quorum.c: stable children names

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, May 11, 2020 8:27 PM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Alberto Garcia <berto@igalia.com>; Dr. David Alan Gilbert
> <dgilbert@redhat.com>; Zhang, Chen <chen.zhang@intel.com>
> Subject: [PATCH 1/5] block/quorum.c: stable children names
> 
> If we remove the child with the highest index from the quorum, decrement
> s->next_child_index. This way we get stable children names as long as we
> only remove the last child.
> 

Looks good for me, and it can solve this bug:
colo: Can not recover colo after svm failover twice
https://bugs.launchpad.net/bugs/1881231

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  block/quorum.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c index 6d7a56bd93..acfa09c2cc
> 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -29,6 +29,8 @@
> 
>  #define HASH_LENGTH 32
> 
> +#define INDEXSTR_LEN 32
> +
>  #define QUORUM_OPT_VOTE_THRESHOLD "vote-threshold"
>  #define QUORUM_OPT_BLKVERIFY      "blkverify"
>  #define QUORUM_OPT_REWRITE        "rewrite-corrupted"
> @@ -972,9 +974,9 @@ static int quorum_open(BlockDriverState *bs, QDict
> *options, int flags,
>      opened = g_new0(bool, s->num_children);
> 
>      for (i = 0; i < s->num_children; i++) {
> -        char indexstr[32];
> -        ret = snprintf(indexstr, 32, "children.%d", i);
> -        assert(ret < 32);
> +        char indexstr[INDEXSTR_LEN];
> +        ret = snprintf(indexstr, INDEXSTR_LEN, "children.%d", i);
> +        assert(ret < INDEXSTR_LEN);
> 
>          s->children[i] = bdrv_open_child(NULL, options, indexstr, bs,
>                                           &child_format, false, &local_err); 
> @@ -1026,7 +1028,7
> @@ static void quorum_add_child(BlockDriverState *bs, BlockDriverState
> *child_bs,  {
>      BDRVQuorumState *s = bs->opaque;
>      BdrvChild *child;
> -    char indexstr[32];
> +    char indexstr[INDEXSTR_LEN];
>      int ret;
> 
>      if (s->is_blkverify) {
> @@ -1041,8 +1043,8 @@ static void quorum_add_child(BlockDriverState *bs,
> BlockDriverState *child_bs,
>          return;
>      }
> 
> -    ret = snprintf(indexstr, 32, "children.%u", s->next_child_index);
> -    if (ret < 0 || ret >= 32) {
> +    ret = snprintf(indexstr, INDEXSTR_LEN, "children.%u", s-
> >next_child_index);
> +    if (ret < 0 || ret >= INDEXSTR_LEN) {
>          error_setg(errp, "cannot generate child name");
>          return;
>      }
> @@ -1069,6 +1071,7 @@ static void quorum_del_child(BlockDriverState *bs,
> BdrvChild *child,
>                               Error **errp)  {
>      BDRVQuorumState *s = bs->opaque;
> +    char indexstr[INDEXSTR_LEN];
>      int i;
> 
>      for (i = 0; i < s->num_children; i++) { @@ -1090,6 +1093,11 @@ static 
> void
> quorum_del_child(BlockDriverState *bs, BdrvChild *child,
>      /* We know now that num_children > threshold, so blkverify must be
> false */
>      assert(!s->is_blkverify);
> 
> +    snprintf(indexstr, INDEXSTR_LEN, "children.%u", s->next_child_index - 1);
> +    if (!strncmp(child->name, indexstr, INDEXSTR_LEN)) {
> +        s->next_child_index--;
> +    }
> +
>      bdrv_drained_begin(bs);
> 
>      /* We can safely remove this child now */
> --
> 2.20.1