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

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


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

I propose we create a refactoring task
(either replication team or I could do it)
to do some improvements in the base64 code.

I agree with your signature change proposal.

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.