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

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

[Abner Chang]

BZ:2917

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>

Abner Chang (1):
  NetworkPkg: Add NETWORK_HTTP_ENABLE macro

 NetworkPkg/Network.fdf.inc           | 5 ++++-
 NetworkPkg/NetworkComponents.dsc.inc | 5 ++++-
 NetworkPkg/NetworkDefines.dsc.inc    | 9 +++++++++
 3 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.17.1

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

[Abner Chang]

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

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
 #   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.
+  #
+  DEFINE NETWORK_HTTP_ENABLE = TRUE
+!endif
+
 !ifndef NETWORK_HTTP_BOOT_ENABLE
   #
   # This flag is to enable or disable HTTP(S) boot feature.
-- 
2.17.1

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

[Laszlo Ersek]

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

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

[Abner Chang]

Ok Laszlo,  I think you told me once before. :)

BTW, do you have comment on this patch because you had ever put some opinions on BZ.

Thanks
Abner

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, November 12, 2020 5:22 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> 
> 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
> 
> 
> 
> 
> 

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

[Laszlo Ersek]

On 11/12/20 01:53, Chang, Abner (HPS SW/FW Technologist) wrote:
> Ok Laszlo,  I think you told me once before. :)
> 
> BTW, do you have comment on this patch because you had ever put some opinions on BZ.

I think you must have missed my points (2) through (6).

Thanks
Laszlo

>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, November 12, 2020 5:22 AM
>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
>> devel@edk2.groups.io
>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
>> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
>> <peter.ohanley@hpe.com>
>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
>>
>> 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
>>
>>
>>
>> 
>>
> 

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

[Abner Chang]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, November 12, 2020 5:22 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> 
> 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.
I don’t quite get the last scenario. If they set NETWORK_HTTP_BOOT_ENABLE to FALSE then NETWORK_HTTP_ENABLE is still TURE for other HTTP use cases.
They can set NETWORK_HTTP_ENABLE to FALSE explicitly if they don’t even need HTTP.

I think those network definitions were designed as default ON.

> 
> 
> 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)
What if the use case just requires HTTP Utility Protocol to produce and consume HTTP headers but not sending out through HTTP protocol, via in-band channel instead.  I don’t think we have to put the restrictions this one.

> 
> 
> (6) Please update the !error message as well, accordingly:
> 
>   ... if NETWORK_HTTP_BOOT_ENABLE or NETWORK_HTTP_ENABLE is set to
> TRUE

We come back to other issues once we clarify (2).
Abner

> 
> Thanks,
> Laszlo
> 
> 
> 
> 
> 

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

[Maciej Rabeda]

Hi Abner,

Agreeing with Laszlo - please pay attention to our comments. We spend 
some time going through the code to write those - for our mutual benefit.
Please submit v2 with corrections addressing Laszlo's comments.

Thanks,
Maciej

On 13-Nov-20 20:29, Laszlo Ersek wrote:
> On 11/12/20 01:53, Chang, Abner (HPS SW/FW Technologist) wrote:
>> Ok Laszlo,  I think you told me once before. :)
>>
>> BTW, do you have comment on this patch because you had ever put some opinions on BZ.
> I think you must have missed my points (2) through (6).
>
> Thanks
> Laszlo
>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Thursday, November 12, 2020 5:22 AM
>>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
>>> devel@edk2.groups.io
>>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
>>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
>>> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
>>> <peter.ohanley@hpe.com>
>>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
>>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
>>>
>>> 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
>>>
>>>
>>>
>>>
>>>
>
>
> 
>
>

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

[Abner Chang]

