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

["Abner Chang" <abner.chang@hpe.com>]

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