I added this task:
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.
>>> 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. :)
> Luís Soares