[-- Attachment #1: Type: text/plain, Size: 9955 bytes --]

Hi Maciej,
I had two feedbacks to (2) and (5) in another thread, email attached.

Thanks
Abner

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Maciej Rabeda
> Sent: Monday, November 16, 2020 8:53 PM
> To: devel@edk2.groups.io; lersek@redhat.com; Chang, Abner (HPS SW/FW
> Technologist) <abner.chang@hpe.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
> Wang, Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> 
> Hi Abner,
> 
> Agreeing with Laszlo - please pay attention to our comments. We spend
> some time going through the code to write those - for our mutual benefit.
> Please submit v2 with corrections addressing Laszlo's comments.
> 
> Thanks,
> Maciej
> 
> On 13-Nov-20 20:29, Laszlo Ersek wrote:
> > On 11/12/20 01:53, Chang, Abner (HPS SW/FW Technologist) wrote:
> >> Ok Laszlo,  I think you told me once before. :)
> >>
> >> BTW, do you have comment on this patch because you had ever put some
> opinions on BZ.
> > I think you must have missed my points (2) through (6).
> >
> > Thanks
> > Laszlo
> >
> >>> -----Original Message-----
> >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> >>> Of Laszlo Ersek
> >>> Sent: Thursday, November 12, 2020 5:22 AM
> >>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> >>> devel@edk2.groups.io
> >>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> >>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle
> >>> (HPS
> >>> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> >>> <peter.ohanley@hpe.com>
> >>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> >>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> >>>
> >>> 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
> >>>
> >>>
> >>>
> >>>
> >>>
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 


[-- Attachment #2: Type: message/rfc822, Size: 14086 bytes --]

From: "Chang, Abner (HPS SW/FW Technologist)" <abner.chang@hpe.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>, "lersek@redhat.com" <lersek@redhat.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>, Jiaxin Wu <jiaxin.wu@intel.com>, Siyuan Fu <siyuan.fu@intel.com>, "Wang, Nickle (HPS SW)" <nickle.wang@hpe.com>, "O'Hanley, Peter (EXL)" <peter.ohanley@hpe.com>
Subject: RE: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro
Date: Mon, 16 Nov 2020 02:32:57 +0000
Message-ID: <AT5PR8401MB113709049775E0DFAF913F9AFFE30@AT5PR8401MB1137.NAMPRD84.PROD.OUTLOOK.COM>



> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, November 12, 2020 5:22 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
>
> 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.
I don’t quite get the last scenario. If they set NETWORK_HTTP_BOOT_ENABLE to FALSE then NETWORK_HTTP_ENABLE is still TURE for other HTTP use cases.
They can set NETWORK_HTTP_ENABLE to FALSE explicitly if they don’t even need HTTP.

I think those network definitions were designed as default ON.

>
>
> 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)
What if the use case just requires HTTP Utility Protocol to produce and consume HTTP headers but not sending out through HTTP protocol, via in-band channel instead.  I don’t think we have to put the restrictions this one.

>
>
> (6) Please update the !error message as well, accordingly:
>
>   ... if NETWORK_HTTP_BOOT_ENABLE or NETWORK_HTTP_ENABLE is set to
> TRUE

We come back to other issues once we clarify (2).
Abner

>
> Thanks,
> Laszlo
>
>
>
> -=-=-=-=-=-=-=-=-=-=-=-
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#67310):
> INVALID URI REMOVED
> 3A__edk2.groups.io_g_devel_message_67310&d=DwICaQ&c=C5b8zRQO1mi
> GmBeVZ2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=
> Ib1CTqs8b8Q8p88F7LG1uLdIQYnG9BIfCuJHCDkTgYc&s=QyXVzsrJKZAr18UQX6
> JIj6aZzvgMBOwtA4L5wIax2zE&e=
> Mute This Topic: INVALID URI REMOVED
> 3A__groups.io_mt_78182924_1772629&d=DwICaQ&c=C5b8zRQO1miGmBeV
> Z2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=Ib1CTq
> s8b8Q8p88F7LG1uLdIQYnG9BIfCuJHCDkTgYc&s=o-
> 1cFCK77J8Gj63hJCgd2a63V-5ev-iOD1pap20Q6tw&e=
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: INVALID URI REMOVED
> 3A__edk2.groups.io_g_devel_unsub&d=DwICaQ&c=C5b8zRQO1miGmBeVZ
> 2LFWg&r=_SN6FZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=Ib1CTqs
> 8b8Q8p88F7LG1uLdIQYnG9BIfCuJHCDkTgYc&s=c-
> xilcssfThv299NcEoxuL2siiZ0wvOx_27uGB6N2go&e=  [abner.chang@hpe.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>

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

[Laszlo Ersek]

Hi Abner,

On 11/16/20 03:32, Chang, Abner (HPS SW/FW Technologist) wrote:
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, November 12, 2020 5:22 AM
>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
>> devel@edk2.groups.io
>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
>> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
>> <peter.ohanley@hpe.com>
>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
>>
>> On 11/11/20 14:19, Abner Chang wrote:

[...]

>>> 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.
> I don’t quite get the last scenario. If they set NETWORK_HTTP_BOOT_ENABLE to FALSE then NETWORK_HTTP_ENABLE is still TURE for other HTTP use cases.
> They can set NETWORK_HTTP_ENABLE to FALSE explicitly if they don’t even need HTTP.
> 

When we implement a new feature (or just a change) in core edk2, it's
best to keep platforms (especially out-of-tree platforms) completely
unaffected, *if* this is possible to do without major difficulties.

Consider a platform that currently contains, in its DSC file,

  DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE

and then an !include directive for "NetworkPkg/NetworkDefines.dsc.inc".

That particular platform permits its users to build it with the HTTP
Boot feature, but the default setting for that platform is to not
include HTTP Boot -- or in fact *ANY* of the HTTP infrastructure elements.

If your patch were applied as-is to core edk2, then such platforms would
suddenly start including *some* parts of the HTTP infrastructure by
default, without ever having asked for it.

> I think those network definitions were designed as default ON.

That default (from NetworkPkg) applies *unless* the platform sets a
different default. Consider "NetworkPkg/NetworkDefines.dsc.inc":

!ifndef NETWORK_HTTP_BOOT_ENABLE
  #
  # This flag is to enable or disable HTTP(S) boot feature.
  #
  DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE
!endif

The platform is welcome to set NETWORK_HTTP_BOOT_ENABLE to FALSE as the
platform default, before including "NetworkPkg/NetworkDefines.dsc.inc".
That doesn't mean the platform *never* wants to enable HTTP boot (users
may still override NETWORK_HTTP_BOOT_ENABLE to TRUE on the build command
line), it just means that the default for the platform is FALSE.

And your patch would break that, as it would sneak some unwanted
components into the default build of such platforms.

It's not good practice to say "they can still set the 'new flag' for
undoing the damage we're causing them". There should be *no damage* to
them in the first place (if that's possible to implement). The burden
should be on platforms that want to consume the new feature to
explicitly ask for the new feature.


> 
>>
>>
>> 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)
> What if the use case just requires HTTP Utility Protocol to produce and consume HTTP headers but not sending out through HTTP protocol, via in-band channel instead.  I don’t think we have to put the restrictions this one.

I don't understand, I'm sorry.

The above combination of flags means that:
- the user wants the HTTP base infrastructure, without HTTP Boot,
- *plus* they disable TLS,
- *plus* they forbid the HTTP base infrastructure for making (or even
accepting) plaintext HTTP connections.

The described scenario requires the HTTP base infrastructure to always
create (or enforce) encrypted (HTTPS) connections, *but* without relying
on TLS.

But that's a requirement that's impossible to satisfy: in edk2, the
*only* means for making or accepting HTTPS connections (regardless of
whether they are for Boot purposes or otherwise) is TLS.

So in this scenario, the build should abort at once.

Laszlo

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

[Abner Chang]

Hi Laszlo,

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, November 18, 2020 1:09 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> 
> Hi Abner,
> 
> On 11/16/20 03:32, Chang, Abner (HPS SW/FW Technologist) wrote:
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Thursday, November 12, 2020 5:22 AM
> >> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> >> devel@edk2.groups.io
> >> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> >> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle
> >> (HPS
> >> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> >> <peter.ohanley@hpe.com>
> >> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> >> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> >>
> >> On 11/11/20 14:19, Abner Chang wrote:
> 
> [...]
> 
> >>> 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.
> > I don’t quite get the last scenario. If they set
> NETWORK_HTTP_BOOT_ENABLE to FALSE then NETWORK_HTTP_ENABLE is
> still TURE for other HTTP use cases.
> > They can set NETWORK_HTTP_ENABLE to FALSE explicitly if they don’t even
> need HTTP.
> >
> 
> When we implement a new feature (or just a change) in core edk2, it's best
> to keep platforms (especially out-of-tree platforms) completely unaffected,
> *if* this is possible to do without major difficulties.

I got your point now, NETWORK_HTTP_BOOT_ENABLE default set to FALSE makes sense.

> 
> Consider a platform that currently contains, in its DSC file,
> 
>   DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE
> 
> and then an !include directive for "NetworkPkg/NetworkDefines.dsc.inc".
> 
> That particular platform permits its users to build it with the HTTP Boot
> feature, but the default setting for that platform is to not include HTTP Boot -
> - or in fact *ANY* of the HTTP infrastructure elements.
> 
> If your patch were applied as-is to core edk2, then such platforms would
> suddenly start including *some* parts of the HTTP infrastructure by default,
> without ever having asked for it.
> 
> > I think those network definitions were designed as default ON.
> 
> That default (from NetworkPkg) applies *unless* the platform sets a
> different default. Consider "NetworkPkg/NetworkDefines.dsc.inc":
> 
> !ifndef NETWORK_HTTP_BOOT_ENABLE
>   #
>   # This flag is to enable or disable HTTP(S) boot feature.
>   #
>   DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE !endif
> 
> The platform is welcome to set NETWORK_HTTP_BOOT_ENABLE to FALSE as
> the platform default, before including
> "NetworkPkg/NetworkDefines.dsc.inc".
> That doesn't mean the platform *never* wants to enable HTTP boot (users
> may still override NETWORK_HTTP_BOOT_ENABLE to TRUE on the build
> command line), it just means that the default for the platform is FALSE.
> 
> And your patch would break that, as it would sneak some unwanted
> components into the default build of such platforms.
> 
> It's not good practice to say "they can still set the 'new flag' for undoing the
> damage we're causing them". There should be *no damage* to them in the
> first place (if that's possible to implement). The burden should be on
> platforms that want to consume the new feature to explicitly ask for the new
> feature.
> 
> 
> >
> >>
> >>
> >> 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
Yes.

> >> TRUE.
> >>
> >>> +  DEFINE NETWORK_HTTP_ENABLE = TRUE
> >>
> >> (4) See (2), this should be FALSE.
Yes

> >>
> >>> +!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.
Yes, but I will break down this long condition check into two blocks for the readability.

!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

> >>
> >> 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)
> > What if the use case just requires HTTP Utility Protocol to produce and
> consume HTTP headers but not sending out through HTTP protocol, via in-
> band channel instead.  I don’t think we have to put the restrictions this one.
> 
> I don't understand, I'm sorry.
> 
> The above combination of flags means that:
> - the user wants the HTTP base infrastructure, without HTTP Boot,
> - *plus* they disable TLS,
> - *plus* they forbid the HTTP base infrastructure for making (or even
> accepting) plaintext HTTP connections.
> 
> The described scenario requires the HTTP base infrastructure to always
> create (or enforce) encrypted (HTTPS) connections, *but* without relying on
> TLS.
> 
> But that's a requirement that's impossible to satisfy: in edk2, the
> *only* means for making or accepting HTTPS connections (regardless of
> whether they are for Boot purposes or otherwise) is TLS.
> 
> So in this scenario, the build should abort at once.
Ah, your point is that’s not reasonable to have both NETWORK_TLS_ENABLE and NETWORK_ALLOW_HTTP_CONNECTIONS set to FALSE on edk2. But this seems to me this scenario falls into the change for (5),

!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
Or you would like to have the different error messages?



My point was the macro would be looked as below after this patch,
!if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
    NetworkPkg/DnsDxe/DnsDxe.inf
    NetworkPkg/HttpDxe/HttpDxe.inf
    NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
!endif

There could be the use case (with NETWORK_HTTP_BOOT_ENABLE = FALSE) that the implementation just requires HttpUtilitiesDxe to manipulate the HTTP headers but not really transferring HTTP request/response through HTTP protocol. HTTP payloads may transferred over the in-band transport.
Above condition check breaks this use case. This also reminds me that the change for (5) also breaks this use case. 

How about we just leave it unchanged for (5), only handle NETWORK_HTTP_BOOT_ENABLE case. Or, we create another macro for NETWORK_HTTP_UTILITY but it seems to me too much.
However, I would like to have another macro for NetworkPkg/DnsDxe/DnsDxe.inf because not all of HTTP connections requires DNS.
How do you think?

Abner

> 
> Laszlo
> 
> 
> 
> 
> 

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

[Laszlo Ersek]

On 11/18/20 04:14, Chang, Abner (HPS SW/FW Technologist) wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, November 18, 2020 1:09 AM
>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
>> devel@edk2.groups.io
>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
>> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
>> <peter.ohanley@hpe.com>
>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro

>> When we implement a new feature (or just a change) in core edk2, it's best
>> to keep platforms (especially out-of-tree platforms) completely unaffected,
>> *if* this is possible to do without major difficulties.
> 
> I got your point now, NETWORK_HTTP_BOOT_ENABLE default set to FALSE makes sense.

Thanks. (And I think you meant NETWORK_HTTP_ENABLE.)


> Yes, but I will break down this long condition check into two blocks for the readability.
> 
> !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

Looks good!


> Ah, your point is that’s not reasonable to have both NETWORK_TLS_ENABLE and NETWORK_ALLOW_HTTP_CONNECTIONS set to FALSE on edk2.

Yes.

> But this seems to me this scenario falls into the change for (5),

Sure.

> !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
> Or you would like to have the different error messages?

No, the above is fine.

> My point was the macro would be looked as below after this patch,
> !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
>     NetworkPkg/DnsDxe/DnsDxe.inf
>     NetworkPkg/HttpDxe/HttpDxe.inf
>     NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
> !endif
> 
> There could be the use case (with NETWORK_HTTP_BOOT_ENABLE = FALSE) that the implementation just requires HttpUtilitiesDxe to manipulate the HTTP headers but not really transferring HTTP request/response through HTTP protocol. HTTP payloads may transferred over the in-band transport.

The current patch includes HttpDxe under "NETWORK_HTTP_ENABLE", and
HttpDxe depends on PcdAllowHttpConnections (in the EfiHttpRequest()
function).

This means that NETWORK_HTTP_ENABLE is not independent of
NETWORK_ALLOW_HTTP_CONNECTIONS.

> Above condition check breaks this use case. This also reminds me that the change for (5) also breaks this use case. 
> 
> How about we just leave it unchanged for (5), only handle NETWORK_HTTP_BOOT_ENABLE case. Or, we create another macro for NETWORK_HTTP_UTILITY but it seems to me too much.
> However, I would like to have another macro for NetworkPkg/DnsDxe/DnsDxe.inf because not all of HTTP connections requires DNS.
> How do you think?

A feature test macro such as NETWORK_HTTP_BOOT_ENABLE is only useful if
it groups together several drivers that implement a particular feature
or feature set. In other words, a feature test macro is only useful if
it allows a platform to ignore specific NetworkPkg drivers, and to ask
for a feature (some higher-level functionality) instead.

Conversely, if we have valid platform use cases that depend on
individual drivers in isolation, then introducing macros for those
individual drivers makes no sense. They don't buy platforms any
simplicity, they just complicate the NetworkPkg core. So in such a case,
platforms should include the drivers they desire one by one.

If a platforms wants HTTP header manipulation and nothing else (no TLS,
no HTTP(S) requests, no DNS lookups), then the platform should include
HttpUtilitiesDxe explicitly, and be done with it. That's why the driver
*exists* as a separate entity in the first place.

If you want to accommodate use cases for REST where a platform may or
may not need DnsDxe, plus (independently) the platform may or may not
need HttpDxe, and the only thing the platform certainly needs is
HttpUtilitiesDxe, then I propose *not* introducing any new feature test
macros. If we foresee platforms deciding with *this granularity* about
the REST-related sub-features, then thos platforms should just include
the appropriate drivers from NetworkPkg by name.

That's my opinion anyway -- the NetworkPkg maintainers have not
commented yet (AFAICS).

Thanks,
Laszlo

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

[Maciej Rabeda]

@Laszlo
AFAICS, you are doing just fine with the reviews, should I formalize it? :)
I have sent one response where I have signed up under your points and 
emphasized the problem with patch subject prefix.

