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; Mon, 15 Jul 2019 14:56:25 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6555130832CC; Mon, 15 Jul 2019 21:56:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-21.ams2.redhat.com [10.36.117.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id A0D3D60BEC; Mon, 15 Jul 2019 21:56:21 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64Decode() To: "Gao, Liming" , "Gao, Zhichao" 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> From: "Laszlo Ersek" Message-ID: Date: Mon, 15 Jul 2019 23:56:20 +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: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4A8656@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Mon, 15 Jul 2019 21:56:25 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 07/15/19 17:22, Gao, Liming wrote: > Laszlo: >=20 >> -----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 ; devel@edk2.groups.io; Gao, L= iming ; Kinney, Michael D >> >> Cc: Marvin H=C3=A4user ; Philippe Mathieu-Daud=C3= =A9 >> Subject: Re: [edk2-devel] [PATCH 2/3] MdePkg/BaseLib: rewrite Base64De= code() >> >> 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 t= he memory address check. See >> https://github.com/ZhichaoGao/edk2/commit/615356ba32d3147957d215debd84= 4e7709f06849 . It is tested in Emulator environment. If >> it is OK, I think you can take my Tested-by for this patch. If there a= re some missing, please let me know. >> >> Thanks for writing that test app. It seems to have pretty good coverag= e. >> I like that it covers the exit points systematically. >> >> Mike, Liming: I intend to pick up Zhichao's T-b, from above. If you ar= e >> aware of another test suite (perhaps used in conjunction with the >> originally contributed Base64Decode() impl), please let me know. >=20 > OK to me. >=20 >> >>> 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. >=20 > Yes. Source is NULL, SourceSize is zero. Code does nothing. I am fine w= ith it.=20 >=20 > 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 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 scop= e. - 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