From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.387.1605295788005973532 for ; Fri, 13 Nov 2020 11:29:48 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RTGhY/Yb; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605295787; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=j/EqRllS/8F7IEq3+7CuAagyjGWgNeiRZZBIS6IOres=; b=RTGhY/Ybtx1UUyjI5Gl/ah9lgqUzcpd4ZkA+dpFFlA0FMs4LiT2kVWnP5RDFZFM4QJK9/p 3u/goeeT7o+xcKhVsshZff8VNiZLx+6aB+M1K7ON7smRnFC8fNbVaRXLmNusgYjtewedxx O/MOedMWaQ/eMpSMF1chN2T9iE5v370= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-481-kxigRCnINveC47KvMrAbGg-1; Fri, 13 Nov 2020 14:29:42 -0500 X-MC-Unique: kxigRCnINveC47KvMrAbGg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6A2241074645; Fri, 13 Nov 2020 19:29:40 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-113.ams2.redhat.com [10.36.112.113]) by smtp.corp.redhat.com (Postfix) with ESMTP id E69B45B4CA; Fri, 13 Nov 2020 19:29:37 +0000 (UTC) Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro 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)" References: <20201111131927.21323-1-abner.chang@hpe.com> <20201111131927.21323-2-abner.chang@hpe.com> <9357a533-108e-b4c3-6aa8-3f9dcea0846c@redhat.com> From: "Laszlo Ersek" Message-ID: <2f7a4ac6-bf2e-0425-4efa-731121b84dab@redhat.com> Date: Fri, 13 Nov 2020 20:29:36 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> >> >> >> >> >