* [NETWORK_HTTP_ENABLE PATCH 0/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro @ 2020-11-11 13:19 Abner Chang 2020-11-11 13:19 ` [NETWORK_HTTP_ENABLE PATCH 1/1] " Abner Chang 0 siblings, 1 reply; 14+ messages in thread From: Abner Chang @ 2020-11-11 13:19 UTC (permalink / raw) To: devel Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Laszlo Ersek, Nickle Wang, Peter O'Hanley 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-11 13:19 [NETWORK_HTTP_ENABLE PATCH 0/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro Abner Chang @ 2020-11-11 13:19 ` Abner Chang 2020-11-11 21:21 ` Laszlo Ersek 0 siblings, 1 reply; 14+ messages in thread From: Abner Chang @ 2020-11-11 13:19 UTC (permalink / raw) To: devel Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Laszlo Ersek, Nickle Wang, Peter O'Hanley 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-11 13:19 ` [NETWORK_HTTP_ENABLE PATCH 1/1] " Abner Chang @ 2020-11-11 21:21 ` Laszlo Ersek 2020-11-12 0:53 ` [edk2-devel] " Abner Chang 2020-11-16 2:32 ` Abner Chang 0 siblings, 2 replies; 14+ messages in thread From: Laszlo Ersek @ 2020-11-11 21:21 UTC (permalink / raw) To: Abner Chang, devel Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Nickle Wang, Peter O'Hanley 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-11 21:21 ` Laszlo Ersek @ 2020-11-12 0:53 ` Abner Chang 2020-11-13 19:29 ` Laszlo Ersek 2020-11-16 2:32 ` Abner Chang 1 sibling, 1 reply; 14+ messages in thread From: Abner Chang @ 2020-11-12 0:53 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) 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 > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-12 0:53 ` [edk2-devel] " Abner Chang @ 2020-11-13 19:29 ` Laszlo Ersek 2020-11-16 12:52 ` Maciej Rabeda 0 siblings, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2020-11-13 19:29 UTC (permalink / raw) To: Chang, Abner (HPS SW/FW Technologist), devel@edk2.groups.io Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) 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 >> >> >> >> >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-13 19:29 ` Laszlo Ersek @ 2020-11-16 12:52 ` Maciej Rabeda 2020-11-17 0:58 ` Abner Chang 0 siblings, 1 reply; 14+ messages in thread From: Maciej Rabeda @ 2020-11-16 12:52 UTC (permalink / raw) To: devel, lersek, Chang, Abner (HPS SW/FW Technologist) Cc: Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) 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 >>> >>> >>> >>> >>> > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-16 12:52 ` Maciej Rabeda @ 2020-11-17 0:58 ` Abner Chang 0 siblings, 0 replies; 14+ messages in thread From: Abner Chang @ 2020-11-17 0:58 UTC (permalink / raw) To: devel@edk2.groups.io, maciej.rabeda@linux.intel.com, lersek@redhat.com Cc: Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) [-- 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] > -=-=-=-=-=-=-=-=-=-=-=- > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-11 21:21 ` Laszlo Ersek 2020-11-12 0:53 ` [edk2-devel] " Abner Chang @ 2020-11-16 2:32 ` Abner Chang 2020-11-17 17:08 ` Laszlo Ersek 1 sibling, 1 reply; 14+ messages in thread From: Abner Chang @ 2020-11-16 2:32 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) > -----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 > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-16 2:32 ` Abner Chang @ 2020-11-17 17:08 ` Laszlo Ersek 2020-11-18 3:14 ` Abner Chang 0 siblings, 1 reply; 14+ messages in thread From: Laszlo Ersek @ 2020-11-17 17:08 UTC (permalink / raw) To: Chang, Abner (HPS SW/FW Technologist), devel@edk2.groups.io Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-17 17:08 ` Laszlo Ersek @ 2020-11-18 3:14 ` Abner Chang 2020-11-18 16:53 ` Laszlo Ersek 0 siblings, 1 reply; 14+ messages in thread From: Abner Chang @ 2020-11-18 3:14 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) 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 > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-18 3:14 ` Abner Chang @ 2020-11-18 16:53 ` Laszlo Ersek 2020-11-18 17:11 ` Maciej Rabeda 2020-11-19 2:14 ` Abner Chang 0 siblings, 2 replies; 14+ messages in thread From: Laszlo Ersek @ 2020-11-18 16:53 UTC (permalink / raw) To: Chang, Abner (HPS SW/FW Technologist), devel@edk2.groups.io Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-18 16:53 ` Laszlo Ersek @ 2020-11-18 17:11 ` Maciej Rabeda 2020-11-19 2:48 ` Abner Chang 2020-11-19 2:14 ` Abner Chang 1 sibling, 1 reply; 14+ messages in thread From: Maciej Rabeda @ 2020-11-18 17:11 UTC (permalink / raw) To: Laszlo Ersek, Chang, Abner (HPS SW/FW Technologist), devel@edk2.groups.io Cc: Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) @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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-18 17:11 ` Maciej Rabeda @ 2020-11-19 2:48 ` Abner Chang 0 siblings, 0 replies; 14+ messages in thread From: Abner Chang @ 2020-11-19 2:48 UTC (permalink / raw) To: devel@edk2.groups.io, maciej.rabeda@linux.intel.com, Laszlo Ersek Cc: Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) > -----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 > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 2020-11-18 16:53 ` Laszlo Ersek 2020-11-18 17:11 ` Maciej Rabeda @ 2020-11-19 2:14 ` Abner Chang 1 sibling, 0 replies; 14+ messages in thread From: Abner Chang @ 2020-11-19 2:14 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io Cc: Maciej Rabeda, Jiaxin Wu, Siyuan Fu, Wang, Nickle (HPS SW), O'Hanley, Peter (EXL) > -----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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-11-19 2:48 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-11-11 13:19 [NETWORK_HTTP_ENABLE PATCH 0/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro Abner Chang 2020-11-11 13:19 ` [NETWORK_HTTP_ENABLE PATCH 1/1] " Abner Chang 2020-11-11 21:21 ` Laszlo Ersek 2020-11-12 0:53 ` [edk2-devel] " Abner Chang 2020-11-13 19:29 ` Laszlo Ersek 2020-11-16 12:52 ` Maciej Rabeda 2020-11-17 0:58 ` Abner Chang 2020-11-16 2:32 ` Abner Chang 2020-11-17 17:08 ` Laszlo Ersek 2020-11-18 3:14 ` Abner Chang 2020-11-18 16:53 ` Laszlo Ersek 2020-11-18 17:11 ` Maciej Rabeda 2020-11-19 2:48 ` Abner Chang 2020-11-19 2:14 ` Abner Chang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox