> -----Original Message-----
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, November 29, 2017 6:54 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org
> Cc: Anthony Perard <anthony.per...@citrix.com>; Justen, Jordan L
> <jordan.l.jus...@intel.com>; Ard Biesheuvel <ard.biesheu...@linaro.org>;
> Carsey, Jaben <jaben.car...@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...@intel.com>
> > Cc: Jordan Justen <jordan.l.jus...@intel.com>
> > Reviewed-by: Laszlo Ersek <ler...@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Cc: Anthony Perard <anthony.per...@citrix.com>
> > Cc: Julien Grall <julien.gr...@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-
> a18a7ed37...@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
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
