From: Alexander Barkov
Date: December 2 2010 6:55am
Subject: Re: bzr commit into mysql-next-mr branch (bar:3201) WL#5510
List-Archive: http://lists.mysql.com/commits/125743
Message-Id: <4CF742CD.3000703@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Luís,

I added this task:

https://intranet.mysql.com/worklog/Server-RawIdeaBin/index.pl?tid=5703

Thanks for your suggestions!


Luís Soares wrote:
> Hi Bar,
> 
> On 11/29/2010 09:22 AM, Alexander Barkov wrote:
>> Hi Luis,
>>
>>
>> Luís Soares wrote:
>>> Hi Bar,
>>>
>>> Nice Work. Sergey has already done a fairly extensive review,
>>> so there is nothing much for me to say. I just have two small
>>> requests for clarification below...
>>>
>>> ... and sorry for taking sometime to finish this review.
>>>
>>> STATUS
>>> ------
>>>
>>> Approved after consideration of some minor issues.
>>>
>>> REQUIRED CHANGES
>>> ----------------
>>> n/a
>>>
>>> REQUESTS
>>> --------
>>>
>>> R1. In my_base64_decoder_getch(MY_BASE64_DECODER *decoder)
>>>
>>> The 'switch (decoder->state)' only asserts when the server
>>> is built with debug info. What about production binaries ?
>>> Shouldn't we at least return an error here (just to be
>>> safe)?
>>
>> Thanks for your suggestion! This should be safer.
>> I added "return TRUE" as follows:
>>
>> default:
>> DBUG_ASSERT(0);
>> return TRUE; /* Wrong state, should not happen */
> 
> OK.
> 
>>
>>>
>>> R2. Why not make the base64_decode/encode signatures different?
>>> So that they work similarly to other buffer handling/copying
>>> functions like memcpy, strcpy (and like our own functions,
>>> strmake, ...):
>>>
>>> - first parameter : dest
>>> - second parameter: source
>>> - third parameter : len
>>> - ...
>>>
>>> +base64_decode(const char *src_base, size_t len,
>>> + void *dst, const char **end_ptr)
>>>
>>>
>>> I know that you haven't touched in base64_encode function
>>> in this patch, but is something for you to consider anyway.
>>>
>>> NOTE: this is not a required change, it's a request for
>>> clarification. So I am not considering this a show
>>> stopper.
>>>
>>
>> This will need some changes in replication code,
>> which is something I'd try to avoid at this point.
> 
> Right. I don't think we should delay this task just because of
> this. We can address this signature change later.
> 
>> I propose we create a refactoring task
>> (either replication team or I could do it)
>> to do some improvements in the base64 code.
> 
> Sure, please, can you file this task?
> 
>> I agree with your signature change proposal.
> 
> Good.
> 
>> I'd also propose the following:
>> - change the size parameters from int to size_t
>> - improve the base64_needed_xxxx_length() functions,
>> to avoid overflow, so we can support the full length range.
>> base64_decode() could support up to
>> 0xFFFFFFFF input bytes on a platform with "sizeof(size_t) == 4",
>> instead of the current limit 0x2AAAAAAA.
>> base64_encode() could support 0xBFFFFFFD input characters,
>> instead of the current limit 0x5EC0D4C6.
>>
> 
> Add this when filing the task.
> 
> Oh... btw, patch approved. :)
> 
> Regards,
> Luís Soares