List:Commits« Previous MessageNext Message »
From:Luís Soares Date:November 29 2010 10:19am
Subject:Re: bzr commit into mysql-next-mr branch (bar:3201) WL#5510
View as plain text  
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
Thread
bzr commit into mysql-next-mr branch (bar:3201) WL#5510Alexander Barkov25 Nov
  • Re: bzr commit into mysql-next-mr branch (bar:3201) WL#5510Luís Soares27 Nov
    • Re: bzr commit into mysql-next-mr branch (bar:3201) WL#5510Alexander Barkov29 Nov
      • Re: bzr commit into mysql-next-mr branch (bar:3201) WL#5510Luís Soares29 Nov
        • Re: bzr commit into mysql-next-mr branch (bar:3201) WL#5510Alexander Barkov2 Dec