Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

[Max Reitz]

On 08.10.2015 10:12, Alberto Garcia wrote:
> 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.

Yes, it could. And that would be just fine. ;-)

We'd just keep the array exactly as big as it needs to be. I find that
pretty intuitive. It's just counter-intuitive if you think one should
never use realloc() for reducing the size of a buffer (and I know I
myself tend to write my code thinking that).

Max

>                 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
>

signature.asc
Description: OpenPGP digital signature