Re: [PATCH 3/3] OvmfPkg: Allow HTTP connections if HTTP Boot enabled

[Laszlo Ersek <lersek@redhat.com>]

Jiaxin,

On 01/19/17 06:18, Jiaxin Wu wrote:
> Overwrite the value of PcdAllowHttpConnections to allow HTTP
> connections if HTTP Boot enabled (-D HTTP_BOOT_ENABLE).
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Justen Jordan L <jordan.l.justen@intel.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Kinney Michael D <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 6 +++++-
>  OvmfPkg/OvmfPkgIa32X64.dsc | 6 +++++-
>  OvmfPkg/OvmfPkgX64.dsc     | 6 +++++-
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index e060602..2c38578 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -1,9 +1,9 @@
>  ## @file
>  #  EFI/Framework Open Virtual Machine Firmware (OVMF) platform
>  #
> -#  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
>  #  are licensed and made available under the terms and conditions of the BSD License
>  #  which accompanies this distribution. The full text of the license may be found at
> @@ -394,10 +394,14 @@
>  
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>  
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>  
> +  !if $(HTTP_BOOT_ENABLE) == TRUE
> +    gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
> +  !endif
> +
>    # DEBUG_INIT      0x00000001  // Initialization
>    # DEBUG_WARN      0x00000002  // Warnings
>    # DEBUG_LOAD      0x00000004  // Load events
>    # DEBUG_FS        0x00000008  // EFI File system
>    # DEBUG_POOL      0x00000010  // Alloc & Free (pool)
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 0e24e7a..2760533 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -1,9 +1,9 @@
>  ## @file
>  #  EFI/Framework Open Virtual Machine Firmware (OVMF) platform
>  #
> -#  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
>  #  are licensed and made available under the terms and conditions of the BSD License
>  #  which accompanies this distribution. The full text of the license may be found at
> @@ -440,10 +440,14 @@
>  !ifdef $(SOURCE_DEBUG_ENABLE)
>    gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdDebugLoadImageMethod|0x2
>  !endif
>  
>  [PcdsFixedAtBuild.X64]
> +!if $(HTTP_BOOT_ENABLE) == TRUE
> +  gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
> +!endif
> +
>  !ifndef $(USE_OLD_SHELL)
>    gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
>  !endif
>  
>  !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 108f7d5..56fddc3 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -1,9 +1,9 @@
>  ## @file
>  #  EFI/Framework Open Virtual Machine Firmware (OVMF) platform
>  #
> -#  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
>  #  are licensed and made available under the terms and conditions of the BSD License
>  #  which accompanies this distribution. The full text of the license may be found at
> @@ -399,10 +399,14 @@
>  
>    gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>  
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>  
> +  !if $(HTTP_BOOT_ENABLE) == TRUE
> +    gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
> +  !endif
> +
>    # DEBUG_INIT      0x00000001  // Initialization
>    # DEBUG_WARN      0x00000002  // Warnings
>    # DEBUG_LOAD      0x00000004  // Load events
>    # DEBUG_FS        0x00000008  // EFI File system
>    # DEBUG_POOL      0x00000010  // Alloc & Free (pool)
> 

thank you for the patch; it looks good. However, for aesthetic and
consistency reasons, I'd like to request an update.

The placement of the new setting is entirely right in the
"OvmfPkgIa32X64.dsc" file. However, the Ia32 and X64 DSC files don't
follow that placement. Therefore I would like to request the following
two changes, for both the Ia32 and X64 DSC files:

- please move the new setting just above the "!ifndef $(USE_OLD_SHELL)"
part, where "PcdShellFile" is set,

- please un-indent the new setting to column zero. (Well, the "!if" goes
to column zero, the actual setting goes to column two.)

The idea is that all three DSC files should remain pair-wise comparable
with the "diff" utility; any differences displayed by "diff" should be
related to *genuine* Ia32 <-> Ia32X64 <-> X64 differences.

For the next version, I think you can carry forward the following two
tags from Gary (see his email elsewhere in this thread):

Reviewed-by: Gary Lin <glin@suse.com>
Tested-by: Gary Lin <glin@suse.com>

(I'm just repeating them here so you can easily cut n' paste them to the
commit message.)

Thank you!
Laszlo