Re: [PATCH v2 1/2] ArmPlatformPkg/ArmVExpressFastBootDxe: eliminate deprecated string functions

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 28 October 2016 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Oct 28, 2016 at 11:48:40AM +0100, Ard Biesheuvel wrote:
>> Get rid of functions that are no longer available when defining
>> DISABLE_NEW_DEPRECATED_INTERFACES
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> index 4d0811cc5eaf..64b25f8a8c45 100644
>> --- a/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> +++ b/ArmPlatformPkg/ArmVExpressPkg/ArmVExpressFastBootDxe/ArmVExpressFastBoot.c
>> @@ -269,7 +269,7 @@ ArmFastbootPlatformInit (
>>
>>        // Copy handle and partition name
>>        Entry->PartitionHandle = AllHandles[LoopIndex];
>> -      StrnCpy (
>> +      CopyMem (
>>          Entry->PartitionName,
>>          PartitionEntries[PartitionNode->PartitionNumber - 1].PartitionName, // Partition numbers start from 1.
>>          PARTITION_NAME_MAX_LENGTH
>> @@ -320,7 +320,8 @@ ArmFastbootPlatformFlashPartition (
>>    CHAR16                   PartitionNameUnicode[60];
>>    BOOLEAN                  PartitionFound;
>>
>> -  AsciiStrToUnicodeStr (PartitionName, PartitionNameUnicode);
>> +  AsciiStrToUnicodeStrS (PartitionName, PartitionNameUnicode,
>> +    ARRAY_SIZE (PartitionNameUnicode));
>>
>>    PartitionFound = FALSE;
>>    Entry = (FASTBOOT_PARTITION_LIST *) GetFirstNode (&(mPartitionListHead));
>> @@ -396,7 +397,7 @@ ArmFastbootPlatformGetVar (
>>    )
>>  {
>>    if (AsciiStrCmp (Name, "product")) {
>> -    AsciiStrCpy (Value, FixedPcdGetPtr (PcdFirmwareVendor));
>> +    AsciiStrCpyS (Value, 61, FixedPcdGetPtr (PcdFirmwareVendor));
>
> I'm totally OK with the reason for hard-coding this, but could you add
> the comment from the feedback on previous version?:
>   // FASTBOOT_COMMAND_MAX_LENGTH - 4 + NULL terminator
> (Or if there's a better way of putting it.)
>

What if I decorate each entry point with the comment block from the
associated protocol header file instead? (in a follow up patch)
That is common practice in Tianocore anyway, and looks like this for
this particular protocol method:

/*
  If the variable referred to by Name exists, copy it (as a null-terminated
  string) into Value. If it doesn't exist, put the Empty string in Value.

  Variable names and values may not be larger than 60 bytes, excluding the
  terminal null character. This is a limitation of the Fastboot protocol.

  The Fastboot application will handle platform-nonspecific variables
  (Currently "version" is the only one of these.)

  @param[in]  Name   Null-terminated name of Fastboot variable to retrieve.
  @param[out] Value  Caller-allocated buffer for null-terminated value of
                     variable.

  @retval EFI_SUCCESS       The variable was retrieved, or it doesn't exist.
  @retval EFI_DEVICE_ERROR  There was an error looking up the variable. This
                            does _not_ include the variable not existing.
*/