[edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro
Maciej Rabeda
maciej.rabeda at linux.intel.com
Mon Nov 16 12:52:46 UTC 2020
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 at edk2.groups.io [mailto:devel at 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 at hpe.com>;
>>> devel at edk2.groups.io
>>> Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>; Jiaxin Wu
>>> <jiaxin.wu at intel.com>; Siyuan Fu <siyuan.fu at intel.com>; Wang, Nickle (HPS
>>> SW) <nickle.wang at hpe.com>; O'Hanley, Peter (EXL)
>>> <peter.ohanley at 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 at hpe.com>
>>>> Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>
>>>> Cc: Jiaxin Wu <jiaxin.wu at intel.com>
>>>> Cc: Siyuan Fu <siyuan.fu at intel.com>
>>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>>> Cc: Nickle Wang <nickle.wang at hpe.com>
>>>> Cc: Peter O'Hanley <peter.ohanley at 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
>>>
>>>
>>>
>>>
>>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67607): https://edk2.groups.io/g/devel/message/67607
Mute This Topic: https://groups.io/mt/78182924/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list