On 08.10.2015 10:12, Alberto Garcia wrote:
> On Wed 07 Oct 2015 08:51:21 PM CEST, Max Reitz <mre...@redhat.com> 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
