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) > Cc: Jiaxin Wu ; Siyuan Fu ; > Wang, Nickle (HPS SW) ; O'Hanley, Peter (EXL) > > 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) ; > >>> devel@edk2.groups.io > >>> Cc: Maciej Rabeda ; Jiaxin Wu > >>> ; Siyuan Fu ; Wang, Nickle > >>> (HPS > >>> SW) ; O'Hanley, Peter (EXL) > >>> > >>> 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 > >>>> Cc: Maciej Rabeda > >>>> Cc: Jiaxin Wu > >>>> Cc: Siyuan Fu > >>>> Cc: Laszlo Ersek > >>>> Cc: Nickle Wang > >>>> Cc: Peter O'Hanley > >>>> --- > >>>> 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.
> >>>> +# (C) Copyright 2020 Hewlett Packard Enterprise Development LP
> >>>> # > >>>> # 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 > >>> > >>> > >>> > >>> > >>> > > > > > > > > > > > > > > >