From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mx.groups.io with SMTP id smtpd.web08.34102.1605531172899823480 for ; Mon, 16 Nov 2020 04:52:52 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.126, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: fxZtqfsiMcf6BZpKKiApJ16b+yFkbLv5UgsAhxBlnUvVnY+6CmJGM9bwJ17Q+yA2vjycopSfo2 TiZzgt1rkfAQ== X-IronPort-AV: E=McAfee;i="6000,8403,9806"; a="158516175" X-IronPort-AV: E=Sophos;i="5.77,482,1596524400"; d="scan'208";a="158516175" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Nov 2020 04:52:50 -0800 IronPort-SDR: M1nLA244s6UMWOMrOvc8bo49nJWUeZQjj8mVyiFEBpptag14l92Ir8gAUr8cjrAeDzUhyJZcBR 4QoEFuGgAisA== X-IronPort-AV: E=Sophos;i="5.77,482,1596524400"; d="scan'208";a="543592545" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.20.28]) ([10.213.20.28]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Nov 2020 04:52:48 -0800 Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro To: devel@edk2.groups.io, lersek@redhat.com, "Chang, Abner (HPS SW/FW Technologist)" 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> <2f7a4ac6-bf2e-0425-4efa-731121b84dab@redhat.com> From: "Maciej Rabeda" Message-ID: <4166fb4d-e7f9-7d51-89a4-f590f9749915@linux.intel.com> Date: Mon, 16 Nov 2020 13:52:46 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.4.1 MIME-Version: 1.0 In-Reply-To: <2f7a4ac6-bf2e-0425-4efa-731121b84dab@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl Hi Abner, Agreeing with Laszlo - please pay attention to our comments. We spend some time going through the code to write those - for our mutual benefit. Please submit v2 with corrections addressing Laszlo's comments. Thanks, Maciej On 13-Nov-20 20:29, Laszlo Ersek wrote: > On 11/12/20 01:53, Chang, Abner (HPS SW/FW Technologist) wrote: >> Ok Laszlo, I think you told me once before. :) >> >> BTW, do you have comment on this patch because you had ever put some opinions on BZ. > I think you must have missed my points (2) through (6). > > Thanks > Laszlo > >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >>> Laszlo Ersek >>> Sent: Thursday, November 12, 2020 5:22 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 >>> >>> On 11/11/20 14:19, Abner Chang wrote: >>>> BZ:2917 >>>> >>>> Add NETWORK_HTTP_ENABLE macro and separate HttpDxe and >>>> HttpUtilitiesDxe drivers from HTTP_NETWORK_HTTP_BOOT_ENABLE macro. >>>> >>>> Signed-off-by: Abner Chang >>>> Cc: Maciej Rabeda >>>> Cc: Jiaxin Wu >>>> Cc: Siyuan Fu >>>> Cc: Laszlo Ersek >>>> Cc: Nickle Wang >>>> Cc: Peter O'Hanley >>>> --- >>>> NetworkPkg/Network.fdf.inc | 5 ++++- >>>> NetworkPkg/NetworkComponents.dsc.inc | 5 ++++- >>>> NetworkPkg/NetworkDefines.dsc.inc | 9 +++++++++ >>>> 3 files changed, 17 insertions(+), 2 deletions(-) >>> (1) meta comment -- Abner, I've noticed that you keep placing expressions >>> related to the feature or bugfix at hand in the bracketed subject prefix. For >>> example, in the current case, it's "NETWORK_HTTP_ENABLE": >>> >>> [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add >>> NETWORK_HTTP_ENABLE macro >>> ^^^^^^^^^^^^^^^^^^^ >>> >>> Please stop doing this. It is incredibly distracting. The subject prefix should >>> contain the following elements: >>> >>> (a) If the patch is not for the main "edk2" repository, then the repository >>> (project) identifier. For example "edk2-wiki", "edk2-InfSpecification", "edk2- >>> platforms", and so on. >>> >>> (b) Either the word PATCH or the word RFC. >>> >>> (c) A version identifier. "v1" is usually not specified (except when the >>> submitter already expects having to send a v2). Sometimes the version >>> identifier takes the form of "v2 RESEND", when it's an identical repost of v2, >>> being reposted only because some people failed to receive v2 originally. >>> >>> (d) the patch number within a series (zero standing for the blurb, and >>> altogether omitted when the series consists of a single patch). >>> >>> In other words, everything we put in the subject prefix is *routing >>> information*. It's not *content*. >>> >>> Please stop putting content in the subject prefix. >>> >>> >>>> diff --git a/NetworkPkg/Network.fdf.inc b/NetworkPkg/Network.fdf.inc >>>> index 803a0d64fd..8a662ad1de 100644 >>>> --- a/NetworkPkg/Network.fdf.inc >>>> +++ b/NetworkPkg/Network.fdf.inc >>>> @@ -46,10 +46,13 @@ >>>> INF NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf >>>> !endif >>>> >>>> - !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE >>>> + !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR >>>> + ($(NETWORK_HTTP_ENABLE) == TRUE) >>>> INF NetworkPkg/DnsDxe/DnsDxe.inf >>>> INF NetworkPkg/HttpDxe/HttpDxe.inf >>>> INF NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf >>>> + !endif >>>> + >>>> + !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE >>>> INF NetworkPkg/HttpBootDxe/HttpBootDxe.inf >>>> !endif >>>> >>>> diff --git a/NetworkPkg/NetworkComponents.dsc.inc >>>> b/NetworkPkg/NetworkComponents.dsc.inc >>>> index 40cb8ee18e..21cb62082f 100644 >>>> --- a/NetworkPkg/NetworkComponents.dsc.inc >>>> +++ b/NetworkPkg/NetworkComponents.dsc.inc >>>> @@ -48,10 +48,13 @@ >>>> NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf >>>> !endif >>>> >>>> - !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE >>>> + !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR >>>> + ($(NETWORK_HTTP_ENABLE) == TRUE) >>>> NetworkPkg/DnsDxe/DnsDxe.inf >>>> NetworkPkg/HttpDxe/HttpDxe.inf >>>> NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf >>>> + !endif >>>> + >>>> + !if $(NETWORK_HTTP_BOOT_ENABLE) == TRUE >>>> NetworkPkg/HttpBootDxe/HttpBootDxe.inf >>>> !endif >>>> >>>> diff --git a/NetworkPkg/NetworkDefines.dsc.inc >>>> b/NetworkPkg/NetworkDefines.dsc.inc >>>> index a442d1b157..6f274582a8 100644 >>>> --- a/NetworkPkg/NetworkDefines.dsc.inc >>>> +++ b/NetworkPkg/NetworkDefines.dsc.inc >>>> @@ -15,12 +15,14 @@ >>>> # DEFINE NETWORK_IP4_ENABLE = TRUE >>>> # DEFINE NETWORK_IP6_ENABLE = TRUE >>>> # DEFINE NETWORK_TLS_ENABLE = TRUE >>>> +# DEFINE NETWORK_HTTP_ENABLE = TRUE >>>> # DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE >>> (2) I disagree; the default value for NETWORK_HTTP_ENABLE should be >>> FALSE. >>> >>> Existent platforms that consume "NetworkPkg/NetworkDefines.dsc.inc", or >>> the higher level "Network.dsc.inc", fall in one of the following categories: >>> >>> - They don't specify NETWORK_HTTP_BOOT_ENABLE at all. As a result, they >>> get the full HTTP stack. >>> >>> - They set NETWORK_HTTP_BOOT_ENABLE explicitly to TRUE. As a result, >>> they get the full HTTP stack. >>> >>> - They set NETWORK_HTTP_BOOT_ENABLE explicitly to FALSE. As a result, >>> they get *none* of the full HTTP stack. They don't get a *subset* of the >>> HTTP stack -- they get *none* of it. >>> >>> The last bullet explains why the NETWORK_HTTP_ENABLE default should be >>> FALSE. >>> >>> >>> The new scenario should only be active if a platform explicitly sets *both* >>> NETWORK_HTTP_ENABLE=TRUE *and* >>> NETWORK_HTTP_BOOT_ENABLE=FALSE. >>> >>> >>>> # DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE >>>> # DEFINE NETWORK_ISCSI_ENABLE = TRUE >>>> # DEFINE NETWORK_VLAN_ENABLE = TRUE >>>> # >>>> # Copyright (c) 2019, Intel Corporation. All rights reserved.
>>>> +# (C) Copyright 2020 Hewlett Packard Enterprise Development LP
>>>> # >>>> # SPDX-License-Identifier: BSD-2-Clause-Patent >>>> # >>>> @@ -73,6 +75,13 @@ >>>> DEFINE NETWORK_TLS_ENABLE = TRUE >>>> !endif >>>> >>>> +!ifndef NETWORK_HTTP_ENABLE >>>> + # >>>> + # This flag is to enable or disable HTTP(S) feature. >>>> + # >>> (3) The documentation here must explain that NETWORK_HTTP_ENABLE is >>> ignored (it has no effect whatsoever) if NETWORK_HTTP_BOOT_ENABLE is >>> TRUE. >>> >>>> + DEFINE NETWORK_HTTP_ENABLE = TRUE >>> (4) See (2), this should be FALSE. >>> >>>> +!endif >>>> + >>>> !ifndef NETWORK_HTTP_BOOT_ENABLE >>>> # >>>> # This flag is to enable or disable HTTP(S) boot feature. >>>> >>> (5) The following condition should be updated too: >>> >>> !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) AND >>> ($(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 is set to TRUE!" >>> !endif >>> >>> That's because NETWORK_ALLOW_HTTP_CONNECTIONS controls >>> "PcdAllowHttpConnections", and this PCD is consumed by HttpDxe as well, >>> not just HttpBootDxe. >>> >>> Thus, the subcondition >>> >>> ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) >>> >>> should be replaced by >>> >>> (($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR >>> ($(NETWORK_HTTP_ENABLE) == TRUE)) >>> >>> because that condition describes whether HttpDxe will be included. >>> >>> Specifically, the following build config should be rejected: >>> >>> NETWORK_HTTP_BOOT_ENABLE = FALSE (manually set) >>> NETWORK_HTTP_ENABLE = TRUE (manually set) >>> NETWORK_TLS_ENABLE = FALSE (manually set) >>> NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE (default) >>> >>> >>> (6) Please update the !error message as well, accordingly: >>> >>> ... if NETWORK_HTTP_BOOT_ENABLE or NETWORK_HTTP_ENABLE is set to >>> TRUE >>> >>> Thanks, >>> Laszlo >>> >>> >>> >>> >>> > > > > >