Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: Optimize the Redfish Discover flow

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On 2023-11-14 23:52, Mike Maslenkin wrote:
> On Tue, Nov 14, 2023 at 9:57 PM Igor Kulchytskyy via groups.io
> <igork=ami.com@groups.io> wrote:
>>
>> Hi Leif,
>> Please see my comments below.
>> Thank you,
>> Igor
>>
>>
>> -----Original Message-----
>> From: Leif Lindholm <quic_llindhol@quicinc.com>
>> Sent: Tuesday, November 14, 2023 12:26 PM
>> To: devel@edk2.groups.io; Igor Kulchytskyy <igork@ami.com>
>> Cc: Abner Chang <abner.chang@amd.com>; Nickle Wang <nicklew@nvidia.com>
>> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v5 2/2] RedfishPkg: RedfishDiscoverDxe: 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 <abner.chang@amd.com>
>>> Cc: Nickle Wang <nicklew@nvidia.com>
>>> Signed-off-by: Igor Kulchytskyy <igork@ami.com>
>>> ---
>>>    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/RedfishPkg/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                                          NewNetworkInterfaceInstalled;
>>> +  UINT8                                            IpType;
>>> +  UINTN                                            ListCount;
>>>
>>> +  ListCount                    = (sizeof (gRequiredProtocol) / sizeof (REDFISH_DISCOVER_REQUIRED_PROTOCOL));
>>>      NewNetworkInterfaceInstalled = FALSE;
>>>      Index                        = 0;
>>> -  do {
>>> +
>>> +  // Get IP Type to filter out unnecessary network protocol if possible
>>> +  IpType = GetHiIpProtocolType ();
>>> +
>>> +  for (Index = 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 = 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)  (CompareMem ((VOID *)&ThisNetworkInterface->MacAddress, &TargetNetworkInterface->MacAddress, ThisNetworkInterface->HwAddressSize))
>>> +#define VALID_TCP6(TargetNetworkInterface, ThisNetworkInterface)   (TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface->NetworkProtocolType == ProtocolTypeTcp6))
>>> +#define VALID_TCP4(TargetNetworkInterface, ThisNetworkInterface)   (!TargetNetworkInterface->IsIpv6 && (ThisNetworkInterface->NetworkProtocolType == ProtocolTypeTcp4))
>>> +#define IS_TCP4_MATCH(IpType)                                      ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) && (IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4))
>>> +#define IS_TCP6_MATCH(IpType)                                      ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) && (IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6))
>>
>> And these macros to test for ==, not !=
>>
>>
>> Igor: First version tested "==", 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
> 
> 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 
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:
> 
> if (IpType == 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 
cases. The input is the SMBIOS table, which could have been corrupted 
accidentally or intentionally.

But yes, rejecting the outright invalid options upfront is a big 
readability improvement.

> for (Index = 0; Index < ListCount; Index++) {
>    if ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp4) &&
>       (IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP4)) {
>       continue;
>    }
>    if ((gRequiredProtocol[Index].ProtocolType == ProtocolTypeTcp6) &&
>       (IpType != REDFISH_HOST_INTERFACE_HOST_IP_ADDRESS_FORMAT_IP6)) {
>       continue;
>    }
>    <skip>

This still looks somewhat like line noise.
We could stick it in a static helper function to get 
BuildupNetworkInterface () more human readable?

Regards,

Leif

> Regards,
> Mike.
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-