@Abner
1. Condition breakdown - I am all for it, it looks good.
2. Granularity - We are simply enabling HTTP layer for Redfish use case 
so it seems reasonable to have a separate flag to enable just that, 
without boot drivers.
I am against removing NETWORK_HTTP_BOOT_ENABLE due to unnecessary impact 
to other packages.

Awaiting v2.

Thanks,
Maciej

On 18-Nov-20 17:53, Laszlo Ersek wrote:
> On 11/18/20 04:14, Chang, Abner (HPS SW/FW Technologist) wrote:
>> Hi Laszlo,
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Wednesday, November 18, 2020 1:09 AM
>>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
>>> devel@edk2.groups.io
>>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
>>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
>>> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
>>> <peter.ohanley@hpe.com>
>>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
>>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
>>> When we implement a new feature (or just a change) in core edk2, it's best
>>> to keep platforms (especially out-of-tree platforms) completely unaffected,
>>> *if* this is possible to do without major difficulties.
>> I got your point now, NETWORK_HTTP_BOOT_ENABLE default set to FALSE makes sense.
> Thanks. (And I think you meant NETWORK_HTTP_ENABLE.)
>
>
>> Yes, but I will break down this long condition check into two blocks for the readability.
>>
>> !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
> Looks good!
>
>
>> Ah, your point is that’s not reasonable to have both NETWORK_TLS_ENABLE and NETWORK_ALLOW_HTTP_CONNECTIONS set to FALSE on edk2.
> Yes.
>
>> But this seems to me this scenario falls into the change for (5),
> Sure.
>
>> !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
>> Or you would like to have the different error messages?
> No, the above is fine.
>
>> My point was the macro would be looked as below after this patch,
>> !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
>>      NetworkPkg/DnsDxe/DnsDxe.inf
>>      NetworkPkg/HttpDxe/HttpDxe.inf
>>      NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
>> !endif
>>
>> There could be the use case (with NETWORK_HTTP_BOOT_ENABLE = FALSE) that the implementation just requires HttpUtilitiesDxe to manipulate the HTTP headers but not really transferring HTTP request/response through HTTP protocol. HTTP payloads may transferred over the in-band transport.
> The current patch includes HttpDxe under "NETWORK_HTTP_ENABLE", and
> HttpDxe depends on PcdAllowHttpConnections (in the EfiHttpRequest()
> function).
>
> This means that NETWORK_HTTP_ENABLE is not independent of
> NETWORK_ALLOW_HTTP_CONNECTIONS.
>
>> Above condition check breaks this use case. This also reminds me that the change for (5) also breaks this use case.
>>
>> How about we just leave it unchanged for (5), only handle NETWORK_HTTP_BOOT_ENABLE case. Or, we create another macro for NETWORK_HTTP_UTILITY but it seems to me too much.
>> However, I would like to have another macro for NetworkPkg/DnsDxe/DnsDxe.inf because not all of HTTP connections requires DNS.
>> How do you think?
> A feature test macro such as NETWORK_HTTP_BOOT_ENABLE is only useful if
> it groups together several drivers that implement a particular feature
> or feature set. In other words, a feature test macro is only useful if
> it allows a platform to ignore specific NetworkPkg drivers, and to ask
> for a feature (some higher-level functionality) instead.
>
> Conversely, if we have valid platform use cases that depend on
> individual drivers in isolation, then introducing macros for those
> individual drivers makes no sense. They don't buy platforms any
> simplicity, they just complicate the NetworkPkg core. So in such a case,
> platforms should include the drivers they desire one by one.
>
> If a platforms wants HTTP header manipulation and nothing else (no TLS,
> no HTTP(S) requests, no DNS lookups), then the platform should include
> HttpUtilitiesDxe explicitly, and be done with it. That's why the driver
> *exists* as a separate entity in the first place.
>
> If you want to accommodate use cases for REST where a platform may or
> may not need DnsDxe, plus (independently) the platform may or may not
> need HttpDxe, and the only thing the platform certainly needs is
> HttpUtilitiesDxe, then I propose *not* introducing any new feature test
> macros. If we foresee platforms deciding with *this granularity* about
> the REST-related sub-features, then thos platforms should just include
> the appropriate drivers from NetworkPkg by name.
>
> That's my opinion anyway -- the NetworkPkg maintainers have not
> commented yet (AFAICS).
>
> Thanks,
> Laszlo
>

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

