Re: [PATCH v3 6/6] OvmfPkg: Add tftp dynamic command

[Laszlo Ersek <lersek@redhat.com>]

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

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.


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.

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/DxeS3BootScriptLib.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/UefiShellCommandLib.inf
> @@ -797,8 +802,6 @@ [Components]
>        NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.inf
>  !endif
>        HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.inf
> -      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/SafeOpenProtocolLib.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/DxeS3BootScriptLib.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/UefiShellCommandLib.inf
> @@ -806,8 +811,6 @@ [Components.X64]
>        NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.inf
>  !endif
>        HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.inf
> -      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/SafeOpenProtocolLib.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/DxeS3BootScriptLib.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/UefiShellCommandLib.inf
> @@ -804,8 +809,6 @@ [Components]
>        NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.inf
>  !endif
>        HandleParsingLib|ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.inf
> -      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/SafeOpenProtocolLib.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
>