Re: [Qemu-block] [Patch for-2.5 v2 4/6] quorum: implement bdrv_add_child() and bdrv_del_child()

[Eric Blake]

On 08/11/2015 01:51 AM, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> Reviewed-by: Alberto Garcia <address@hidden>
> ---
>  block/quorum.c | 75 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 2f6c45f..1305086 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -66,6 +66,9 @@ typedef struct QuorumVotes {
>  typedef struct BDRVQuorumState {
>      BlockDriverState **bs; /* children BlockDriverStates */
>      int num_children;      /* children count */
> +    int max_children;      /* The maximum children count, we need to 
> reallocate
> +                            * bs if num_children will larger than maximum.

s/will/grows/


> @@ -995,6 +999,70 @@ static void quorum_attach_aio_context(BlockDriverState 
> *bs,
>      }
>  }
>  
> +static void quorum_add_child(BlockDriverState *bs, QDict *options, Error 
> **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int ret;
> +    Error *local_err = NULL;
> +
> +    bdrv_drain(bs);
> +
> +    if (s->num_children == s->max_children) {
> +        if (s->max_children >= INT_MAX) {
> +            error_setg(errp, "Too many children");
> +            return;
> +        }
> +
> +        s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
> +        s->bs[s->num_children] = NULL;
> +        s->max_children += 1;

why not use ++?

> +    }
> +
> +    ret = bdrv_open_image(&s->bs[s->num_children], NULL, options, "child", 
> bs,
> +                          &child_format, false, &local_err);
> +    if (ret < 0) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    s->num_children++;
> +}
> +
> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState 
> *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int i;
> +
> +    for (i = 0; i < s->num_children; i++) {
> +        if (s->bs[i] == child_bs) {
> +            break;
> +        }
> +    }
> +
> +    if (i == s->num_children) {
> +        error_setg(errp, "Invalid child");
> +        return;
> +    }

The previous patch already assert()ed that the child was present; can't
this one just assert(i < s->num_children)?

> +
> +    if (s->num_children <= s->threshold) {
> +        error_setg(errp,
> +            "The number of children cannot be lower than the vote 
> threshold");
> +        return;

Might be nice to include the numeric value of that threshold in the
error message.

> +    }
> +
> +    if (s->num_children == 1) {
> +        error_setg(errp, "Cannot remove the last child");
> +        return;
> +    }

Isn't this dead code, as the vote threshold always has to be at least 1,
so the previous 'if' already rejected an attempt to go lower than the
threshold?

> +
> +    bdrv_drain(bs);
> +    /* We can safely remove this child now */
> +    memmove(&s->bs[i], &s->bs[i+1], (s->num_children - i - 1) * sizeof(void 
> *));

Spaces around '+'.

> +    s->num_children--;
> +    s->bs[s->num_children] = NULL;
> +    bdrv_unref(child_bs);
> +}
> +
>  static void quorum_refresh_filename(BlockDriverState *bs)
>  {
>      BDRVQuorumState *s = bs->opaque;
> @@ -1049,6 +1117,9 @@ static BlockDriver bdrv_quorum = {
>      .bdrv_detach_aio_context            = quorum_detach_aio_context,
>      .bdrv_attach_aio_context            = quorum_attach_aio_context,
>  
> +    .bdrv_add_child                     = quorum_add_child,
> +    .bdrv_del_child                     = quorum_del_child,
> +
>      .is_filter                          = true,
>      .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
>  };
> 

Overall seems reasonable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature