[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v5 2/4] quorum: implement bdrv_add_child() and b
From: |
Wen Congyang |
Subject: |
Re: [Qemu-block] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child() |
Date: |
Thu, 8 Oct 2015 10:10:14 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
On 10/07/2015 10:12 PM, Alberto Garcia wrote:
> On Tue 22 Sep 2015 09:44:20 AM CEST, Wen Congyang wrote:
>
>> +++ 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 grows larger than maximum.
>> + */
>> int threshold; /* if less than threshold children reads gave the
>> * same result a quorum error occurs.
>> */
>
> As you announce in the cover letter of this series, your code depends on
> the parents list patch written by Kevin here:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04579.html
>
> As you might be aware, and as part of the same series by Kevin,
> BDRVQuorumState will no longer hold a list of BlockDriverState but a
> list of BdrvChild instead:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2015-09/msg04571.html
I notice that, and I only one patch from Kevin now. I will fix it in the
next version.
>
>> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState
>> *child_bs,
>> + Error **errp)
>> +{
>> + BDRVQuorumState *s = bs->opaque;
>> +
>> + bdrv_drain(bs);
>> +
>> + if (s->num_children == s->max_children) {
>> + if (s->max_children >= INT_MAX) {
>> + error_setg(errp, "Too many children");
>> + return;
>> + }
>
> max_children can never be greater than INT_MAX. Use == instead.
>
>> + s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
>> + s->bs[s->num_children] = NULL;
>
> No need to set the pointer to NULL here, and you are anyway setting the
> pointer to the new child a few lines afterwards.
Yes, I will remove it in the next version.
>
>> + s->max_children++;
>> + }
>> +
>> + bdrv_ref(child_bs);
>> + bdrv_attach_child(bs, child_bs, &child_format);
>> + s->bs[s->num_children++] = child_bs;
>> +}
>> +
>> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState
>> *child_bs,
>> + Error **errp)
>> +{
>> + BDRVQuorumState *s = bs->opaque;
>> + BdrvChild *child;
>> + int i;
>> +
>> + for (i = 0; i < s->num_children; i++) {
>> + if (s->bs[i] == child_bs) {
>> + break;
>> + }
>> + }
>> +
>> + QLIST_FOREACH(child, &bs->children, next) {
>> + if (child->bs == child_bs) {
>> + break;
>> + }
>> + }
>> +
>> + /* we have checked it in bdrv_del_child() */
>> + assert(i < s->num_children && child);
>> +
>> + if (s->num_children <= s->threshold) {
>> + error_setg(errp,
>> + "The number of children cannot be lower than the vote threshold
>> %d",
>> + s->threshold);
>> + return;
>> + }
>> +
>> + 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 *));
>> + s->num_children--;
>> + s->bs[s->num_children] = NULL;
>
> Same here, no one will check or use s->bs[s->num_children] so there's no
> need to make it NULL.
>
> Apart from the issue of using only part of Kevin's series, the rest are
> minor things.
I will fix it in the next version.
>
> Thanks and sorry for the late review!
Thanks for your review
Wen Congyang
>
> Berto
> .
>