[edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling

[PierreGondois]

Hello Jeshua,

On 10/6/23 18:28, Jeshua Smith wrote:
> This patch enhances error handling and reporting in the CM ObjectParser.
> Specifically:
> 1. ObjectIDs used as array indexes are checked for being out of bounds,
>     and if so an error message is printed before the assert.
> 2. An error message is printed for unsupported NameSpaceIDs.
> 3. Adds support for unimplemented parsers by allowing IDs to list a
>     NULL parser, resulting in an unimplemented message being printed.

I am not sure I see in which context 3. would be used/necessary. Is it possible
to detail ?

(Code-wise everything looks good to me)

Regards,
Pierre


> 
> Signed-off-by: Jeshua Smith <jeshuas at nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 47 +++++++++++++------
>   1 file changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> index 92df1efee8..22b8fdb906 100644
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> @@ -795,6 +795,7 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  StdNamespaceObjectParser[] = {
>       ARRAY_SIZE (StdObjAcpiTableInfoParser) },
>     { "EStdObjSmbiosTableList", StdObjSmbiosTableInfoParser,
>       ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
> +  { "EStdObjMax",             NULL,                       0}
>   };
>   
>   /** Print string data.
> @@ -1066,6 +1067,12 @@ ParseCmObjDesc (
>           return;
>         }
>   
> +      if (ObjId >= ARRAY_SIZE (StdNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the StdNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &StdNamespaceObjectParser[ObjId];
>         break;
>       case EObjNameSpaceArm:
> @@ -1074,10 +1081,17 @@ ParseCmObjDesc (
>           return;
>         }
>   
> +      if (ObjId >= ARRAY_SIZE (ArmNamespaceObjectParser)) {
> +        DEBUG ((DEBUG_ERROR, "ObjId 0x%x is missing from the ArmNamespaceObjectParser array\n", ObjId));
> +        ASSERT (0);
> +        return;
> +      }
> +
>         ParserArray = &ArmNamespaceObjectParser[ObjId];
>         break;
>       default:
>         // Not supported
> +      DEBUG ((DEBUG_ERROR, "NameSpaceId 0x%x, ObjId 0x%x is not supported by the parser\n", NameSpaceId, ObjId));
>         ASSERT (0);
>         return;
>     } // switch
> @@ -1095,21 +1109,26 @@ ParseCmObjDesc (
>         ObjIndex + 1,
>         ObjectCount
>         ));
> -    PrintCmObjDesc (
> -      (VOID *)((UINTN)CmObjDesc->Data + Offset),
> -      ParserArray->Parser,
> -      ParserArray->ItemCount,
> -      &RemainingSize,
> -      1
> -      );
> -    if ((RemainingSize > CmObjDesc->Size) ||
> -        (RemainingSize < 0))
> -    {
> -      ASSERT (0);
> -      return;
> -    }
> +    if (ParserArray->Parser == NULL) {
> +      DEBUG ((DEBUG_ERROR, "Parser not implemented\n"));
> +      RemainingSize = 0;
> +    } else {
> +      PrintCmObjDesc (
> +        (VOID *)((UINTN)CmObjDesc->Data + Offset),
> +        ParserArray->Parser,
> +        ParserArray->ItemCount,
> +        &RemainingSize,
> +        1
> +        );
> +      if ((RemainingSize > CmObjDesc->Size) ||
> +          (RemainingSize < 0))
> +      {
> +        ASSERT (0);
> +        return;
> +      }
>   
> -    Offset = CmObjDesc->Size - RemainingSize;
> +      Offset = CmObjDesc->Size - RemainingSize;
> +    }
>     } // for
>   
>     ASSERT (RemainingSize == 0);


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109486): https://edk2.groups.io/g/devel/message/109486
Mute This Topic: https://groups.io/mt/101801385/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-