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.
>> Approved after consideration of some minor issues.
>> REQUIRED CHANGES
>> 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
> Thanks for your suggestion! This should be safer.
> I added "return TRUE" as follows:
> 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
> 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.
> 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. :)