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

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

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

Awaiting v2.

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
>