Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()

["Laszlo Ersek" <lersek@redhat.com>]

On 07/15/19 17:22, Gao, Liming wrote:
> Laszlo:
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek
>> Sent: Saturday, July 13, 2019 3:31 AM
>> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Cc: Marvin Häuser <mhaeuser@outlook.de>; Philippe Mathieu-Daudé <philmd@redhat.com>
>> Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode()
>>
>> On 07/12/19 04:31, Gao, Zhichao wrote:
>>> Sorry for late respond.
>>> The whole code is OK for me. And I write a tiny test for it without the memory address check. See
>> https://github.com/ZhichaoGao/edk2/commit/615356ba32d3147957d215debd844e7709f06849 . It is tested in Emulator environment. If
>> it is OK, I think you can take my Tested-by for this patch. If there are some missing, please let me know.
>>
>> Thanks for writing that test app. It seems to have pretty good coverage.
>> I like that it covers the exit points systematically.
>>
>> Mike, Liming: I intend to pick up Zhichao's T-b, from above. If you are
>> aware of another test suite (perhaps used in conjunction with the
>> originally contributed Base64Decode() impl), please let me know.
> 
> OK to me.
> 
>>
>>> Base64Decode parameter Source is indicate as OPTIONAL. Although it is OK to be NULL, and return DestinationSize to be zero with
>> success status to indicate no decode occurred . I don't know if it is meaningful to report the result as that. In my opinion, NULL Source is
>> an invalid input. Just my opinion, if the maintainer is OK with that, I am OK too.
> 
> Yes. Source is NULL, SourceSize is zero. Code does nothing. I am fine with it. 
> 
> I have no other comments for the code logic. Reviewed-by: Liming Gao <liming.gao@intel.com>

Thank you!

A few questions:


(1) Liming, does your R-b apply to the first patch in the series as
well? (That patch too is for MdePkg.)

Here are two links to patch#1 in the series, for your convenience:

http://mid.mail-archive.com/20190702102836.27589-2-lersek@redhat.com
https://edk2.groups.io/g/devel/message/43162


(2) Zhichao, you made a good remark about block-scoped variables, and
the CCS. You also gave your Tested-by for the present version. So, we
have the following two options:

(2a)

- I push the present patch as-is, including your Tested-by. (Together
with Liming's R-b -- he is OK with the present version.)

- Subsequently, I post a separate (incremental) patch for moving the
variables from the inner block scope to the outer function block scope.
This incremental patch needs another MdePkg maintainer review, but it
doesn't need additional testing from you.

(2b) Alternatively:

- I post v2 of this series, which incorporates the movement of the
variables from the inner block scope to the outermost function block scope.

- I keep Liming's R-b. (The variable definition movement is
straight-forward and I don't think it invalidates Liming's R-b).

- I drop your Tested-by, because the variable movement technically
changes code that your testing exercised, and a Tested-by tag should
only be applied to the *exact* code that was exercised by the testing.

- Therefore, for preserving your testing work in the git history, you
would have to redo the testing, please.


Zhichao, do you prefer (2a) or (2b)?

Personally, I prefer (2a), because (2a) is safe -- importantly, the v1
code is *valid* C --, and it is less work for the community (for you,
Liming and myself, together).

Thanks!
Laszlo