From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 79453211A43BC for ; Fri, 14 Dec 2018 05:56:08 -0800 (PST) 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 E7E4146263; Fri, 14 Dec 2018 13:56:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-122-102.rdu2.redhat.com [10.10.122.102]) by smtp.corp.redhat.com (Postfix) with ESMTP id D6F37672E4; Fri, 14 Dec 2018 13:56:06 +0000 (UTC) To: Ard Biesheuvel , siyuan.fu@intel.com Cc: edk2-devel@lists.01.org, julien.grall@linaro.org References: <20181214112253.10372-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: <546ee80d-28da-12fa-341b-8850b61aa682@redhat.com> Date: Fri, 14 Dec 2018 14:56:00 +0100 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: <20181214112253.10372-1-ard.biesheuvel@linaro.org> 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.29]); Fri, 14 Dec 2018 13:56:08 +0000 (UTC) Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 X-List-Received-Date: Fri, 14 Dec 2018 13:56:08 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/14/18 12:22, Ard Biesheuvel wrote: > Commit 9a67ba261fe9 ("ArmVirtPkg: Replace obsoleted network drivers > from platform DSC/FDF") failed to take into account that the now > unconditionally included IScsiDxe.inf from NetworkPkg requires a > resolution for TcpIoLib. I don't understand why this happened. (a) I warned *precisely* about this issue, when I reviewed the v2 version of said commit. See bullet (5) in the following message: http://mid.mail-archive.com/cdd81f4c-1bdc-8bae-63a9-58eb4eb2afbd@redhat.com (b) What's more, my comments for the v3 version were summarily ignored as well. See bullet (2) in: http://mid.mail-archive.com/91d253ae-9d0c-e28b-1bda-1be98cee4340@redhat.com And now, the BaseCryptLib, OpensslLib and IntrinsicLib resolutions for [LibraryClasses.common.UEFI_DRIVER] have been added to "ArmVirtPkg/ArmVirtQemuKernel.dsc", despite their being redundant and my having pointed out that fact. Worse, "ArmVirtQemuKernel.dsc" uses "OpensslLib.inf", while "OpensslLibCrypto.inf" from "ArmVirt.dsc.inc" is perfectly sufficient. Commit 9a67ba261fe9 does not carry my R-b, and that's not a random fact. The v3 patch was *not* ready for being pushed, to my eyes. And I was pretty explicit about that. > Since specifying such a resolution is harmless > for platforms that have no networking enabled, let's just fix things > by dropping the conditionals around it. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/ArmVirt.dsc.inc | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc > index c3549c84d4c6..89c2db074711 100644 > --- a/ArmVirtPkg/ArmVirt.dsc.inc > +++ b/ArmVirtPkg/ArmVirt.dsc.inc > @@ -80,9 +80,7 @@ > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf > IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf > -!if $(NETWORK_IP6_ENABLE) == TRUE > TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf > -!endif > !if $(HTTP_BOOT_ENABLE) == TRUE > HttpLib|MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.inf > !endif > I'm *very strongly* tempted to simply revert 9a67ba261fe9, for blatantly ignoring my explicit requests for updates. However, that would only result in my having to review more (possibly incomplete) iterations of the patch. At least, this incremental fix is in line with my request in (a) -- "we should make the current TcpIoLib class resolution unconditional". Please go ahead and push it. Reviewed-by: Laszlo Ersek I should really file a TianoCore BZ about the wrong / redundant OpensslLib resolution in ArmVirtQemuKernel.dsc too. It's difficult for me to find te motivation for that right now, seeing the disregard for my earlier reviews. Laszlo