Re: [edk2-devel] [edk2-redfish-client][PATCH] RedfishClientPkg/RedfishFeatureUtilityLib: validate string array

["Nickle Wang via groups.io" <nicklew=nvidia.com@groups.io>]

Hi Igor,

Sorry for my delay response. Having three checks in ValidateRedfishStringArrayValues is complicate to me so I took some time to verify it on my system.

> Maybe their functionality can be combined in ValidateRedfishStringArrayValues?
> Some additional an out parameter from that function can indicate If there is no
> change in array.

This is good idea! I update ValidateRedfishStringArrayValues in version 2 patch file here: https://edk2.groups.io/g/devel/message/112025  PR: https://github.com/tianocore/edk2-redfish-client/pull/58 Please help me to check them if you have time.

Thanks,
Nickle

> -----Original Message-----
> From: Igor Kulchytskyy <igork@ami.com>
> Sent: Thursday, November 9, 2023 11:11 PM
> To: Nickle Wang <nicklew@nvidia.com>; devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@amd.com>; Nick Ramirez
> <nramirez@nvidia.com>
> Subject: RE: [EXTERNAL] [edk2-redfish-client][PATCH]
> RedfishClientPkg/RedfishFeatureUtilityLib: validate string array
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Nickle,
> I noticed one typo. See below the text.
> 
> And I have a question regarding newly introduced
> ValidateRedfishStringArrayValues function.
> You use that function to check if value in Redfish string array can be found in HII
> configuration string array.
> And you still call CompareRedfishStringArrayValues function. Both iterate through
> ArrayHead and RedfishValue.Value.StringArray.
> Maybe their functionality can be combined in ValidateRedfishStringArrayValues?
> Some additional an out parameter from that function can indicate If there is no
> change in array.
> What do you think?
> Thank you,
> Igor
> 
> 
> -----Original Message-----
> From: Nickle Wang <nicklew@nvidia.com>
> Sent: Wednesday, November 8, 2023 2:25 AM
> To: devel@edk2.groups.io
> Cc: Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy <igork@ami.com>;
> Nick Ramirez <nramirez@nvidia.com>
> Subject: [EXTERNAL] [edk2-redfish-client][PATCH]
> RedfishClientPkg/RedfishFeatureUtilityLib: validate string array
> 
> 
> **CAUTION: The e-mail below is from an external source. Please exercise
> caution before opening attachments, clicking links, or following guidance.**
> 
> - Add function to validate Redfish request for string array type. There is case that
> user request invalid string array and feature driver can not find corresponding HII
> option.
> - Fix typo.
> 
> Signed-off-by: Nickle Wang <nicklew@nvidia.com>
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Igor Kulchytskyy <igork@ami.com>
> Cc: Nick Ramirez <nramirez@nvidia.com>
> ---
>  .../Library/RedfishFeatureUtilityLib.h        |  56 +++++--
>  .../RedfishFeatureUtilityLib.c                | 156 +++++++++++++-----
>  2 files changed, 154 insertions(+), 58 deletions(-)
> 
> diff --git a/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> b/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> index e2f728b2..440c4b68 100644
> --- a/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> +++ b/RedfishClientPkg/Include/Library/RedfishFeatureUtilityLib.h
> @@ -22,7 +22,7 @@
> 
>    Read redfish resource by given resource URI.
> 
> -  @param[in]  Service       Redfish srvice instacne to make query.
> +  @param[in]  Service       Redfish service instance to make query.
>    @param[in]  ResourceUri   Target resource URI.
>    @param[out] Response      HTTP response from redfish service.
> 
> @@ -47,7 +47,7 @@ GetResourceByUri (
>    @param[out] ArraySignatureClose       String to the close of array signature.
> 
>    @retval     EFI_SUCCESS            Index is found.
> -  @retval     EFI_NOT_FOUND          The non-array configure language string is
> retured.
> +  @retval     EFI_NOT_FOUND          The non-array configure language string is
> returned.
>    @retval     EFI_INVALID_PARAMETER  The format of input ConfigureLang is
> wrong.
>    @retval     Others                 Errors occur.
> 
> @@ -118,10 +118,10 @@ GetNumberOfRedpathNodes (
> 
>    @param[in]  NodeString             The node string to parse.
>    @param[in]  Index                  Index of the node.
> -  @param[out] EndOfNodePtr           Pointer to receive the poitner to
> +  @param[out] EndOfNodePtr           Pointer to receive the pointer to
>                                       the last character of node string.
> 
> -  @retval     EFI_STRING             the begining of the node string.
> +  @retval     EFI_STRING             the beginning of the node string.
> 
>  **/
>  EFI_STRING
> @@ -140,7 +140,7 @@ GetRedpathNodeByIndex (
>    @param[out] Index                 The array index number.
> 
>    @retval     EFI_SUCCESS            Index is found.
> -  @retval     EFI_NOT_FOUND          The non-array configure language string is
> retured.
> +  @retval     EFI_NOT_FOUND          The non-array configure language string is
> returned.
>    @retval     EFI_INVALID_PARAMETER  The format of input ConfigureLang is
> wrong.
>    @retval     Others                 Errors occur.
> 
> @@ -188,7 +188,7 @@ DestroyConfiglanguageList (
> 
>    @param[in]  DestConfigLang        Pointer to the node's configure language
> string.
>                                      The memory pointed by ConfigLang must be allocated
> -                                    through memory allocation interface. Becasue we will
> replace
> +                                    through memory allocation
> + interface. Because we will replace
>                                      the pointer in this function.
>    @param[in]  MaxtLengthConfigLang  The maximum length of ConfigLang.
>    @param[in]  ConfigLangInstance    Pointer to Collection member instance.
> @@ -244,7 +244,7 @@ ApplyFeatureSettingsStringType (
> 
>  /**
> 
> -  Apply property value to UEFI HII database in numric type.
> +  Apply property value to UEFI HII database in numeric type.
> 
>    @param[in]  Schema        Property schema.
>    @param[in]  Version       Property schema version.
> @@ -356,7 +356,7 @@ ApplyFeatureSettingsNumericArrayType (
>    @param[in]  Schema        Property schema.
>    @param[in]  Version       Property schema version.
>    @param[in]  ConfigureLang Configure language refers to this property.
> -  @param[in]  ArrayHead     Head of Redfich CS boolean array value.
> +  @param[in]  ArrayHead     Head of Redfish CS boolean array value.
> 
>    @retval     EFI_SUCCESS     New value is applied successfully.
>    @retval     Others          Errors occur.
> @@ -421,7 +421,7 @@ CreatePayloadToPatchResource (
>    @param[in]    ConfigLang        ConfigLang to save
>    @param[in]    Uri               Redfish Uri to save
> 
> -  @retval  EFI_INVALID_PARAMETR   SystemId is NULL or EMPTY
> +  @retval  EFI_INVALID_PARAMETER   SystemId is NULL or EMPTY
>    @retval  EFI_SUCCESS            Redfish uri is saved
> 
>  **/
> @@ -433,7 +433,7 @@ RedfisSetRedfishUri (
> 
>  /**
> 
> -  Get the property name by given Configure Langauge.
> +  Get the property name by given Configure Language.
> 
>    @param[in]  ResourceUri              URI of root of resource.
>    @param[in]  ConfigureLang            Configure Language string.
> @@ -576,7 +576,7 @@ GetOdataId (
> 
>  /**
> 
> -  Return config language from given URI and prperty name. It's call responsibility
> to release returned buffer.
> +  Return config language from given URI and property name. It's call
> responsibility to release returned buffer.
> 
>    @param[in] Uri            The URI to match
>    @param[in] PropertyName   The property name of resource. This is optional.
> @@ -790,7 +790,7 @@ MatchPropertyWithJsonContext (
> 
>  /**
> 
> -  Create string array and append to arry node in Redfish JSON convert format.
> +  Create string array and append to array node in Redfish JSON convert format.
> 
>    @param[in,out]  Head          The head of string array.
>    @param[in]      StringArray   Input string array.
> @@ -809,7 +809,7 @@ AddRedfishCharArray (
> 
>  /**
> 
> -  Create numeric array and append to arry node in Redfish JSON convert format.
> +  Create numeric array and append to array node in Redfish JSON convert
> format.
> 
>    @param[in,out]  Head           The head of string array.
>    @param[in]      NumericArray   Input numeric array.
> @@ -828,7 +828,7 @@ AddRedfishNumericArray (
> 
>  /**
> 
> -  Create boolean array and append to arry node in Redfish JSON convert format.
> +  Create boolean array and append to array node in Redfish JSON convert format.
> 
>    @param[in,out]  Head           The head of string array.
>    @param[in]      BooleanArray   Input boolean array.
> @@ -866,12 +866,34 @@ CompareRedfishStringArrayValues (
>    IN UINTN                 ArraySize
>    );
> 
> +/**
> +
> +  Check and see if value in Redfish string array can be found in HII
> + configuration string array. This is to see if there is any invalid
> + values from Redfish.
> +
> +  @param[in]  Head          The head of string array.
> +  @param[in]  StringArray   Input string array.
> +  @param[in]  ArraySize     The size of StringArray.
> +
> +  @retval     TRUE          All string in Redfish array are as same as string
> 
> Igor: I think here we should have a plural of "string"
> +  @retval     TRUE          All strings in Redfish array are as same as strings
> 
> 
> +                            in HII configuration array.
> +              FALSE         These two array are not identical.
> +
> +**/
> +BOOLEAN
> +ValidateRedfishStringArrayValues (
> +  IN RedfishCS_char_Array  *Head,
> +  IN CHAR8                 **StringArray,
> +  IN UINTN                 ArraySize
> +  );
> +
>  /**
> 
>    Check and see if value in Redfish numeric array are all the same as the one
>    from HII configuration.
> 
> -  @param[in]  Head          The head of Redfish CS numeraic array.
> +  @param[in]  Head          The head of Redfish CS numeric array.
>    @param[in]  NumericArray  Input numeric array.
>    @param[in]  ArraySize     The size of NumericArray.
> 
> @@ -914,9 +936,9 @@ CompareRedfishBooleanArrayValues (
>    This is just a simple check.
> 
>    @param[in]  RedfishVagueKeyValuePtr     The vague key value sets on Redfish
> service.
> -  @param[in]  RedfishVagueKeyValueNumber  The numebr of vague key value
> sets
> +  @param[in]  RedfishVagueKeyValueNumber  The number of vague key value
> + sets
>    @param[in]  ConfigVagueKeyValuePtr      The vague configuration on platform.
> -  @param[in]  ConfigVagueKeyValueNumber   The numebr of vague key value
> sets
> +  @param[in]  ConfigVagueKeyValueNumber   The number of vague key value
> sets
> 
>    @retval     TRUE          All values are the same.
>                FALSE         There is some difference.
> diff --git
> a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
> b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
> index 13e29902..a3ca1006 100644
> ---
> a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib.c
> +++ b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUt
> +++ ilityLib.c
> @@ -772,61 +772,69 @@ ApplyFeatureSettingsStringArrayType (
>    }
> 
>    //
> -  // If there is no change in array, do nothing
> +  // Check and see if element in request string array can be found in HII string
> array.
>    //
> -  if (!CompareRedfishStringArrayValues (ArrayHead,
> RedfishValue.Value.StringArray, RedfishValue.ArrayCount)) {
> -    //
> -    // Apply settings from redfish
> -    //
> -    DEBUG ((DEBUG_MANAGEABILITY, "%a: %a.%a apply %s for array\n",
> __func__, Schema, Version, ConfigureLang));
> -    FreeArrayTypeRedfishValue (&RedfishValue);
> -
> +  if (ValidateRedfishStringArrayValues (ArrayHead,
> + RedfishValue.Value.StringArray, RedfishValue.ArrayCount)) {
>      //
> -    // Convert array from RedfishCS_char_Array to EDKII_REDFISH_VALUE
> +    // If there is no change in array, do nothing
>      //
> -    RedfishValue.ArrayCount = 0;
> -    Buffer                  = ArrayHead;
> -    while (Buffer != NULL) {
> -      RedfishValue.ArrayCount += 1;
> -      Buffer                   = Buffer->Next;
> -    }
> +    if (!CompareRedfishStringArrayValues (ArrayHead,
> RedfishValue.Value.StringArray, RedfishValue.ArrayCount)) {
> +      //
> +      // Apply settings from redfish
> +      //
> +      DEBUG ((DEBUG_MANAGEABILITY, "%a: %a.%a apply %s for array\n",
> __func__, Schema, Version, ConfigureLang));
> +      FreeArrayTypeRedfishValue (&RedfishValue);
> 
> -    //
> -    // Allocate pool for new values
> -    //
> -    RedfishValue.Value.StringArray = AllocatePool (RedfishValue.ArrayCount
> *sizeof (CHAR8 *));
> -    if (RedfishValue.Value.StringArray == NULL) {
> -      ASSERT (FALSE);
> -      return EFI_OUT_OF_RESOURCES;
> -    }
> +      //
> +      // Convert array from RedfishCS_char_Array to EDKII_REDFISH_VALUE
> +      //
> +      RedfishValue.ArrayCount = 0;
> +      Buffer                  = ArrayHead;
> +      while (Buffer != NULL) {
> +        RedfishValue.ArrayCount += 1;
> +        Buffer                   = Buffer->Next;
> +      }
> 
> -    Buffer = ArrayHead;
> -    Index  = 0;
> -    while (Buffer != NULL) {
> -      RedfishValue.Value.StringArray[Index] = AllocateCopyPool (AsciiStrSize
> (Buffer->ArrayValue), Buffer->ArrayValue);
> -      if (RedfishValue.Value.StringArray[Index] == NULL) {
> +      //
> +      // Allocate pool for new values
> +      //
> +      RedfishValue.Value.StringArray = AllocatePool (RedfishValue.ArrayCount
> *sizeof (CHAR8 *));
> +      if (RedfishValue.Value.StringArray == NULL) {
>          ASSERT (FALSE);
> -        FreePool (RedfishValue.Value.StringArray);
>          return EFI_OUT_OF_RESOURCES;
>        }
> 
> -      Buffer = Buffer->Next;
> -      Index++;
> -    }
> +      Buffer = ArrayHead;
> +      Index  = 0;
> +      while (Buffer != NULL) {
> +        RedfishValue.Value.StringArray[Index] = AllocateCopyPool (AsciiStrSize
> (Buffer->ArrayValue), Buffer->ArrayValue);
> +        if (RedfishValue.Value.StringArray[Index] == NULL) {
> +          ASSERT (FALSE);
> +          FreePool (RedfishValue.Value.StringArray);
> +          return EFI_OUT_OF_RESOURCES;
> +        }
> 
> -    ASSERT (Index <= RedfishValue.ArrayCount);
> +        Buffer = Buffer->Next;
> +        Index++;
> +      }
> 
> -    Status = RedfishPlatformConfigSetValue (Schema, Version, ConfigureLang,
> RedfishValue);
> -    if (!EFI_ERROR (Status)) {
> -      //
> -      // Configuration changed. Enable system reboot flag.
> -      //
> -      REDFISH_ENABLE_SYSTEM_REBOOT ();
> +      ASSERT (Index <= RedfishValue.ArrayCount);
> +
> +      Status = RedfishPlatformConfigSetValue (Schema, Version, ConfigureLang,
> RedfishValue);
> +      if (!EFI_ERROR (Status)) {
> +        //
> +        // Configuration changed. Enable system reboot flag.
> +        //
> +        REDFISH_ENABLE_SYSTEM_REBOOT ();
> +      } else {
> +        DEBUG ((DEBUG_ERROR, "%a: apply %s array failed: %r\n", __func__,
> ConfigureLang, Status));
> +      }
>      } else {
> -      DEBUG ((DEBUG_ERROR, "%a: apply %s array failed: %r\n", __func__,
> ConfigureLang, Status));
> +      DEBUG ((DEBUG_ERROR, "%a: %a.%a %s array value has no change\n",
> + __func__, Schema, Version, ConfigureLang));
>      }
>    } else {
> -    DEBUG ((DEBUG_ERROR, "%a: %a.%a %s array value has no change\n",
> __func__, Schema, Version, ConfigureLang));
> +    DEBUG ((DEBUG_ERROR, "%a: %a.%a %s array value has invalid element,
> skip!\n", __func__, Schema, Version, ConfigureLang));
> +    Status = EFI_DEVICE_ERROR;
>    }
> 
>    for (Index = 0; Index < RedfishValue.ArrayCount; Index++) { @@ -3432,6
> +3440,72 @@ CompareRedfishStringArrayValues (
>    return TRUE;
>  }
> 
> +/**
> +
> +  Check and see if value in Redfish string array can be found in HII
> + configuration string array. This is to see if there is any invalid
> + values from Redfish.
> +
> +  @param[in]  Head          The head of string array.
> +  @param[in]  StringArray   Input string array.
> +  @param[in]  ArraySize     The size of StringArray.
> +
> +  @retval     TRUE          All string in Redfish array are as same as string
> +                            in HII configuration array.
> +              FALSE         These two array are not identical.
> +
> +**/
> +BOOLEAN
> +ValidateRedfishStringArrayValues (
> +  IN RedfishCS_char_Array  *Head,
> +  IN CHAR8                 **StringArray,
> +  IN UINTN                 ArraySize
> +  )
> +{
> +  UINTN                 Index;
> +  UINTN                 InputArrayCount;
> +  RedfishCS_char_Array  *CharArrayBuffer;
> +
> +  if ((Head == NULL) || (StringArray == NULL) || (ArraySize == 0)) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // Check to see if string from Redfish can be found in string array
> + // returned by HII or not. If not, the input from Redfish is invalid.
> +  //
> +  CharArrayBuffer = Head;
> +  Index           = 0;
> +  InputArrayCount = 0;
> +  while (CharArrayBuffer != NULL) {
> +    for (Index = 0; Index < ArraySize; Index++) {
> +      if (AsciiStrCmp (StringArray[Index], CharArrayBuffer->ArrayValue) == 0) {
> +        break;
> +      }
> +    }
> +
> +    if (Index == ArraySize) {
> +      DEBUG ((DEBUG_ERROR, "%a: input string: %a is not found in HII string
> list\n", __func__, CharArrayBuffer->ArrayValue));
> +      return FALSE;
> +    }
> +
> +    InputArrayCount += 1;
> +    CharArrayBuffer  = CharArrayBuffer->Next;  }
> +
> +  //
> +  // Check to see if the number of string from Redfish equals to the
> + // number of string returned by HII. HII only accepts the same  //
> + number of string array due to the design or HII ordered list.
> +  //
> +  if (InputArrayCount != ArraySize) {
> +    DEBUG ((DEBUG_ERROR, "%a: input string size: %d is not the same as HII
> string list size: %d\n", __func__, InputArrayCount, ArraySize));
> +    return FALSE;
> +  }
> +
> +  return TRUE;
> +}
> +
>  /**
> 
>    Check and see if value in Redfish numeric array are all the same as the one
> --
> 2.17.1
> 
> -The information contained in this message may be confidential and proprietary
> to American Megatrends (AMI). This communication is intended to be read only by
> the individual or entity to whom it is addressed or by their designee. If the reader
> of this message is not the intended recipient, you are on notice that any
> distribution of this message, in any form, is strictly prohibited. Please promptly
> notify the sender by reply e-mail or by telephone at 770-246-8600, and then
> delete or destroy all copies of the transmission.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112026): https://edk2.groups.io/g/devel/message/112026
Mute This Topic: https://groups.io/mt/102459779/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-