public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "PierreGondois" <pierre.gondois@arm.com>
To: Jeshua Smith <jeshuas@nvidia.com>, devel@edk2.groups.io
Cc: Sami.Mujawar@arm.com
Subject: Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text\r handling
Date: Tue, 10 Oct 2023 12:13:53 +0200	[thread overview]
Message-ID: <b3ba1404-6839-44bd-949f-f7ea5cb3da38@arm.com> (raw)
In-Reply-To: <d5bb66a2b0864374d1b7f0789cfeca2cb9c23d1a.1696608794.git.jeshuas@nvidia.com>

Hello Jeshua,
Just a small remark, but this should be equivalent, so:

Reviewed-by: Pierre Gondois <pierre.gondois@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@nvidia.com>
> ---
>   .../ConfigurationManagerObjectParser.c        | 176 ++++++++++++++----
>   1 file changed, 143 insertions(+), 33 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> index 99d6032510..92df1efee8 100644
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.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() ?

>   };
>   
>   /** 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 (#109487): https://edk2.groups.io/g/devel/message/109487
Mute This Topic: https://groups.io/mt/101801390/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2023-10-10 10:13 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-06 16:28 [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io
2023-10-06 16:28 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text handling Jeshua Smith via groups.io
2023-10-10 10:13   ` PierreGondois [this message]
2023-10-10 14:57     ` Jeshua Smith via groups.io
2023-10-06 16:28 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg/TableHelperLib: Enhance error handling Jeshua Smith via groups.io
2023-10-10 10:13   ` PierreGondois
2023-10-10 15:41     ` Jeshua Smith via groups.io
2023-10-11 12:51       ` PierreGondois
2023-10-23 15:30 ` [edk2-devel] [PATCH 0/2] DynamicTablesPkg/TableHelperLib updates Jeshua Smith via groups.io
2023-10-23 16:55 ` Sami Mujawar
     [not found] ` <1790CAE852EF102A.20272@groups.io>
2023-10-23 17:10   ` Sami Mujawar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b3ba1404-6839-44bd-949f-f7ea5cb3da38@arm.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox