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

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

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, November 19, 2020 12:53 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/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
> 
> 
> > 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).
Yes. just keep it simple for Network stack.

> 
> Thanks,
> Laszlo