Sorry for the delay review. I go through the changes, and it looks good to me.
Reviewed-by: Nickle Wang <nick...@nvidia.com> Regards, Nickle > -----Original Message----- > From: Chang, Abner <abner.ch...@amd.com> > Sent: Monday, October 2, 2023 11:10 AM > To: Mike Maslenkin <mike.maslen...@gmail.com>; devel@edk2.groups.io; Nickle > Wang <nick...@nvidia.com> > Cc: ig...@ami.com > Subject: RE: [PATCH 5/9] RedfishClientPkg: reduce identation level by adding > early > return > > External email: Use caution opening links or attachments > > > [AMD Official Use Only - General] > > Never mind, I see you had created a PR for this. However, we need > @nick...@nvidia.com to check if the code logic is kept the same with this > change. > > Thank you for helping on this package, Mike. > Abner > > > -----Original Message----- > > From: Chang, Abner > > Sent: Monday, October 2, 2023 11:01 AM > > To: Mike Maslenkin <mike.maslen...@gmail.com>; devel@edk2.groups.io > > Cc: nick...@nvidia.com; ig...@ami.com > > Subject: RE: [PATCH 5/9] RedfishClientPkg: reduce identation level by > > adding early return > > > > Hi Mike, > > I can't apply the entire patch set from either outlook or Group.io. > > The format of patch on both are looked weird. I can still review the > > short ones, but the change of 5/9 is a bit long one. Could you please > > check the patch format and resend 5/9? Thank you. > > > > Abner > > > > > -----Original Message----- > > > From: Mike Maslenkin <mike.maslen...@gmail.com> > > > Sent: Saturday, September 30, 2023 5:59 AM > > > To: devel@edk2.groups.io > > > Cc: Chang, Abner <abner.ch...@amd.com>; nick...@nvidia.com; > > > ig...@ami.com; Mike Maslenkin <mike.maslen...@gmail.com> > > > Subject: [PATCH 5/9] RedfishClientPkg: reduce identation level by > > > adding > > early > > > return > > > > > > Caution: This message originated from an External Source. Use proper > > caution > > > when opening attachments, clicking links, or responding. > > > > > > > > > This functions contain memory leaks. > > > Less identation helps to solve this issues. > > > > > > Signed-off-by: Mike Maslenkin <mike.maslen...@gmail.com> > > > --- > > > .../RedfishFeatureUtilityLib.c | 289 +++++++++--------- > > > 1 file changed, 146 insertions(+), 143 deletions(-) > > > > > > diff --git > > > > > a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib. > > > c > > > > > b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib. > > > c > > > index 8fa1dc2c3535..0941f33fd73a 100644 > > > --- > > > > > a/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib. > > > c > > > +++ > > > > > b/RedfishClientPkg/Library/RedfishFeatureUtilityLib/RedfishFeatureUtilityLib. > > > c > > > @@ -763,68 +763,69 @@ ApplyFeatureSettingsStringArrayType ( > > > Status = RedfishPlatformConfigGetValue (Schema, Version, > > > ConfigureLang, &RedfishValue); > > > > > > if (EFI_ERROR (Status)) { > > > > > > DEBUG ((DEBUG_ERROR, "%a, %a.%a %s failed: %r\n", __func__, > > > Schema, Version, ConfigureLang, Status)); > > > > > > - } else { > > > > > > - if (RedfishValue.Type != RedfishValueTypeStringArray) { > > > > > > - DEBUG ((DEBUG_ERROR, "%a, %a.%a %s value is not string array > > type\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > - return EFI_DEVICE_ERROR; > > > > > > - } > > > > > > + return Status; > > > > > > + } > > > > > > + > > > > > > + if (RedfishValue.Type != RedfishValueTypeStringArray) { > > > > > > + DEBUG ((DEBUG_ERROR, "%a, %a.%a %s value is not string array > > > + type\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + return EFI_DEVICE_ERROR; > > > > > > + } > > > > > > > > > > > > + // > > > > > > + // If there is no change in array, do nothing > > > > > > + // > > > > > > + if (!CompareRedfishStringArrayValues (ArrayHead, > > > RedfishValue.Value.StringArray, RedfishValue.ArrayCount)) { > > > > > > // > > > > > > - // If there is no change in array, do nothing > > > > > > + // Apply settings from redfish > > > > > > // > > > > > > - 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); > > > > > > + DEBUG ((DEBUG_MANAGEABILITY, "%a, %a.%a apply %s for array\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + FreeArrayTypeRedfishValue (&RedfishValue); > > > > > > > > > > > > - // > > > > > > - // Convert array from RedfishCS_char_Array to EDKII_REDFISH_VALUE > > > > > > - // > > > > > > - RedfishValue.ArrayCount = 0; > > > > > > - Buffer = ArrayHead; > > > > > > - while (Buffer != NULL) { > > > > > > - RedfishValue.ArrayCount += 1; > > > > > > - Buffer = Buffer->Next; > > > > > > - } > > > > > > + // > > > > > > + // Convert array from RedfishCS_char_Array to > > > + EDKII_REDFISH_VALUE > > > > > > + // > > > > > > + RedfishValue.ArrayCount = 0; > > > > > > + Buffer = ArrayHead; > > > > > > + while (Buffer != NULL) { > > > > > > + RedfishValue.ArrayCount += 1; > > > > > > + Buffer = Buffer->Next; > > > > > > + } > > > > > > > > > > > > - // > > > > > > - // Allocate pool for new values > > > > > > - // > > > > > > - RedfishValue.Value.StringArray = AllocatePool > > > (RedfishValue.ArrayCount > > > *sizeof (CHAR8 *)); > > > > > > - if (RedfishValue.Value.StringArray == NULL) { > > > > > > + // > > > > > > + // 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; > > > > > > + } > > > > > > + > > > > > > + 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); > > > > > > return EFI_OUT_OF_RESOURCES; > > > > > > } > > > > > > > > > > > > - 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); > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > - } > > > > > > - > > > > > > - Buffer = Buffer->Next; > > > > > > - Index++; > > > > > > - } > > > > > > + Buffer = Buffer->Next; > > > > > > + Index++; > > > > > > + } > > > > > > > > > > > > - ASSERT (Index <= RedfishValue.ArrayCount); > > > > > > + 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)); > > > > > > - } > > > > > > + 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, %a.%a %s array value has no change\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + DEBUG ((DEBUG_ERROR, "%a, apply %s array failed: %r\n", > > > + __func__, > > > ConfigureLang, Status)); > > > > > > } > > > > > > + } else { > > > > > > + DEBUG ((DEBUG_ERROR, "%a, %a.%a %s array value has no > > > + change\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > } > > > > > > > > > > > > return Status; > > > > > > @@ -866,63 +867,64 @@ ApplyFeatureSettingsNumericArrayType ( > > > Status = RedfishPlatformConfigGetValue (Schema, Version, > > > ConfigureLang, &RedfishValue); > > > > > > if (EFI_ERROR (Status)) { > > > > > > DEBUG ((DEBUG_ERROR, "%a, %a.%a %s failed: %r\n", __func__, > > > Schema, Version, ConfigureLang, Status)); > > > > > > - } else { > > > > > > - if (RedfishValue.Type != RedfishValueTypeIntegerArray) { > > > > > > - DEBUG ((DEBUG_ERROR, "%a, %a.%a %s value is not string array > > type\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > - return EFI_DEVICE_ERROR; > > > > > > - } > > > > > > + return Status; > > > > > > + } > > > > > > + > > > > > > + if (RedfishValue.Type != RedfishValueTypeIntegerArray) { > > > > > > + DEBUG ((DEBUG_ERROR, "%a, %a.%a %s value is not string array > > > + type\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + return EFI_DEVICE_ERROR; > > > > > > + } > > > > > > > > > > > > + // > > > > > > + // If there is no change in array, do nothing > > > > > > + // > > > > > > + if (!CompareRedfishNumericArrayValues (ArrayHead, > > > RedfishValue.Value.IntegerArray, RedfishValue.ArrayCount)) { > > > > > > // > > > > > > - // If there is no change in array, do nothing > > > > > > + // Apply settings from redfish > > > > > > // > > > > > > - if (!CompareRedfishNumericArrayValues (ArrayHead, > > > RedfishValue.Value.IntegerArray, RedfishValue.ArrayCount)) { > > > > > > - // > > > > > > - // Apply settings from redfish > > > > > > - // > > > > > > - DEBUG ((DEBUG_MANAGEABILITY, "%a, %a.%a apply %s for array\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > - FreeArrayTypeRedfishValue (&RedfishValue); > > > > > > + DEBUG ((DEBUG_MANAGEABILITY, "%a, %a.%a apply %s for array\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + FreeArrayTypeRedfishValue (&RedfishValue); > > > > > > > > > > > > - // > > > > > > - // Convert array from RedfishCS_int64_Array to EDKII_REDFISH_VALUE > > > > > > - // > > > > > > - RedfishValue.ArrayCount = 0; > > > > > > - Buffer = ArrayHead; > > > > > > - while (Buffer != NULL) { > > > > > > - RedfishValue.ArrayCount += 1; > > > > > > - Buffer = Buffer->Next; > > > > > > - } > > > > > > + // > > > > > > + // Convert array from RedfishCS_int64_Array to > > > + EDKII_REDFISH_VALUE > > > > > > + // > > > > > > + RedfishValue.ArrayCount = 0; > > > > > > + Buffer = ArrayHead; > > > > > > + while (Buffer != NULL) { > > > > > > + RedfishValue.ArrayCount += 1; > > > > > > + Buffer = Buffer->Next; > > > > > > + } > > > > > > > > > > > > - // > > > > > > - // Allocate pool for new values > > > > > > - // > > > > > > - RedfishValue.Value.IntegerArray = AllocatePool > > (RedfishValue.ArrayCount > > > * sizeof (INT64)); > > > > > > - if (RedfishValue.Value.IntegerArray == NULL) { > > > > > > - ASSERT (FALSE); > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > - } > > > > > > + // > > > > > > + // Allocate pool for new values > > > > > > + // > > > > > > + RedfishValue.Value.IntegerArray = AllocatePool > > > + (RedfishValue.ArrayCount > > * > > > sizeof (INT64)); > > > > > > + if (RedfishValue.Value.IntegerArray == NULL) { > > > > > > + ASSERT (FALSE); > > > > > > + return EFI_OUT_OF_RESOURCES; > > > > > > + } > > > > > > > > > > > > - Buffer = ArrayHead; > > > > > > - Index = 0; > > > > > > - while (Buffer != NULL) { > > > > > > - RedfishValue.Value.IntegerArray[Index] = > > > (INT64)*Buffer->ArrayValue; > > > > > > - Buffer = Buffer->Next; > > > > > > - Index++; > > > > > > - } > > > > > > + Buffer = ArrayHead; > > > > > > + Index = 0; > > > > > > + while (Buffer != NULL) { > > > > > > + RedfishValue.Value.IntegerArray[Index] = > > > + (INT64)*Buffer->ArrayValue; > > > > > > + Buffer = Buffer->Next; > > > > > > + Index++; > > > > > > + } > > > > > > > > > > > > - ASSERT (Index <= RedfishValue.ArrayCount); > > > > > > + 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)); > > > > > > - } > > > > > > + 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, %a.%a %s array value has no change\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + DEBUG ((DEBUG_ERROR, "%a, apply %s array failed: %r\n", > > > + __func__, > > > ConfigureLang, Status)); > > > > > > } > > > > > > + } else { > > > > > > + DEBUG ((DEBUG_ERROR, "%a, %a.%a %s array value has no > > > + change\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > } > > > > > > > > > > > > return Status; > > > > > > @@ -964,63 +966,64 @@ ApplyFeatureSettingsBooleanArrayType ( > > > Status = RedfishPlatformConfigGetValue (Schema, Version, > > > ConfigureLang, &RedfishValue); > > > > > > if (EFI_ERROR (Status)) { > > > > > > DEBUG ((DEBUG_ERROR, "%a, %a.%a %s failed: %r\n", __func__, > > > Schema, Version, ConfigureLang, Status)); > > > > > > - } else { > > > > > > - if (RedfishValue.Type != RedfishValueTypeBooleanArray) { > > > > > > - DEBUG ((DEBUG_ERROR, "%a, %a.%a %s value is not string array > > type\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > - return EFI_DEVICE_ERROR; > > > > > > - } > > > > > > + return Status; > > > > > > + } > > > > > > + > > > > > > + if (RedfishValue.Type != RedfishValueTypeBooleanArray) { > > > > > > + DEBUG ((DEBUG_ERROR, "%a, %a.%a %s value is not string array > > > + type\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + return EFI_DEVICE_ERROR; > > > > > > + } > > > > > > > > > > > > + // > > > > > > + // If there is no change in array, do nothing > > > > > > + // > > > > > > + if (!CompareRedfishBooleanArrayValues (ArrayHead, > > > RedfishValue.Value.BooleanArray, RedfishValue.ArrayCount)) { > > > > > > // > > > > > > - // If there is no change in array, do nothing > > > > > > + // Apply settings from redfish > > > > > > // > > > > > > - if (!CompareRedfishBooleanArrayValues (ArrayHead, > > > RedfishValue.Value.BooleanArray, RedfishValue.ArrayCount)) { > > > > > > - // > > > > > > - // Apply settings from redfish > > > > > > - // > > > > > > - DEBUG ((DEBUG_MANAGEABILITY, "%a, %a.%a apply %s for array\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > - FreeArrayTypeRedfishValue (&RedfishValue); > > > > > > + DEBUG ((DEBUG_MANAGEABILITY, "%a, %a.%a apply %s for array\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + FreeArrayTypeRedfishValue (&RedfishValue); > > > > > > > > > > > > - // > > > > > > - // Convert array from RedfishCS_int64_Array to EDKII_REDFISH_VALUE > > > > > > - // > > > > > > - RedfishValue.ArrayCount = 0; > > > > > > - Buffer = ArrayHead; > > > > > > - while (Buffer != NULL) { > > > > > > - RedfishValue.ArrayCount += 1; > > > > > > - Buffer = Buffer->Next; > > > > > > - } > > > > > > + // > > > > > > + // Convert array from RedfishCS_int64_Array to > > > + EDKII_REDFISH_VALUE > > > > > > + // > > > > > > + RedfishValue.ArrayCount = 0; > > > > > > + Buffer = ArrayHead; > > > > > > + while (Buffer != NULL) { > > > > > > + RedfishValue.ArrayCount += 1; > > > > > > + Buffer = Buffer->Next; > > > > > > + } > > > > > > > > > > > > - // > > > > > > - // Allocate pool for new values > > > > > > - // > > > > > > - RedfishValue.Value.BooleanArray = AllocatePool > > (RedfishValue.ArrayCount > > > * sizeof (BOOLEAN)); > > > > > > - if (RedfishValue.Value.BooleanArray == NULL) { > > > > > > - ASSERT (FALSE); > > > > > > - return EFI_OUT_OF_RESOURCES; > > > > > > - } > > > > > > + // > > > > > > + // Allocate pool for new values > > > > > > + // > > > > > > + RedfishValue.Value.BooleanArray = AllocatePool > > (RedfishValue.ArrayCount > > > * sizeof (BOOLEAN)); > > > > > > + if (RedfishValue.Value.BooleanArray == NULL) { > > > > > > + ASSERT (FALSE); > > > > > > + return EFI_OUT_OF_RESOURCES; > > > > > > + } > > > > > > > > > > > > - Buffer = ArrayHead; > > > > > > - Index = 0; > > > > > > - while (Buffer != NULL) { > > > > > > - RedfishValue.Value.BooleanArray[Index] = (BOOLEAN)*Buffer- > > > >ArrayValue; > > > > > > - Buffer = Buffer->Next; > > > > > > - Index++; > > > > > > - } > > > > > > + Buffer = ArrayHead; > > > > > > + Index = 0; > > > > > > + while (Buffer != NULL) { > > > > > > + RedfishValue.Value.BooleanArray[Index] = (BOOLEAN)*Buffer- > > > >ArrayValue; > > > > > > + Buffer = Buffer->Next; > > > > > > + Index++; > > > > > > + } > > > > > > > > > > > > - ASSERT (Index <= RedfishValue.ArrayCount); > > > > > > + 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)); > > > > > > - } > > > > > > + 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, %a.%a %s array value has no change\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > + DEBUG ((DEBUG_ERROR, "%a, apply %s array failed: %r\n", > > > + __func__, > > > ConfigureLang, Status)); > > > > > > } > > > > > > + } else { > > > > > > + DEBUG ((DEBUG_ERROR, "%a, %a.%a %s array value has no > > > + change\n", > > > __func__, Schema, Version, ConfigureLang)); > > > > > > } > > > > > > > > > > > > return Status; > > > > > > -- > > > 2.32.0 (Apple Git-132) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109506): https://edk2.groups.io/g/devel/message/109506 Mute This Topic: https://groups.io/mt/101667464/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-