From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C1FBC740032 for ; Wed, 15 Nov 2023 12:01:54 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=cijyVuuGVDWwNIxbuCG1uj2gLk3uLm0qHUGMLwan/hM=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:CC:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700049713; v=1; b=T+mAV7UgKqLvDbdvGVTfLzFgU54xh+SZPhwDFmI5su66lQ2pD2btV6c+TO8fLrQlvBrAAlf+ mQSm6i9+sJsBNFTmgJUzUWhDwZ/xiPRCh43XS9tWJ3OU+3IprOTD9l208ItPjAPg1jcYAV/K1WZ Mt6zHj5fyyAuRhuu+hpdhmE4= X-Received: by 127.0.0.2 with SMTP id ADQWYY7687511xqrtWuuPo5v; Wed, 15 Nov 2023 04:01:53 -0800 X-Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web11.11332.1700049712655889399 for ; Wed, 15 Nov 2023 04:01:52 -0800 X-Received: from pps.filterd (m0279868.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 3AFAdwPk032479; Wed, 15 Nov 2023 12:01:49 GMT X-Received: from nasanppmta05.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3ucq5f8wkj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 15 Nov 2023 12:01:49 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA05.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 3AFC1mAJ004714 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 15 Nov 2023 12:01:48 GMT X-Received: from [10.251.43.200] (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.39; Wed, 15 Nov 2023 04:01:46 -0800 Message-ID: <2f8fbb9f-bca8-4771-9178-6f0507ad089e@quicinc.com> Date: Wed, 15 Nov 2023 12:01:43 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow To: , , CC: Abner Chang , Nickle Wang References: <20231114142815.1604-1-igork@ami.com> <3e8c08dc-a665-41f1-8c51-d4e2743bdac1@quicinc.com> From: "Leif Lindholm" In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-GUID: EBjH_aFYGcMTPQlnTJmGyJ33eV2lKWX6 X-Proofpoint-ORIG-GUID: EBjH_aFYGcMTPQlnTJmGyJ33eV2lKWX6 X-MIME-Autoconverted: from 8bit to quoted-printable by mx0a-0031df01.pphosted.com id 3AFAdwPk032479 Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Vc3pT1UdoVQB4EdOqOSkiV1fx7686176AA= Content-Language: en-GB Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=T+mAV7Ug; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=quicinc.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2023-11-14 23:52, Mike Maslenkin wrote: > On Tue, Nov 14, 2023 at 9:57=E2=80=AFPM Igor Kulchytskyy via groups.io > wrote: >> >> Hi Leif, >> Please see my comments below. >> Thank you, >> Igor >> >> >> -----Original Message----- >> From: Leif Lindholm >> Sent: Tuesday, November 14, 2023 12:26 PM >> To: devel@edk2.groups.io; Igor Kulchytskyy >> Cc: Abner Chang ; Nickle Wang >> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishD= iscoverDxe: Optimize the Redfish Discover flow >> >> >> **CAUTION: The e-mail below is from an external source. Please exercise = caution before opening attachments, clicking links, or following guidance.*= * >> >> On 2023-11-14 14:28, Igor Kulchytskyy via groups.io wrote: >>> Filter out the network interfaces which are not supported by >>> Redfish Host Interface. >>> >>> Cc: Abner Chang >>> Cc: Nickle Wang >>> Signed-off-by: Igor Kulchytskyy >>> --- >>> RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c | 163 ++++++= ++++++++------ >>> RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h | 6 + >>> 2 files changed, 120 insertions(+), 49 deletions(-) >>> >>> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c b/Redfi= shPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c >>> index 0f622e05a9..ae83cd3c97 100644 >>> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c >>> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverDxe.c >> >> >>> @@ -1601,10 +1681,22 @@ BuildupNetworkInterface ( >>> EFI_REDFISH_DISCOVER_REST_EX_INSTANCE_INTERNAL *RestExInstance; >>> EFI_TPL OldTpl; >>> BOOLEAN NewNetworkInterfa= ceInstalled; >>> + UINT8 IpType; >>> + UINTN ListCount; >>> >>> + ListCount =3D (sizeof (gRequiredProtocol) / sizeo= f (REDFISH_DISCOVER_REQUIRED_PROTOCOL)); >>> NewNetworkInterfaceInstalled =3D FALSE; >>> Index =3D 0; >>> - do { >>> + >>> + // Get IP Type to filter out unnecessary network protocol if possibl= e >>> + IpType =3D GetHiIpProtocolType (); >>> + >>> + for (Index =3D 0; Index < ListCount; Index++) { >>> + // Check IP Type and skip an unnecessary network protocol if does = not match >>> + if (IS_TCP4_MATCH (IpType) || IS_TCP6_MATCH (IpType)) { >> >> The logic of these macros is inverted compared to their names, though. >> >> You want this test to read >> if (!IS_TCP4_MATCH (IpType) && !IS_TCP6_MATCH (IpType)) { >> >>> + continue; >>> + } >>> + >>> Status =3D gBS->OpenProtocol ( >>> // Already in list? >>> ControllerHandle, >> >>> diff --git a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h b/= RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h >>> index 01454acc1d..3093eea0d5 100644 >>> --- a/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h >>> +++ b/RedfishPkg/RedfishDiscoverDxe/RedfishDiscoverInternal.h >>> @@ -39,6 +39,12 @@ >>> #define REDFISH_DISCOVER_VERSION 0x00010000 >>> #define EFI_REDFISH_DISCOVER_NETWORK_INTERFACE_TPL TPL_NOTIFY >>> >>> +#define MAC_COMPARE(ThisNetworkInterface, TargetNetworkInterface) (Co= mpareMem ((VOID *)&ThisNetworkInterface->MacAddress, &TargetNetworkInterfac= e->MacAddress, ThisNetworkInterface->HwAddressSize)) >>> +#define VALID_TCP6(TargetNetworkInterface, ThisNetworkInterface) (Ta= rgetNetworkInterface->IsIpv6 && (ThisNetworkInterface->NetworkProtocolType = =3D=3D ProtocolTypeTcp6)) >>> +#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface) (!T= argetNetworkInterface->IsIpv6 && (ThisNetworkInterface->NetworkProtocolType= =3D=3D ProtocolTypeTcp4)) >>> +#define IS_TCP4_MATCH(IpType) ((g= RequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp4) && (IpType != =3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) >>> +#define IS_TCP6_MATCH(IpType) ((g= RequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp6) && (IpType != =3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) >> >> And these macros to test for =3D=3D, not !=3D >> >> >> Igor: First version tested "=3D=3D", but we agreed that it may not work = if we have a wrong value of IpType. >> >> Otherwise the code reads like it does the opposite of what it does. >> >> (You could also keep the logic and call the macros IS_TCP#_MISMATCH, but >> that feels a bit convoluted.) >> >> Igor: I would prefer to go with IS_TCP#_MISMATCH names. >> >> Regards, >> >> Leif >=20 > Sorry, could I add my 2 cents? Always! > For me all newly added defines looks bad, just because those > implicitly use reference to a global variable > plus local variable state (i.e current cycle index). Fair. But the original test is basically line noise. I will point out that gRequiredProtocol appears to be misnamed to start=20 with. It's only used in this file, so ought to be mRequiredProtocol. However, your point still stands. > Could we rewrite code in a simple and straight forward manner, similar to= : >=20 > if (IpType =3D=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_UNKNOWN) = { > // The protocol type is not specified in SMBIOS table type 42h > return EFI_UNSUPPORTED; > } The test needs to be an allowlist, not a denylist of specific error=20 cases. The input is the SMBIOS table, which could have been corrupted=20 accidentally or intentionally. But yes, rejecting the outright invalid options upfront is a big=20 readability improvement. > for (Index =3D 0; Index < ListCount; Index++) { > if ((gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp4) && > (IpType !=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) { > continue; > } > if ((gRequiredProtocol[Index].ProtocolType =3D=3D ProtocolTypeTcp6) && > (IpType !=3D REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) { > continue; > } > This still looks somewhat like line noise. We could stick it in a static helper function to get=20 BuildupNetworkInterface () more human readable? Regards, Leif > Regards, > Mike. >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111267): https://edk2.groups.io/g/devel/message/111267 Mute This Topic: https://groups.io/mt/102584140/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-