Re: [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro

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

On 11/11/20 14:19, Abner Chang wrote:
> BZ:2917
> 
> Add NETWORK_HTTP_ENABLE macro and separate HttpDxe
> and HttpUtilitiesDxe drivers from
> HTTP_NETWORK_HTTP_BOOT_ENABLE macro.
> 
> 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    | 9 +++++++++
>  3 files changed, 17 insertions(+), 2 deletions(-)

(1) meta comment -- Abner, I've noticed that you keep placing expressions related to the feature or bugfix at hand in the bracketed subject prefix. For example, in the current case, it's "NETWORK_HTTP_ENABLE":

  [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro
   ^^^^^^^^^^^^^^^^^^^

Please stop doing this. It is incredibly distracting. The subject prefix should contain the following elements:

(a) If the patch is not for the main "edk2" repository, then the repository (project) identifier. For example "edk2-wiki", "edk2-InfSpecification",  "edk2-platforms", and so on.

(b) Either the word PATCH or the word RFC.

(c) A version identifier. "v1" is usually not specified (except when the submitter already expects having to send a v2). Sometimes the version identifier takes the form of "v2 RESEND", when it's an identical repost of v2, being reposted only because some people failed to receive v2 originally.

(d) the patch number within a series (zero standing for the blurb, and altogether omitted when the series consists of a single patch).

In other words, everything we put in the subject prefix is *routing information*. It's not *content*.

Please stop putting content in the subject prefix.


> 
> 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 a442d1b157..6f274582a8 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            = TRUE
>  #   DEFINE NETWORK_HTTP_BOOT_ENABLE       = TRUE

(2) I disagree; the default value for NETWORK_HTTP_ENABLE should be FALSE.

Existent platforms that consume "NetworkPkg/NetworkDefines.dsc.inc", or the higher level "Network.dsc.inc", fall in one of the following categories:

- They don't specify NETWORK_HTTP_BOOT_ENABLE at all. As a result, they get the full HTTP stack.

- They set NETWORK_HTTP_BOOT_ENABLE explicitly to TRUE. As a result, they get the full HTTP stack.

- They set NETWORK_HTTP_BOOT_ENABLE explicitly to FALSE. As a result, they get *none* of the full HTTP stack. They don't get a *subset* of the HTTP stack -- they get *none* of it.

The last bullet explains why the NETWORK_HTTP_ENABLE default should be FALSE.


The new scenario should only be active if a platform explicitly sets *both* NETWORK_HTTP_ENABLE=TRUE *and* NETWORK_HTTP_BOOT_ENABLE=FALSE.


>  #   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
>  #   DEFINE NETWORK_ISCSI_ENABLE           = TRUE
>  #   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,6 +75,13 @@
>    DEFINE NETWORK_TLS_ENABLE = TRUE
>  !endif
>  
> +!ifndef NETWORK_HTTP_ENABLE
> +  #
> +  # This flag is to enable or disable HTTP(S) feature.
> +  #

(3) The documentation here must explain that NETWORK_HTTP_ENABLE is ignored (it has no effect whatsoever) if NETWORK_HTTP_BOOT_ENABLE is TRUE.

> +  DEFINE NETWORK_HTTP_ENABLE = TRUE

(4) See (2), this should be FALSE.

> +!endif
> +
>  !ifndef NETWORK_HTTP_BOOT_ENABLE
>    #
>    # This flag is to enable or disable HTTP(S) boot feature.
> 

(5) The following condition should be updated too:

  !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!"
  !endif

That's because NETWORK_ALLOW_HTTP_CONNECTIONS controls "PcdAllowHttpConnections", and this PCD is consumed by HttpDxe as well, not just HttpBootDxe.

Thus, the subcondition

  ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE)

should be replaced by

  (($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE))

because that condition describes whether HttpDxe will be included.

Specifically, the following build config should be rejected:

  NETWORK_HTTP_BOOT_ENABLE       = FALSE (manually set)
  NETWORK_HTTP_ENABLE            = TRUE  (manually set)
  NETWORK_TLS_ENABLE             = FALSE (manually set)
  NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE (default)


(6) Please update the !error message as well, accordingly:

  ... if NETWORK_HTTP_BOOT_ENABLE or NETWORK_HTTP_ENABLE is set to TRUE

Thanks,
Laszlo