On 09/01/2015 02:53 AM, Eric Blake wrote:
> 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?
Yes, the vote threshold has to be at least 1. But current codes have a bug, and
vote threshold can be 0. The patch is queued in Max's tree. I will remove
it in the later version.
All other comments will be addressed in the next version.
Thanks
Wen Congyang
>
>> +
>> + 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.
>
