qemu-block
[Top][All Lists]
Advanced

[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: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()
Date: Thu, 08 Oct 2015 10:12:48 +0200
User-agent: Notmuch/0.13.2 (http://notmuchmail.org) Emacs/23.2.1 (i486-pc-linux-gnu)

On Wed 07 Oct 2015 08:51:21 PM CEST, Max Reitz <address@hidden> wrote:
>> +    if (s->num_children == s->max_children) {
>> +        if (s->max_children >= INT_MAX) {
>
> Opposing Berto (:-)), I like >= even if the > part is actually
> impossible. I myself like to use constructs such as:
>
> for (i = 0; i < n; i++) {
>     /* ... */
> }
> if (i >= n) {
>     /* ... */
> }
>
> Even though @i can never exceed @n after the loop. This is because my
> way of thinking is "What if it could exceed @n? Then I'd like to take
> this branch as well." The same applies here. s->max_children can never
> exceed INT_MAX, but if it could, we'd want that to be an error, too.

If s->max_children (and therefore s->num_children) could exceed the
upper limit then we would probably want to assert on that, because it
means that there's something seriously broken. The purpose of this code
is to make sure that the upper limit is... well, an upper limit :-) If
that invariant no longer holds then I don't think we want a simple "Too
many children" error.

>> +        s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
>> +        s->bs[s->num_children] = NULL;
>> +        s->max_children++;
>> +    }
>
> Just a suggestion, please feel free to ignore it completely:
>
> You can drop the s->max_children field and just always call g_renew()
> with s->num_children + 1 as the @count parameter. There shouldn't be
> any (visible) performance penalty, but it would simplify the code.

If s->num_children has decreased since the previous g_renew() call
(because the user called quorum_del_child()) that could actually reduce
the array size. Nothing would break though, at worst it would just be a
bit counter-intuitive :-)

https://www.gnu.org/software/libc/manual/html_node/Changing-Block-Size.html

Berto



reply via email to

[Prev in Thread] Current Thread [Next in Thread]