From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 16 Jul 2019 03:48:16 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 87D8CC034DF3; Tue, 16 Jul 2019 10:48:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-224.ams2.redhat.com [10.36.116.224]) by smtp.corp.redhat.com (Postfix) with ESMTP id BEAED60920; Tue, 16 Jul 2019 10:48:11 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode() To: "Gao, Zhichao" , "Gao, Liming" Cc: "devel@edk2.groups.io" , "Kinney, Michael D" , =?UTF-8?Q?Marvin_H=c3=a4user?= , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20190702102836.27589-1-lersek@redhat.com> <20190702102836.27589-3-lersek@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B80750F@SHSMSX101.ccr.corp.intel.com> <35840ec1-6208-1a6f-b7a2-e63f929118fe@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E4A8656@SHSMSX104.ccr.corp.intel.com> <3CE959C139B4C44DBEA1810E3AA6F9000B807FDB@SHSMSX101.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 16 Jul 2019 12:48:10 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B807FDB@SHSMSX101.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 16 Jul 2019 10:48:15 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07/16/19 03:18, Gao, Zhichao wrote: >=20 >=20 >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Tuesday, July 16, 2019 5:56 AM >> To: Gao, Liming ; Gao, Zhichao >> >> Cc: devel@edk2.groups.io; Kinney, Michael D ; >> Marvin H=C3=A4user ; Philippe Mathieu-Daud=C3=A9 >> >> Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite >> Base64Decode() >> >> On 07/15/19 17:22, Gao, Liming wrote: >>> Laszlo: >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf O= f >>>> Laszlo Ersek >>>> Sent: Saturday, July 13, 2019 3:31 AM >>>> To: Gao, Zhichao ; devel@edk2.groups.io; Gao, >>>> Liming ; Kinney, Michael D >>>> >>>> Cc: Marvin H=C3=A4user ; Philippe Mathieu-Daud=C3= =A9 >>>> >>>> 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/615356ba32d3147957d215deb >> d8 >>>> 44e7709f06849 . 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, pleas= e let >> me know. >>>> >>>> Thanks for writing that test app. It seems to have pretty good cover= age. >>>> 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 th= e >>>> 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 i= s >>>> 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 a= m 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 >>> >> >> Thank you! >> >> A few questions: >> >> >> (1) Liming, does your R-b apply to the first patch in the series as we= ll? (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 var= iables >> from the inner block scope to the outermost function block scope. >> >> - I keep Liming's R-b. (The variable definition movement is straight-f= orward >> and I don't think it invalidates Liming's R-b). >> >> - I drop your Tested-by, because the variable movement technically cha= nges >> code that your testing exercised, and a Tested-by tag should only be a= pplied >> 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 a= nd myself, >> together). >=20 > (2a) makes less work and makes a good history, (2b) makes a clean commi= t message. I have no idea on which is better. > For me, I prefer (2a) too because I don't think clean commit message is= such important. Wait, I think we have a misunderstanding here. *Both* (2a) and (2b) are "fully clean" with regard to commit messages. Under (2a), we will have three patches for MdePkg, in the end: - specification update & present code removal (patch v1 1/3) - reimplementation (patch v1 2/3), carrying Liming's R-b and your T-b - separate update to clean up the location of the variable definitions. This needs Liming's R-b, but not your T-b. Note that the "separate update" in the end does *not* invalidate your T-b on the reimplementation patch. You *did* test that version, and your T-b is attached to *that* version. Your T-b would not be attached to the final (separate) small patch at all -- and that is alright. So it's all faithful to reality. Clean commit messages are extremely important to me. The point is that (2a) and (2b) *both* satisfy that. In other words, it's not a distinguishing factor between (2a) and (2b). > It's up to you to decide. If you choose (2b) I am fine to do a test aga= in and provide a T-B. OK, I think I will go with (2a) then. But, I still need Liming's R-b (or Mike's) on the first patch in the present series. And Jordan's on the last one (but maybe I'll just defer the last patch, for OvmfPkg, to a separate TianoCore BZ). Thanks! Laszlo