From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mx.groups.io with SMTP id smtpd.web09.27540.1605719480210303833 for ; Wed, 18 Nov 2020 09:11:20 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.65, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: DoRm/hQd6xpDXb9R7DfovxMqtVGcZMNCH+NzqPhvAUap2wLMK0a67R9Ok8oAtwXY1sf9J67kcG 1pl6GyYZfNcw== X-IronPort-AV: E=McAfee;i="6000,8403,9809"; a="171248529" X-IronPort-AV: E=Sophos;i="5.77,488,1596524400"; d="scan'208";a="171248529" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Nov 2020 09:11:16 -0800 IronPort-SDR: iYuesxaC+aukqBxORi0zaM9oBAz7evW3aZEz7lzYB+WqEkBWuToWEgQFQsvK/SR2WX/qPzWE/K ZpjpRiTowH4A== X-IronPort-AV: E=Sophos;i="5.77,488,1596524400"; d="scan'208";a="476436827" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.22.66]) ([10.213.22.66]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Nov 2020 09:11:13 -0800 Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro To: Laszlo Ersek , "Chang, Abner (HPS SW/FW Technologist)" , "devel@edk2.groups.io" Cc: Jiaxin Wu , Siyuan Fu , "Wang, Nickle (HPS SW)" , "O'Hanley, Peter (EXL)" References: <20201111131927.21323-1-abner.chang@hpe.com> <20201111131927.21323-2-abner.chang@hpe.com> <9357a533-108e-b4c3-6aa8-3f9dcea0846c@redhat.com> <0a6b860d-5e45-20bb-0f44-42c4ac99f178@redhat.com> From: "Maciej Rabeda" Message-ID: <3b18ba66-3f0a-b6ba-b01f-4230d302b0c9@linux.intel.com> Date: Wed, 18 Nov 2020 18:11:11 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.3 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: pl @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) ; >>> devel@edk2.groups.io >>> Cc: Maciej Rabeda ; Jiaxin Wu >>> ; Siyuan Fu ; Wang, Nickle (HPS >>> SW) ; O'Hanley, Peter (EXL) >>> >>> 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 >