From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
 by mx.groups.io with SMTP id smtpd.web10.14222.1678434651956388445
 for <devel@edk2.groups.io>;
 Thu, 09 Mar 2023 23:50:52 -0800
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
	by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6C943C14;
	Thu,  9 Mar 2023 23:51:34 -0800 (PST)
Received: from [192.168.1.12] (unknown [172.31.20.19])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6862F3F67D;
	Thu,  9 Mar 2023 23:50:50 -0800 (PST)
Message-ID: <60da00db-1ddb-0689-8dce-0dc821a3aee8@arm.com>
Date: Fri, 10 Mar 2023 08:50:30 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [edk2-devel] [PATCH v1 1/1] DynamicTablesPkg: Add SMBIOS String table helper library
To: Sami Mujawar <sami.mujawar@arm.com>, devel@edk2.groups.io
References: <61170181-b9e3-cb57-a439-47f0d816bf04@arm.com>
 <8800.1678360288123253286@groups.io>
From: "PierreGondois" <pierre.gondois@arm.com>
In-Reply-To: <8800.1678360288123253286@groups.io>
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hello Sami,

On 3/9/23 12:11, Sami Mujawar wrote:
> Hi Pierre,
> 
> On Tue, Oct 4, 2022 at 01:22 AM, PierreGondois wrote:
> 
>             +EFI_STATUS
>             +EFIAPI
>             +StringTablePublishStringSet (
>             + IN STRING_TABLE *CONST StrTable,
>             + IN CHAR8 *CONST SmbiosStringAreaStart,
>             + IN CONST UINTN SmbiosStringAreaSize
>             + )
>             +{
>             + UINT8 Index;
>             + STRING_ELEMENT *StrElement;
>             + CHAR8 *SmbiosString;
>             + UINTN BytesRemaining;
>             + UINTN BytesCopied;
>             +
>             + if ((StrTable == NULL) || (SmbiosStringAreaStart == NULL)) {
>             + return EFI_INVALID_PARAMETER;
>             + }
>             +
>             + if (SmbiosStringAreaSize < StringTableGetStringSetSize (StrTable)) {
>             + return EFI_BUFFER_TOO_SMALL;
>             + }
>             +
>             + SmbiosString = SmbiosStringAreaStart;
>             + BytesRemaining = SmbiosStringAreaSize;
>             +
>             + if (StrTable->StrCount == 0) {
>             + // See Section 6.1.3 Text strings, SMBIOS Specification Version 3.6.0
>             + // If the formatted portion of the structure contains string-reference
>             + // fields and all the string fields are set to 0 (no string references),
>             + // the formatted section of the structure is followed by two null (00h)
>             + // BYTES.
>             + *SmbiosString++ = '\0';
>             + } else {
>             + for (Index = 0; Index < StrTable->StrCount; Index++) {
>             + StrElement = &StrTable->Elements[Index];
>             + AsciiStrCpyS (SmbiosString, BytesRemaining, StrElement->String);
>             +
>             + // See Section 6.1.3 Text strings, SMBIOS Specification Version 3.6.0
>             + // - Each string is terminated with a null (00h) BYTE
>             + // Bytes Copied = String length + 1 for the string NULL terminator.
>             + BytesCopied = StrElement->StringLen + 1;
>             + BytesRemaining -= BytesCopied;
>             + SmbiosString += BytesCopied;
>             + }
>             + }
>             +
>             + // See Section 6.1.3 Text strings, SMBIOS Specification Version 3.6.0
>             + // - the set of strings is terminated with an additional null (00h) BYTE.
>             + *SmbiosString = '\0';
> 
>         [GM] Shouldn't you advance the SmbiosString pointer by one more ? After
>         the loop isn't SmbiosString going to be at the NULL char of the last
>         string ? And we're trying to add one more NULL character ?
>         Should it be:
>         SmbiosString++;
>         *SmbiosString = '\0';
> 
>     I didn't try to run the code, but it seems ok to me. Assuming the string has 9 chars:
>     """
>     // Copy 9 chars + 1 NULL char
>     AsciiStrCpyS (SmbiosString, BytesRemaining, StrElement->String);
> 
>     // We have copied 10 chars
>     BytesCopied = StrElement->StringLen + 1;
>     BytesRemaining -= BytesCopied;
>     // Increment SmbiosString of 10 chars, so SmbiosString is pointing
>     // to an un-initialized char now and we can continue.
>     SmbiosString += BytesCopied;
>     """
> 
>     However, maybe we need to add an extra check/ASSERT for BytesRemaining before
>     writing the last NULL char.
> 
> The check at the function entry i.e. "if (SmbiosStringAreaSize < StringTableGetStringSetSize (StrTable)) {"
> should cover this, as BytesRemaining is derived from SmbiosStringAreaSize.
> So as long as StringTableGetStringSetSize() returns the correct size, the additional check before writing
> the last NULL char is not needed.

Ok sure, it was just a suggestion.
Also just in case, there was this remark from you:
https://edk2.groups.io/g/devel/message/93654

Otherwise:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Regards,
Pierre

> 
> Please let me know if you think otherwise.
> 
> Regards,
> 
> Sami Mujawar