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, 14 May 2019 04:58:58 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4022F309265B; Tue, 14 May 2019 11:58:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-233.rdu2.redhat.com [10.10.120.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id B63121001F40; Tue, 14 May 2019 11:58:56 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v3 0/6] CryptoPkg: Upgrade OpenSSL to 1.1.1b From: "Laszlo Ersek" To: xiaoyux.lu@intel.com Cc: devel@edk2.groups.io, Gary Lin , Jian J Wang , Ting Ye References: <1557753912-30122-1-git-send-email-xiaoyux.lu@intel.com> Message-ID: <74aa9307-b97a-75c2-71d0-b41c111dfe15@redhat.com> Date: Tue, 14 May 2019 13:58:55 +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: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Tue, 14 May 2019 11:58:58 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/13/19 21:24, Laszlo Ersek wrote: > On 05/13/19 15:25, Xiaoyu lu wrote: >> (1) CryptoPkg/OpensslLib: Modify process_files.pl for upgrading >> OpenSSL OpenSSL only support seeding NONE for UEFI(rand_unix.c line >> 93). So add --with-rand-seed=none to process_files.pl. >> >> (2) CryptoPkg/OpensslLib: Exclude unnecessary files in >> process_files.pl >> When running process_files.py to configure OpenSSL, we can exclude >> some unnecessary files. This can reduce porting time, compiling >> time and library size. >> >> (3) CryptoPkg/IntrinsicLib: Fix possible unresolved external symbol >> issue >> >> (4) CryptoPkg/OpensslLib: Prepare for upgrading OpenSSL >> Disable warning for building OpenSSL_1_1_1b >> >> (5) CryptoPkg: Upgrade OpenSSL to 1.1.1b >> Update OpenSSL submodule to OpenSSL_1_1_1b >> OpenSSL_1_1_1b(50eaac9f3337667259de725451f201e784599687) >> >> OpenSSL doesn't implement some rand_pool function for UEFI. >> Use EFI_RNG_PROTOCOL to generate random for entropy. >> If EFI_RNG_PROTOCOL is not avaliable, fall back to performance >> counter, but we not sure about the amount of randomness it >> provides. >> >> (6) CryptoPkg/BaseCryptLib: Make HMAC_CTX size backward compatible >> >> Note: Will be remove next update. >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1792 >> Ref: https://github.com/openssl/openssl/pull/4338 >> >> >> Cc: Jian J Wang >> Cc: Ting Ye > > I'm withdrawing from reviewing or testing this series. To be clear, the reason I abandoned reviewing / testing this series is not due to the use of TimerLib as entropy source, in patch #5. I addressed that separately, stating that I wouldn't review patch #5, only regression-test it. The reason I intend to leave upcoming reviews & testing of this series as a whole to others is that I've found a number of mistakes in relation to the development workflow. And, it's exhausting for me to repeat all the same guidelines, when I had documented them in the wiki [*]. At the same time, I realize that it may be difficult for a new edk2 contributor to adhere to everything described in [*] -- especially given that [*] is not an official edk2 document, just something that I personally distilled from experience. In other words, my insisting on [*] in many repeated emails is exhausting for both new contributors, and myself as a reviewer. Which is why I thought I'd save us both some busywork, by withdrawing from this series. If you'd like me to look over this series again, then a v4 will be necessary, just in order to remedy the following workflow-level problems. (Afterwards, a v5 may be necessary for further technical fixes.) (1) Some of your patches are authored by "Xiaoyu Lu", some others by "Xiaoyu lu" (lower case). This messes up the shortlog in the blurb (and other statistics collected from the git log); you are represented as two different people. Please pick *one* email address (name included), and stick with that. Rebase the series, and use git commit --amend --author=... for fixing up the authorship on the patches that need it. Make sure your Signed-off-by follows suit in the commit messages. (2) The series is hard to apply for local testing, with "git am", due to patch #5 modifying both CRLF and LF files. That's not necessarily a problem with the patch itself, but the norm has been, for non-trivial patch series, to push a topic branch to a personal repo, and to reference that repo & branch in the blurb. It permits easy fetching and easy commenting both. This wasn't done in v2, and I struggled with "git am". It hasn't been done in v3 either. Please do it in v4 and further versions. (3) In my review of the v1 series, I requested that the CC_FLAGS changes, for the "OpensslLib.inf" and "OpensslLibCrypto.inf" files, be isolated to their own standalone patch. In v2, this was nicely addressed in patch #4, and I gave my R-b. In v3 however, you squashed a totally unrelated -- but at the series level, necessary -- change into the same patch (namely "sys/syscall.h"). While that improved the end result of the series for sure, it *negated* the v2 improvement in the specific patch. In my v2 review, this was how I asked for "sys/syscall.h": So please include a patch in the v3 series that adds "CryptoPkg/Library/Include/sys/syscall.h" like suggested above. *Separate patch*. If you disagree with my request, that's 100% part of the process, but then please respond under the request, rather than dumping an entire new version of the series on me that does not comply with my request. (4) In version 3, you failed to pick up my Reviewed-by tags that I had given for v2 1/6 and v2 6/6. In more technical terms, this means that you should have run "git rebase -i", selected the "reword" action for patches v2 1/6 and v2 6/6, and appended -- using the clipboard -- my R-b tags, from my review emails, to the commit messages. This is documented in detail, in [*] (contributor step 28). (Referring to the previous bullet, you also failed to pick up my R-b for patch v2 4/6. However, ultimately, that was the correct action for that patch, given that you modified the patch in v3. If a patch is modified significantly in a revision, then review tags garnered earlier should be dropped, so that reviewers check the patch again.) (5) Jian had some questions still open under v2 5/6, when you posted v3. The questions were addressed to me. Sometimes I cannot answer on the next day, and yes, there was a weekend to. If you think a reviewer missed something, please wait one or two business days, and ping them off-list or on-list, before sending the next version. If there isn't enough time left to catch the upcoming stable tag with this work, then we should postpone this work to the next stable tag. [*] https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers Thanks Laszlo