From: "Abner Chang" <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 [thread overview]
Message-ID: <AT5PR8401MB113709049775E0DFAF913F9AFFE30@AT5PR8401MB1137.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <9357a533-108e-b4c3-6aa8-3f9dcea0846c@redhat.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
>
>
>
>
>
next prev parent reply other threads:[~2020-11-16 2:33 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AT5PR8401MB113709049775E0DFAF913F9AFFE30@AT5PR8401MB1137.NAMPRD84.PROD.OUTLOOK.COM \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox