From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 14D7B203564B0 for ; Wed, 29 Nov 2017 02:49:30 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F156D8553E; Wed, 29 Nov 2017 10:53:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-48.rdu2.redhat.com [10.10.120.48]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3CEDB5D6A3; Wed, 29 Nov 2017 10:53:52 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Anthony Perard , Jordan Justen , Ard Biesheuvel , "Carsey, Jaben" References: <20171129005952.208940-1-ruiyu.ni@intel.com> <20171129005952.208940-7-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <50f9cb65-5947-ce38-ee3f-656986058875@redhat.com> Date: Wed, 29 Nov 2017 11:53:51 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171129005952.208940-7-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Wed, 29 Nov 2017 10:53:54 +0000 (UTC) Subject: Re: [PATCH v3 6/6] OvmfPkg: Add tftp dynamic command X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Nov 2017 10:49:31 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 > Cc: Jordan Justen > Reviewed-by: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Anthony Perard > Cc: Julien Grall > --- > 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 { > + > + gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > + } > ShellPkg/Application/Shell/Shell.inf { > > 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.
> +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> # (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> # > # 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 { > + > + gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > + } > ShellPkg/Application/Shell/Shell.inf { > > 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.
> +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> # (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> # > # 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 { > + > + gEfiShellPkgTokenSpaceGuid.PcdShellLibAutoInitialize|FALSE > + } > ShellPkg/Application/Shell/Shell.inf { > > 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.
> +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> # (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> # > # 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 >