From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::d44; helo=mail-io1-xd44.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd44.google.com (mail-io1-xd44.google.com [IPv6:2607:f8b0:4864:20::d44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6B33421CAD9B2 for ; Fri, 14 Dec 2018 06:03:31 -0800 (PST) Received: by mail-io1-xd44.google.com with SMTP id t24so4536490ioi.0 for ; Fri, 14 Dec 2018 06:03:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ZANOTrbKKWyIpmdm5iURCueD3yXyr2BH+dajldzrc9c=; b=Y4rZ76V17mxg1h1/fz6k0sDxkQfsYdZptzjTi6YNePi3HN5PDytUJRjDAAdH+V1qPO 6j/6Hl8b8l0assQ5eZI/MdG8Zv/HQFkAjZKv556vUA1ila8chZhxHeLaHC75oOIO4piM 6iyfkYrjiHTPeQgBA00NWxeIirT7v7GHDzRcY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ZANOTrbKKWyIpmdm5iURCueD3yXyr2BH+dajldzrc9c=; b=S+gVbMbkctu3rziaOAv+ahzaQ7Js1tCHI76tkqkfnqsovQcYgi+ZNm7ZEUgrQXz7f8 Kh7DfsdZeuMWXQ2uXgM1a4GCNDdqPJm+Y4eZyhUj7NI1gdIR37zWBFK/XY/mEtCIePeo bFTHifj55zGUS9tSdi5p9TL+WSpzr3KhsvxLg/LRj9/y2M+80usL2qNWmBqg5TSifdFW 4BGFEfrUA8B2y2sP333r8/5fGsyhEMT5WFjKd0UbB/Mz32Y76t2oP4Val3Cem8Ne+RLW rRuOdjT8utS+qhH0ODNU1B5K9FXXo539mSMtPtP2XeT2GWoI3iTp0dlGOch6oqLBRGi+ CKiQ== X-Gm-Message-State: AA+aEWZ+u334obZofalrxHeGMdyHEbnJ6QJKGeEDoMmbkSvzy1FxgNw+ SeNPxG33BwA6+tdy8MnJ4XirF5jsKefB6M1bOJP7Wg== X-Google-Smtp-Source: AFSGD/WZDQAyrQUUgUcFVRyh6JX/sn8Iaajgz/OSBj+tvV9UN4Xp8S3y0KCZ67hccZyf1mhWc5HEPZjfplxe1X96s9U= X-Received: by 2002:a5e:cb0b:: with SMTP id p11mr2725037iom.60.1544796209562; Fri, 14 Dec 2018 06:03:29 -0800 (PST) MIME-Version: 1.0 References: <20181214112253.10372-1-ard.biesheuvel@linaro.org> <546ee80d-28da-12fa-341b-8850b61aa682@redhat.com> In-Reply-To: <546ee80d-28da-12fa-341b-8850b61aa682@redhat.com> From: Ard Biesheuvel Date: Fri, 14 Dec 2018 15:03:18 +0100 Message-ID: To: Laszlo Ersek Cc: Siyuan Fu , "edk2-devel@lists.01.org" , Julien Grall 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 14:03:31 -0000 Content-Type: text/plain; charset="UTF-8" On Fri, 14 Dec 2018 at 14:56, Laszlo Ersek wrote: > > 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 > Thanks. Apologies for not looking more carefully whether the feedback had in fact been incorporated before giving my R-b Pushed as 9a67ba261fe9..a9ff32909b47 > 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. > I assumed [incorrectly, I suppose] that people actually build test platforms that they modify, but apparently not :-)