[edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling
Jeshua Smith via groups.io
jeshuas=nvidia.com at groups.io
Tue Oct 10 14:57:31 UTC 2023
Thanks for the review. Reply inline.
-----Original Message-----
From: Pierre Gondois <pierre.gondois at arm.com>
Sent: Tuesday, October 10, 2023 4:14 AM
To: Jeshua Smith <jeshuas at nvidia.com>; devel at edk2.groups.io
Cc: Sami.Mujawar at arm.com
Subject: Re: [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling
External email: Use caution opening links or attachments
Hello Jeshua,
Just a small remark, but this should be equivalent, so:
Reviewed-by: Pierre Gondois <pierre.gondois at arm.com>
On 10/6/23 18:28, Jeshua Smith wrote:
> This fixes two bugs and adds some enhancements to the handling of
> characters and strings in objects being printed by the CM ObjectParser.
>
> Bug fixes:
> 1. PrintOemID() currently attempts to print characters with "%C",
> but the correct syntax is (lowercase) "%c". This bug results in
> "CCCCCC" being printed instead of the actual ASCII characters.
> 2. PrintString() is being passed a pointer to data in objects, but in
> some cases this data is the actual string to print and other cases
> it is a pointer to the string to print. This adds a PrintStringPtr
> function and uses the correct functions depending on the situation.
>
> Enhancements:
> 1. Some objects contain ASCII characters, which are currently printed
> as their hex values. This adds functions to print out ASCII
> character fields as text rather than hex, and uses those functions in
> several cases where the object data is defined to be ASCII.
> 2. The PrintOemID() function is replaced with the new identical but more
> generecically-named PrintChar6() function.
>
> Signed-off-by: Jeshua Smith <jeshuas at nvidia.com>
> ---
> .../ConfigurationManagerObjectParser.c | 176 ++++++++++++++----
> 1 file changed, 143 insertions(+), 33 deletions(-)
>
> diff --git
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> index 99d6032510..92df1efee8 100644
> ---
> a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerO
> bjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationMana
> +++ gerObjectParser.c
> @@ -14,7 +14,7 @@
> STATIC
> VOID
> EFIAPI
> -PrintOemId (
> +PrintString (
> CONST CHAR8 *Format,
> UINT8 *Ptr
> );
> @@ -22,7 +22,31 @@ PrintOemId (
> STATIC
> VOID
> EFIAPI
> -PrintString (
> +PrintStringPtr (
> + CONST CHAR8 *Format,
> + UINT8 *Ptr
> + );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar4 (
> + CONST CHAR8 *Format,
> + UINT8 *Ptr
> + );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar6 (
> + CONST CHAR8 *Format,
> + UINT8 *Ptr
> + );
> +
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar8 (
> CONST CHAR8 *Format,
> UINT8 *Ptr
> );
> @@ -190,16 +214,16 @@ STATIC CONST CM_OBJ_PARSER CmArmItsGroupNodeParser[] = {
> /** A parser for EArmObjNamedComponent.
> */
> STATIC CONST CM_OBJ_PARSER CmArmNamedComponentNodeParser[] = {
> - { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
> - { "IdMappingCount", 4, "0x%x", NULL },
> - { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
> - { "Flags", 4, "0x%x", NULL },
> - { "CacheCoherent", 4, "0x%x", NULL },
> - { "AllocationHints", 1, "0x%x", NULL },
> - { "MemoryAccessFlags", 1, "0x%x", NULL },
> - { "AddressSizeLimit", 1, "0x%x", NULL },
> - { "ObjectName", sizeof (CHAR8 *), NULL, PrintString },
> - { "Identifier", 4, "0x%x", NULL },
> + { "Token", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
> + { "IdMappingCount", 4, "0x%x", NULL },
> + { "IdMappingToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL },
> + { "Flags", 4, "0x%x", NULL },
> + { "CacheCoherent", 4, "0x%x", NULL },
> + { "AllocationHints", 1, "0x%x", NULL },
> + { "MemoryAccessFlags", 1, "0x%x", NULL },
> + { "AddressSizeLimit", 1, "0x%x", NULL },
> + { "ObjectName", sizeof (CHAR8 *), NULL, PrintStringPtr },
> + { "Identifier", 4, "0x%x", NULL },
> };
>
> /** A parser for EArmObjRootComplex.
> @@ -740,19 +764,19 @@ STATIC CONST CM_OBJ_PARSER_ARRAY ArmNamespaceObjectParser[] = {
> */
> STATIC CONST CM_OBJ_PARSER StdObjCfgMgrInfoParser[] = {
> { "Revision", 4, "0x%x", NULL },
> - { "OemId[6]", 6, "%C%C%C%C%C%C", PrintOemId }
> + { "OemId[6]", 6, "%c%c%c%c%c%c", PrintChar6 }
NIT:
Is it necessary, since "%c%c%c%c%c%c" is the default format of PrintChar6() ?
[JDS]: I considered changing it as unnecessary, but in the end left it as is since the original code used the same pattern (i.e. passing in a format that was the same as the default format).
> };
>
> /** A parser for EStdObjAcpiTableList.
> */
> STATIC CONST CM_OBJ_PARSER StdObjAcpiTableInfoParser[] = {
> - { "AcpiTableSignature", 4, "0x%x", NULL },
> - { "AcpiTableRevision", 1, "%d", NULL },
> - { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL },
> - { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL },
> - { "OemTableId", 8, "0x%LLX", NULL },
> - { "OemRevision", 4, "0x%x", NULL },
> - { "MinorRevision", 1, "0x%x", NULL },
> + { "AcpiTableSignature", 4, "%c%c%c%c", PrintChar4 },
> + { "AcpiTableRevision", 1, "%d", NULL },
> + { "TableGeneratorId", sizeof (ACPI_TABLE_GENERATOR_ID), "0x%x", NULL },
> + { "AcpiTableData", sizeof (EFI_ACPI_DESCRIPTION_HEADER *), "0x%p", NULL },
> + { "OemTableId", 8, "%c%c%c%c%c%c%c%c", PrintChar8 },
> + { "OemRevision", 4, "0x%x", NULL },
> + { "MinorRevision", 1, "0x%x", NULL },
> };
>
> /** A parser for EStdObjSmbiosTableList.
> @@ -773,22 +797,99 @@ STATIC CONST CM_OBJ_PARSER_ARRAY StdNamespaceObjectParser[] = {
> ARRAY_SIZE (StdObjSmbiosTableInfoParser) },
> };
>
> -/** Print OEM Id.
> +/** Print string data.
> +
> + The string must be NULL terminated.
> +
> + @param [in] Format Format to print the Ptr.
> + @param [in] Ptr Pointer to the string.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintString (
> + IN CONST CHAR8 *Format,
> + IN UINT8 *Ptr
> + )
> +{
> + if (Ptr == NULL) {
> + ASSERT (0);
> + return;
> + }
> +
> + DEBUG ((DEBUG_ERROR, "%a", Ptr));
> +}
> +
> +/** Print string from pointer.
> +
> + The string must be NULL terminated.
> +
> + @param [in] Format Format to print the string.
> + @param [in] Ptr Pointer to the string pointer.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintStringPtr (
> + IN CONST CHAR8 *Format,
> + IN UINT8 *Ptr
> + )
> +{
> + UINT8 *String;
> +
> + if (Ptr == NULL) {
> + ASSERT (0);
> + return;
> + }
> +
> + String = *(UINT8 **)Ptr;
> +
> + if (String == NULL) {
> + String = (UINT8 *)"(NULLPTR)";
> + }
> +
> + PrintString (Format, String);
> +}
> +
> +/** Print 4 characters.
>
> @param [in] Format Format to print the Ptr.
> - @param [in] Ptr Pointer to the OEM Id.
> + @param [in] Ptr Pointer to the characters.
> **/
> STATIC
> VOID
> EFIAPI
> -PrintOemId (
> +PrintChar4 (
> IN CONST CHAR8 *Format,
> IN UINT8 *Ptr
> )
> {
> DEBUG ((
> - DEBUG_INFO,
> - (Format != NULL) ? Format : "%C%C%C%C%C%C",
> + DEBUG_ERROR,
> + (Format != NULL) ? Format : "%c%c%c%c",
> + Ptr[0],
> + Ptr[1],
> + Ptr[2],
> + Ptr[3]
> + ));
> +}
> +
> +/** Print 6 characters.
> +
> + @param [in] Format Format to print the Ptr.
> + @param [in] Ptr Pointer to the characters.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +PrintChar6 (
> + IN CONST CHAR8 *Format,
> + IN UINT8 *Ptr
> + )
> +{
> + DEBUG ((
> + DEBUG_ERROR,
> + (Format != NULL) ? Format : "%c%c%c%c%c%c",
> Ptr[0],
> Ptr[1],
> Ptr[2],
> @@ -798,22 +899,31 @@ PrintOemId (
> ));
> }
>
> -/** Print string.
> -
> - The string must be NULL terminated.
> +/** Print 8 characters.
>
> @param [in] Format Format to print the Ptr.
> - @param [in] Ptr Pointer to the string.
> + @param [in] Ptr Pointer to the characters.
> **/
> STATIC
> VOID
> EFIAPI
> -PrintString (
> - CONST CHAR8 *Format,
> - UINT8 *Ptr
> +PrintChar8 (
> + IN CONST CHAR8 *Format,
> + IN UINT8 *Ptr
> )
> {
> - DEBUG ((DEBUG_ERROR, "%a", Ptr));
> + DEBUG ((
> + DEBUG_ERROR,
> + (Format != NULL) ? Format : "%c%c%c%c%c%c%c%c",
> + Ptr[0],
> + Ptr[1],
> + Ptr[2],
> + Ptr[3],
> + Ptr[4],
> + Ptr[5],
> + Ptr[6],
> + Ptr[7]
> + ));
> }
>
> /** Print fields of the objects.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109495): https://edk2.groups.io/g/devel/message/109495
Mute This Topic: https://groups.io/mt/101801390/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list