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

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

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