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

["Sami Mujawar" <sami.mujawar@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 3236 bytes --]

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.

Please let me know if you think otherwise.

Regards,

Sami Mujawar

[-- Attachment #2: Type: text/html, Size: 3381 bytes --]