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

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

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