Re: [edk2-devel] [PATCH v1 1/1] DynamicTablesPkg: Add SMBIOS String table helper library

["PierreGondois" <pierre.gondois@arm.com>]

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