public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Laszlo Ersek <lersek@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>,
	 "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally
Date: Fri, 14 Dec 2018 15:03:18 +0100	[thread overview]
Message-ID: <CAKv+Gu-3hMWWv1Wvkdz021dXX7k1XX5JKMshhkmfBOXKk8GRBw@mail.gmail.com> (raw)
In-Reply-To: <546ee80d-28da-12fa-341b-8850b61aa682@redhat.com>

On Fri, 14 Dec 2018 at 14:56, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
> > ---
> >  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 <lersek@redhat.com>
>

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 :-)


  reply	other threads:[~2018-12-14 14:03 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-14 11:22 [PATCH] ArmVirtPkg/ArmVirt.dsc.inc: define TcpIoLib resolution unconditionally Ard Biesheuvel
2018-12-14 13:56 ` Laszlo Ersek
2018-12-14 14:03   ` Ard Biesheuvel [this message]
2018-12-17  1:50   ` Fu, Siyuan
2018-12-17  3:06     ` Gao, Liming
2018-12-17  3:22       ` Fu, Siyuan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKv+Gu-3hMWWv1Wvkdz021dXX7k1XX5JKMshhkmfBOXKk8GRBw@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox