From: "Ni, Ruiyu" <ruiyu.ni@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: Anthony Perard <anthony.perard@citrix.com>,
"Justen, Jordan L" <jordan.l.justen@intel.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
"Carsey, Jaben" <jaben.carsey@intel.com>
Subject: Re: [PATCH v3 6/6] OvmfPkg: Add tftp dynamic command
Date: Wed, 29 Nov 2017 13:12:54 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BAD70DD@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <50f9cb65-5947-ce38-ee3f-656986058875@redhat.com>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, November 29, 2017 6:54 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Carsey, Jaben <jaben.carsey@intel.com>
> Subject: Re: [edk2] [PATCH v3 6/6] OvmfPkg: Add tftp dynamic command
>
> Hi Ray,
>
> On 11/29/17 01:59, Ruiyu Ni wrote:
> > The TFTP command was converted from a NULL class library instance
> > to a dynamic shell command in commit 0961002352e9.
> > This patch complements commit f9bc2f876326, which only removed the
> > old library, but didn't add the new dynamic command。
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Julien Grall <julien.grall@linaro.org>
> > ---
> > OvmfPkg/OvmfPkgIa32.dsc | 7 +++++--
> > OvmfPkg/OvmfPkgIa32.fdf | 3 ++-
> > OvmfPkg/OvmfPkgIa32X64.dsc | 7 +++++--
> > OvmfPkg/OvmfPkgIa32X64.fdf | 3 ++-
> > OvmfPkg/OvmfPkgX64.dsc | 7 +++++--
> > OvmfPkg/OvmfPkgX64.fdf | 3 ++-
> > 6 files changed, 21 insertions(+), 9 deletions(-)
>
> after v1 I asked for some changes, and gave my R-b conditional on those
> changes:
>
> http://mid.mail-archive.com/fe382f8c-0c1a-f66c-cbba-
> a18a7ed37aaa@redhat.com
>
> The v2 and v3 versions of the same patch contain code that were (a) not
> present in v1 and (b) not requested by me after v1. So, I could not have
> seen that code or commented on it. I don't think my R-b from v1 should
> have been carried forward to v3, and then used as the basis for pushing
> the v3 patch as 984ba6a46747.
>
> The commit message doesn't say anything about PcdShellLibAutoInitialize,
> and about the removal of the FileHandleLib resolution.
>
> The removal of the FileHandleLib resolution under Shell.inf was
> justified, of course (because it was a duplicate / unnecessary
> resolution), but it should have been broken out to a separate patch, or
> at least mentioned in the commit message.
>
> Also I can find PcdShellLibAutoInitialize in "ShellPkg.dec",
>
> ## This flag is used to control initialization of the shell library
> # This should be FALSE for compiling the shell application itself only.
>
> but quoting it in the commit message is helpful. The general idea is to
> spend a bit more time on patch creation so that review is faster/easier
> (there could be multiple reviewers, and in the future the commit could
> be consulted several times).
It's late because I already pushed the patch.:(
>
> In fact, given how "PcdShellLibAutoInitialize" is now used in the OVMF
> DSC files, I would say that the description in "ShellPkg.dec" is now out
> of date. The documentation should say that the PCD should be FALSE for
> the shell application itself, *plus* DXE_DRIVER modules that implement
> dynamic shell commands.
You are correct. I had another patch to modify the comments in ShellPkg.dec.
>
>
> I'm doing my best to be responsive; please give me a chance to comment
> on OvmfPkg changes that I've never seen or requested.
Ok.
>
> Thanks
> Laszlo
>
> > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> > index 19fa0b4c8d..9d23f8c162 100644
> > --- a/OvmfPkg/OvmfPkgIa32.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32.dsc
> > @@ -193,6 +193,7 @@ [LibraryClasses]
> > TlsLib|CryptoPkg/Library/TlsLib/TlsLib.inf
> > !endif
> >
> > + ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> >
> S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScript
> Lib.inf
> > SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/
> BaseOrderedCollectionRedBlackTreeLib.inf
> > @@ -783,6 +784,10 @@ [Components]
> > !endif
> >
> > !ifndef $(USE_OLD_SHELL)
> > +
> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> {
> > + <PcdsFixedAtBuild>
> > + gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> > + }
> > ShellPkg/Application/Shell/Shell.inf {
> > <LibraryClasses>
> >
> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLi
> b.inf
> > @@ -797,8 +802,6 @@ [Components]
> >
> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2Co
> mmandsLib.inf
> > !endif
> >
> HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.i
> nf
> > - ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> > - FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
> > PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> > # SafeBlockIoLib|ShellPkg/Library/SafeBlockIoLib/SafeBlockIoLib.inf
> > #
> SafeOpenProtocolLib|ShellPkg/Library/SafeOpenProtocolLib/SafeOpenProtocol
> Lib.inf
> > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
> > index 06a439f8cb..ba980834d7 100644
> > --- a/OvmfPkg/OvmfPkgIa32.fdf
> > +++ b/OvmfPkg/OvmfPkgIa32.fdf
> > @@ -1,7 +1,7 @@
> > ## @file
> > # Open Virtual Machine Firmware: FDF
> > #
> > -# Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> > # (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> > #
> > # This program and the accompanying materials
> > @@ -285,6 +285,7 @@ [FV.DXEFV]
> > INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
> >
> > !ifndef $(USE_OLD_SHELL)
> > +INF
> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> > INF ShellPkg/Application/Shell/Shell.inf
> > !else
> > INF RuleOverride = BINARY EdkShellBinPkg/FullShell/FullShell.inf
> > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> > index e1555dbfa8..a9c667fed8 100644
> > --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> > @@ -198,6 +198,7 @@ [LibraryClasses]
> > TlsLib|CryptoPkg/Library/TlsLib/TlsLib.inf
> > !endif
> >
> > + ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> >
> S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScript
> Lib.inf
> > SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/
> BaseOrderedCollectionRedBlackTreeLib.inf
> > @@ -792,6 +793,10 @@ [Components.X64]
> > !endif
> >
> > !ifndef $(USE_OLD_SHELL)
> > +
> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> {
> > + <PcdsFixedAtBuild>
> > + gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> > + }
> > ShellPkg/Application/Shell/Shell.inf {
> > <LibraryClasses>
> >
> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLi
> b.inf
> > @@ -806,8 +811,6 @@ [Components.X64]
> >
> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2Co
> mmandsLib.inf
> > !endif
> >
> HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.i
> nf
> > - ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> > - FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
> > PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> > # SafeBlockIoLib|ShellPkg/Library/SafeBlockIoLib/SafeBlockIoLib.inf
> > #
> SafeOpenProtocolLib|ShellPkg/Library/SafeOpenProtocolLib/SafeOpenProtocol
> Lib.inf
> > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
> > index ced4c5639f..72ac82e76b 100644
> > --- a/OvmfPkg/OvmfPkgIa32X64.fdf
> > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
> > @@ -1,7 +1,7 @@
> > ## @file
> > # Open Virtual Machine Firmware: FDF
> > #
> > -# Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> > # (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> > #
> > # This program and the accompanying materials
> > @@ -286,6 +286,7 @@ [FV.DXEFV]
> > INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
> >
> > !ifndef $(USE_OLD_SHELL)
> > +INF
> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> > INF ShellPkg/Application/Shell/Shell.inf
> > !else
> > INF RuleOverride = BINARY USE = X64 EdkShellBinPkg/FullShell/FullShell.inf
> > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> > index 83d63e55d7..abf570512a 100644
> > --- a/OvmfPkg/OvmfPkgX64.dsc
> > +++ b/OvmfPkg/OvmfPkgX64.dsc
> > @@ -198,6 +198,7 @@ [LibraryClasses]
> > TlsLib|CryptoPkg/Library/TlsLib/TlsLib.inf
> > !endif
> >
> > + ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> >
> S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScript
> Lib.inf
> > SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >
> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/
> BaseOrderedCollectionRedBlackTreeLib.inf
> > @@ -790,6 +791,10 @@ [Components]
> > !endif
> >
> > !ifndef $(USE_OLD_SHELL)
> > +
> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> {
> > + <PcdsFixedAtBuild>
> > + gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE
> > + }
> > ShellPkg/Application/Shell/Shell.inf {
> > <LibraryClasses>
> >
> ShellCommandLib|ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLi
> b.inf
> > @@ -804,8 +809,6 @@ [Components]
> >
> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2Co
> mmandsLib.inf
> > !endif
> >
> HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.i
> nf
> > - ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
> > - FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
> > PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
> > # SafeBlockIoLib|ShellPkg/Library/SafeBlockIoLib/SafeBlockIoLib.inf
> > #
> SafeOpenProtocolLib|ShellPkg/Library/SafeOpenProtocolLib/SafeOpenProtocol
> Lib.inf
> > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
> > index 62dd58f6e4..2fc17810eb 100644
> > --- a/OvmfPkg/OvmfPkgX64.fdf
> > +++ b/OvmfPkg/OvmfPkgX64.fdf
> > @@ -1,7 +1,7 @@
> > ## @file
> > # Open Virtual Machine Firmware: FDF
> > #
> > -# Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> > +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> > # (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> > #
> > # This program and the accompanying materials
> > @@ -286,6 +286,7 @@ [FV.DXEFV]
> > INF MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf
> >
> > !ifndef $(USE_OLD_SHELL)
> > +INF
> ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> > INF ShellPkg/Application/Shell/Shell.inf
> > !else
> > INF RuleOverride = BINARY EdkShellBinPkg/FullShell/FullShell.inf
> >
prev parent reply other threads:[~2017-11-29 13:08 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-29 0:59 [PATCH v3 0/6] Fix build failure due to tftp/dp library removal Ruiyu Ni
2017-11-29 0:59 ` [PATCH v3 1/6] ShellPkg/tftp: Correct file comments header of Tftp.uni Ruiyu Ni
2017-11-29 0:59 ` [PATCH v3 2/6] EmulatorPkg: Fix build failure due to Tftp library removal Ruiyu Ni
2017-11-29 0:59 ` [PATCH v3 3/6] ArmVirtPkg: " Ruiyu Ni
2017-11-29 9:36 ` Ni, Ruiyu
2017-11-29 9:44 ` Ard Biesheuvel
2017-11-29 9:57 ` Ni, Ruiyu
2017-11-29 9:59 ` Ard Biesheuvel
2017-11-29 10:03 ` Ni, Ruiyu
2017-11-29 10:05 ` Ard Biesheuvel
2017-11-29 11:06 ` Laszlo Ersek
2017-11-29 0:59 ` [PATCH v3 4/6] BeagleBoardPkg: " Ruiyu Ni
2017-11-29 7:57 ` Ard Biesheuvel
2017-11-29 8:28 ` Ni, Ruiyu
2017-11-29 8:40 ` Ard Biesheuvel
2017-11-29 0:59 ` [PATCH v3 5/6] CorebootPayloadPkg: Fix build failure due to Tftp/Dp " Ruiyu Ni
2017-11-29 1:51 ` You, Benjamin
2017-11-29 0:59 ` [PATCH v3 6/6] OvmfPkg: Add tftp dynamic command Ruiyu Ni
2017-11-29 10:53 ` Laszlo Ersek
2017-11-29 13:12 ` Ni, Ruiyu [this message]
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=734D49CCEBEEF84792F5B80ED585239D5BAD70DD@SHSMSX104.ccr.corp.intel.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