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

[Leif Lindholm <leif.lindholm@linaro.org>]

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.

/
    Leif