[Abner Chang]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 19, 2020 12:53 AM
> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> devel@edk2.groups.io
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle (HPS
> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> 
> On 11/18/20 04:14, Chang, Abner (HPS SW/FW Technologist) wrote:
> > Hi Laszlo,
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Wednesday, November 18, 2020 1:09 AM
> >> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> >> devel@edk2.groups.io
> >> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> >> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle
> >> (HPS
> >> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> >> <peter.ohanley@hpe.com>
> >> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> >> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> 
> >> When we implement a new feature (or just a change) in core edk2, it's
> >> best to keep platforms (especially out-of-tree platforms) completely
> >> unaffected,
> >> *if* this is possible to do without major difficulties.
> >
> > I got your point now, NETWORK_HTTP_BOOT_ENABLE default set to FALSE
> makes sense.
> 
> Thanks. (And I think you meant NETWORK_HTTP_ENABLE.)
Yes
> 
> 
> > Yes, but I will break down this long condition check into two blocks for the
> readability.
> >
> > !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
> 
> Looks good!
> 
> 
> > Ah, your point is that’s not reasonable to have both
> NETWORK_TLS_ENABLE and NETWORK_ALLOW_HTTP_CONNECTIONS set to
> FALSE on edk2.
> 
> Yes.
> 
> > But this seems to me this scenario falls into the change for (5),
> 
> Sure.
> 
> > !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
> > Or you would like to have the different error messages?
> 
> No, the above is fine.
> 
> > My point was the macro would be looked as below after this patch, !if
> > ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR
> ($(NETWORK_HTTP_ENABLE) == TRUE)
> >     NetworkPkg/DnsDxe/DnsDxe.inf
> >     NetworkPkg/HttpDxe/HttpDxe.inf
> >     NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
> > !endif
> >
> > There could be the use case (with NETWORK_HTTP_BOOT_ENABLE = FALSE)
> that the implementation just requires HttpUtilitiesDxe to manipulate the
> HTTP headers but not really transferring HTTP request/response through
> HTTP protocol. HTTP payloads may transferred over the in-band transport.
> 
> The current patch includes HttpDxe under "NETWORK_HTTP_ENABLE", and
> HttpDxe depends on PcdAllowHttpConnections (in the EfiHttpRequest()
> function).
> 
> This means that NETWORK_HTTP_ENABLE is not independent of
> NETWORK_ALLOW_HTTP_CONNECTIONS.
> 
> > Above condition check breaks this use case. This also reminds me that the
> change for (5) also breaks this use case.
> >
> > How about we just leave it unchanged for (5), only handle
> NETWORK_HTTP_BOOT_ENABLE case. Or, we create another macro for
> NETWORK_HTTP_UTILITY but it seems to me too much.
> > However, I would like to have another macro for
> NetworkPkg/DnsDxe/DnsDxe.inf because not all of HTTP connections
> requires DNS.
> > How do you think?
> 
> A feature test macro such as NETWORK_HTTP_BOOT_ENABLE is only useful if
> it groups together several drivers that implement a particular feature or
> feature set. In other words, a feature test macro is only useful if it allows a
> platform to ignore specific NetworkPkg drivers, and to ask for a feature
> (some higher-level functionality) instead.
> 
> Conversely, if we have valid platform use cases that depend on individual
> drivers in isolation, then introducing macros for those individual drivers
> makes no sense. They don't buy platforms any simplicity, they just
> complicate the NetworkPkg core. So in such a case, platforms should include
> the drivers they desire one by one.
> 
> If a platforms wants HTTP header manipulation and nothing else (no TLS, no
> HTTP(S) requests, no DNS lookups), then the platform should include
> HttpUtilitiesDxe explicitly, and be done with it. That's why the driver
> *exists* as a separate entity in the first place.
> 
> If you want to accommodate use cases for REST where a platform may or
> may not need DnsDxe, plus (independently) the platform may or may not
> need HttpDxe, and the only thing the platform certainly needs is
> HttpUtilitiesDxe, then I propose *not* introducing any new feature test
> macros. If we foresee platforms deciding with *this granularity* about the
> REST-related sub-features, then thos platforms should just include the
> appropriate drivers from NetworkPkg by name.
> 
> That's my opinion anyway -- the NetworkPkg maintainers have not
> commented yet (AFAICS).
Yes. just keep it simple for Network stack.

> 
> Thanks,
> Laszlo

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

[Abner Chang]

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Maciej Rabeda
> Sent: Thursday, November 19, 2020 1:11 AM
> To: Laszlo Ersek <lersek@redhat.com>; Chang, Abner (HPS SW/FW
> Technologist) <abner.chang@hpe.com>; devel@edk2.groups.io
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>;
> Wang, Nickle (HPS SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> <peter.ohanley@hpe.com>
> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
> 
> @Laszlo
> AFAICS, you are doing just fine with the reviews, should I formalize it? :) I
> have sent one response where I have signed up under your points and
> emphasized the problem with patch subject prefix.
> 
> @Abner
> 1. Condition breakdown - I am all for it, it looks good.
> 2. Granularity - We are simply enabling HTTP layer for Redfish use case so it
> seems reasonable to have a separate flag to enable just that, without boot
> drivers.
> I am against removing NETWORK_HTTP_BOOT_ENABLE due to unnecessary
> impact to other packages.
No, we won't remove NETWORK_HTTP_BOOT_ENABLE.
> 
> Awaiting v2.
Thanks, v3 is sent (v2 has the different subject which only has "PATCH" in the prefix).

Abner

> 
> Thanks,
> Maciej
> 
> On 18-Nov-20 17:53, Laszlo Ersek wrote:
> > On 11/18/20 04:14, Chang, Abner (HPS SW/FW Technologist) wrote:
> >> Hi Laszlo,
> >>
> >>> -----Original Message-----
> >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> >>> Of Laszlo Ersek
> >>> Sent: Wednesday, November 18, 2020 1:09 AM
> >>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@hpe.com>;
> >>> devel@edk2.groups.io
> >>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu
> >>> <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Wang, Nickle
> >>> (HPS
> >>> SW) <nickle.wang@hpe.com>; O'Hanley, Peter (EXL)
> >>> <peter.ohanley@hpe.com>
> >>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
> >>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro When we implement
> a new
> >>> feature (or just a change) in core edk2, it's best to keep platforms
> >>> (especially out-of-tree platforms) completely unaffected,
> >>> *if* this is possible to do without major difficulties.
> >> I got your point now, NETWORK_HTTP_BOOT_ENABLE default set to
> FALSE makes sense.
> > Thanks. (And I think you meant NETWORK_HTTP_ENABLE.)
> >
> >
> >> Yes, but I will break down this long condition check into two blocks for the
> readability.
> >>
> >> !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
> > Looks good!
> >
> >
> >> Ah, your point is that’s not reasonable to have both
> NETWORK_TLS_ENABLE and NETWORK_ALLOW_HTTP_CONNECTIONS set to
> FALSE on edk2.
> > Yes.
> >
> >> But this seems to me this scenario falls into the change for (5),
> > Sure.
> >
> >> !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
> >> Or you would like to have the different error messages?
> > No, the above is fine.
> >
> >> My point was the macro would be looked as below after this patch, !if
> >> ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR
> ($(NETWORK_HTTP_ENABLE) == TRUE)
> >>      NetworkPkg/DnsDxe/DnsDxe.inf
> >>      NetworkPkg/HttpDxe/HttpDxe.inf
> >>      NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
> >> !endif
> >>
> >> There could be the use case (with NETWORK_HTTP_BOOT_ENABLE =
> FALSE) that the implementation just requires HttpUtilitiesDxe to manipulate
> the HTTP headers but not really transferring HTTP request/response through
> HTTP protocol. HTTP payloads may transferred over the in-band transport.
> > The current patch includes HttpDxe under "NETWORK_HTTP_ENABLE", and
> > HttpDxe depends on PcdAllowHttpConnections (in the EfiHttpRequest()
> > function).
> >
> > This means that NETWORK_HTTP_ENABLE is not independent of
> > NETWORK_ALLOW_HTTP_CONNECTIONS.
> >
> >> Above condition check breaks this use case. This also reminds me that the
> change for (5) also breaks this use case.
> >>
> >> How about we just leave it unchanged for (5), only handle
> NETWORK_HTTP_BOOT_ENABLE case. Or, we create another macro for
> NETWORK_HTTP_UTILITY but it seems to me too much.
> >> However, I would like to have another macro for
> NetworkPkg/DnsDxe/DnsDxe.inf because not all of HTTP connections
> requires DNS.
> >> How do you think?
> > A feature test macro such as NETWORK_HTTP_BOOT_ENABLE is only useful
> > if it groups together several drivers that implement a particular
> > feature or feature set. In other words, a feature test macro is only
> > useful if it allows a platform to ignore specific NetworkPkg drivers,
> > and to ask for a feature (some higher-level functionality) instead.
> >
> > Conversely, if we have valid platform use cases that depend on
> > individual drivers in isolation, then introducing macros for those
> > individual drivers makes no sense. They don't buy platforms any
> > simplicity, they just complicate the NetworkPkg core. So in such a
> > case, platforms should include the drivers they desire one by one.
> >
> > If a platforms wants HTTP header manipulation and nothing else (no
> > TLS, no HTTP(S) requests, no DNS lookups), then the platform should
> > include HttpUtilitiesDxe explicitly, and be done with it. That's why
> > the driver
> > *exists* as a separate entity in the first place.
> >
> > If you want to accommodate use cases for REST where a platform may or
> > may not need DnsDxe, plus (independently) the platform may or may not
> > need HttpDxe, and the only thing the platform certainly needs is
> > HttpUtilitiesDxe, then I propose *not* introducing any new feature
> > test macros. If we foresee platforms deciding with *this granularity*
> > about the REST-related sub-features, then thos platforms should just
> > include the appropriate drivers from NetworkPkg by name.
> >
> > That's my opinion anyway -- the NetworkPkg maintainers have not
> > commented yet (AFAICS).
> >
> > Thanks,
> > Laszlo
> >
> 
> 
> 
> 
>