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

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

On 28 October 2016 at 16:13, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Oct 28, 2016 at 04:02:21PM +0100, Ard Biesheuvel wrote:
>> 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.
>> */
>
> That works for me.
>

Good. Does that mean you are happy with this patch as-is then?