From: "Jeshua Smith via groups.io" <jeshuas=nvidia.com@groups.io>
To: Pierre Gondois <pierre.gondois@arm.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Sami.Mujawar@arm.com" <Sami.Mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg/TableHelperLib: Fix and improve text\r handling
Date: Tue, 10 Oct 2023 14:57:31 +0000 [thread overview]
Message-ID: <DM6PR12MB337116FE0201064287CEB4D0DBCDA@DM6PR12MB3371.namprd12.prod.outlook.com> (raw)
In-Reply-To: <b3ba1404-6839-44bd-949f-f7ea5cb3da38@arm.com>
Thanks for the review. Reply inline.
-----Original Message-----
From: Pierre Gondois <pierre.gondois@arm.com>
Sent: Tuesday, October 10, 2023 4:14 AM
To: Jeshua Smith <jeshuas@nvidia.com>; devel@edk2.groups.io
Cc: Sami.Mujawar@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@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/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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-10-10 14:57 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
2023-10-10 14:57 ` Jeshua Smith via groups.io [this message]
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=DM6PR12MB337116FE0201064287CEB4D0DBCDA@DM6PR12MB3371.namprd12.prod.outlook.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