On 08/11/2015 01:51 AM, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <we...@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com>
> Signed-off-by: Gonglei <arei.gong...@huawei.com>
> Reviewed-by: Alberto Garcia <be...@igalia.com>
> ---
> 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
