List:Internals« Previous MessageNext Message »
From:Michael Widenius Date:November 21 2002 12:25am
Subject:Re: bk commit into 3.23 tree
View as plain text  
Hi!

>>>>> "Benjamin" == Benjamin Pflugmann <benjamin-mysql@stripped>
> writes:
<cut>

>> +++ 1.15/mysys/mf_keycache.c	Mon Nov 18 14:59:43 2002
>> @@ -548,13 +548,14 @@
>> count++;
>> }
>> /* Only allocate a new buffer if its bigger than the one we have */
>> -      if (count <= FLUSH_CACHE ||
>> -	  !(cache=(SEC_LINK**) my_malloc(sizeof(SEC_LINK*)*count,MYF(0))))
>> +      if (count > FLUSH_CACHE)
>> {
>> -	cache=cache_buff;		/* Fall back to safe buffer */
>> -	count=FLUSH_CACHE;
>> +	if (!(cache=(SEC_LINK**) my_malloc(sizeof(SEC_LINK*)*count,MYF(0))))
>> +        {
>> +	  cache=cache_buff;		/* Fall back to safe buffer */
>> +	  count=FLUSH_CACHE;
>> +        }
>> }
>> -      end=cache+count;
>> }

Benjamin> Sorry, if I am ignorant here, but before the change, it was assured,
Benjamin> that

Benjamin>   count <= FLASH_CACHE

Benjamin> after this code part.

The old code was:

      if (count <= FLUSH_CACHE ||
          !(cache=(SEC_LINK**) my_malloc(sizeof(SEC_LINK*)*count,MYF(0))))
      {
        cache=cache_buff;               /* Fall back to safe buffer */
        count=FLUSH_CACHE;
      }

Assume that count > FLUSH_CACHE

In this case there is two options:

- The malloc succeeds
  Then it's ok that count contains a bigger value than FLUSH_CACHE
  as the buffer is large enough to hold count values.
- If malloc fails, then count is set to FLUSH_CACHE and we are setting
  cache to point to the stack allocated cache of FLUSH_CACHE size.

In other words, here count can be bigger than FLUSH_CACHE if the
malloc succeeds.

The 'tricky' part with the old code is that it also set
count to FLUSH_CACHE if count was < FLUSH_CACHE.
This was ok as the loop afterwards will stop as soon as the list is
exhausted, which will happen before count reaches FLUSH_CACHE.

This was not obvious so I changed the code to make it easier to read:

      if (count > FLUSH_CACHE)
      {
        if (!(cache=(SEC_LINK**) my_malloc(sizeof(SEC_LINK*)*count,MYF(0))))
        {
          cache=cache_buff;             /* Fall back to safe buffer */
          count=FLUSH_CACHE;
        }
      }

Benjamin> Now, it may be left at a bigger value (if the
Benjamin> call to my_malloc fails).

If my_malloc() failed in the old case then count was set to FLUSH_CACHE.

In the new code we do exactly the same thing.

Did you misread the patch or did I miss something ?

<cut>

Benjamin> No long explanation needed, I just wanted to make you aware of this,
Benjamin> in case it is an issue.

Thanks for the concern but I belive the new code is ok.  If not,
please respond to me where the above logic fails.

Regards,
Monty
Thread
bk commit into 3.23 treemonty18 Nov
  • Re: bk commit into 3.23 treeBenjamin Pflugmann18 Nov
    • Re: bk commit into 3.23 treeMichael Widenius21 Nov
      • Re: bk commit into 3.23 treeBenjamin Pflugmann22 Nov