Re: [edk2-devel] [PATCH v3] NetworkPkg: Add NETWORK_HTTP_ENABLE macro

["Laszlo Ersek" <lersek@redhat.com>]

On 11/19/20 02:58, Abner Chang wrote:
> BZ:2917
> 
> Add NETWORK_HTTP_ENABLE macro and separate HttpDxe
> and HttpUtilitiesDxe drivers from
> HTTP_NETWORK_HTTP_BOOT_ENABLE macro.
> 
> Current NETWORK_HTTP_BOOT_ENABLE macro is defined to enable HTTP
> boot feature in POST, this macro is not only enabling HTTP Boot
> related modules but also enabling other generic HTTP modules
> such as HttpDxe, HttpUtilitiesDxe and DnsDxe.
> These HTTP base drivers would not be only used by HTTP boot
> when we introduce the use case of Redfish implementation over
> HTTP to edk2.
> We should have a dedicate macro to enable generic HTTP functions
> on Network stack and additionally provide NETWORK_HTTP_BOOT_ENABLE
> for HTTP boot functionality for the use case that platform doesn't
> require HTTP boot.
> 
> Signed-off-by: Abner Chang <abner.chang@hpe.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Nickle Wang <nickle.wang@hpe.com>
> Cc: Peter O'Hanley <peter.ohanley@hpe.com>
> ---
>  NetworkPkg/Network.fdf.inc           |  5 ++++-
>  NetworkPkg/NetworkComponents.dsc.inc |  5 ++++-
>  NetworkPkg/NetworkDefines.dsc.inc    | 19 +++++++++++++++++--
>  NetworkPkg/NetworkPkg.ci.yaml        |  2 ++
>  4 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/NetworkPkg/Network.fdf.inc b/NetworkPkg/Network.fdf.inc
> index 803a0d64fd..8a662ad1de 100644
> --- a/NetworkPkg/Network.fdf.inc
> +++ b/NetworkPkg/Network.fdf.inc
> @@ -46,10 +46,13 @@
>      INF  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
>    !endif
>  
> -  !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE
> +  !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
>      INF  NetworkPkg/DnsDxe/DnsDxe.inf
>      INF  NetworkPkg/HttpDxe/HttpDxe.inf
>      INF  NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
> +  !endif
> +
> +  !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE
>      INF  NetworkPkg/HttpBootDxe/HttpBootDxe.inf
>    !endif
>  
> diff --git a/NetworkPkg/NetworkComponents.dsc.inc b/NetworkPkg/NetworkComponents.dsc.inc
> index 40cb8ee18e..21cb62082f 100644
> --- a/NetworkPkg/NetworkComponents.dsc.inc
> +++ b/NetworkPkg/NetworkComponents.dsc.inc
> @@ -48,10 +48,13 @@
>      NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf
>    !endif
>  
> -  !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE
> +  !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
>      NetworkPkg/DnsDxe/DnsDxe.inf
>      NetworkPkg/HttpDxe/HttpDxe.inf
>      NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
> +  !endif
> +
> +  !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE
>      NetworkPkg/HttpBootDxe/HttpBootDxe.inf
>    !endif
>  
> diff --git a/NetworkPkg/NetworkDefines.dsc.inc b/NetworkPkg/NetworkDefines.dsc.inc
> index 18921d81f6..54deb6342a 100644
> --- a/NetworkPkg/NetworkDefines.dsc.inc
> +++ b/NetworkPkg/NetworkDefines.dsc.inc
> @@ -15,12 +15,14 @@
>  #   DEFINE NETWORK_IP4_ENABLE             = TRUE
>  #   DEFINE NETWORK_IP6_ENABLE             = TRUE
>  #   DEFINE NETWORK_TLS_ENABLE             = TRUE
> +#   DEFINE NETWORK_HTTP_ENABLE            = FALSE
>  #   DEFINE NETWORK_HTTP_BOOT_ENABLE       = TRUE
>  #   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
>  #   DEFINE NETWORK_ISCSI_ENABLE           = FALSE
>  #   DEFINE NETWORK_VLAN_ENABLE            = TRUE
>  #
>  # Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +# (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
>  #
>  #    SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -73,10 +75,21 @@
>    DEFINE NETWORK_TLS_ENABLE = TRUE
>  !endif
>  
> +!ifndef NETWORK_HTTP_ENABLE
> +  #
> +  # This flag is to enable or disable HTTP(S) feature.
> +  # The default is set to FALSE to not affecting the existing
> +  # platforms.
> +  # NETWORK_HTTP_ENABLE set to FALSE is not affecting NETWORK_HTTP_BOOT_ENABLE
> +  # when NETWORK_HTTP_BOOT_ENABLE is TRUE.
> +  DEFINE NETWORK_HTTP_ENABLE = FALSE
> +!endif
> +
>  !ifndef NETWORK_HTTP_BOOT_ENABLE
>    #
>    # This flag is to enable or disable HTTP(S) boot feature.
>    #
> +  #

(1) This new empty line does not seem useful.

I think the maintainer that merges this patch can remove the empty line;
no need to post v4 just because of it.

>    DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE
>  !endif
>  
> @@ -112,7 +125,9 @@
>      !error "Must enable at least IP4 or IP6 stack if NETWORK_ENABLE is set to TRUE!"
>    !endif
>  
> -  !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) AND ($(NETWORK_TLS_ENABLE) == FALSE) AND ($(NETWORK_ALLOW_HTTP_CONNECTIONS) == FALSE)
> -    !error "Must enable TLS to support HTTPS, or allow unsecured HTTP connection, if NETWORK_HTTP_BOOT_ENABLE is set to TRUE!"
> +  !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
> +    !if ($(NETWORK_TLS_ENABLE) == FALSE) AND ($(NETWORK_ALLOW_HTTP_CONNECTIONS) == FALSE)
> +      !error "Must enable TLS to support HTTPS, or allow unsecured HTTP connection, if NETWORK_HTTP_BOOT_ENABLE or NETWORK_HTTP_ENABLE is set to TRUE!"
> +    !endif
>    !endif
>  !endif
> diff --git a/NetworkPkg/NetworkPkg.ci.yaml b/NetworkPkg/NetworkPkg.ci.yaml
> index 1a3ab71792..07dc7abd69 100644
> --- a/NetworkPkg/NetworkPkg.ci.yaml
> +++ b/NetworkPkg/NetworkPkg.ci.yaml
> @@ -3,6 +3,7 @@
>  #
>  # Copyright (c) Microsoft Corporation
>  # Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> +# (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>  ##
>  {
> @@ -71,6 +72,7 @@
>          "BLD_*_NETWORK_IP4_ENABLE": "TRUE",
>          "BLD_*_NETWORK_IP6_ENABLE": "TRUE",
>          "BLD_*_NETWORK_TLS_ENABLE": "TRUE",
> +        "BLD_*_NETWORK_HTTP_ENABLE": "FALSE",
>          "BLD_*_NETWORK_HTTP_BOOT_ENABLE": "TRUE",
>          "BLD_*_NETWORK_ISCSI_ENABLE": "TRUE",
>      }
> 

The patch should be merged after the edk2-stable202011 release.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo