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

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

On Fri, Oct 28, 2016 at 04:14:43PM +0100, Ard Biesheuvel wrote:
> 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?

If it goes after the one I just acked, sure:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>