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.
