Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName

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

Hi Eric,

On Tue, Oct 30, 2018 at 04:38:36PM +0800, Eric Jin wrote:
> Make the HwErrRecVariableName as valid the string.
> Ensure the HwErrRecVariable could be deleted before the test exit.
> 
> Cc: Supreeth Venkatesh <supreeth.venkatesh@arm.com>

(Sidenote - I don't see Supreeth actually on cc on this patch.)

> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Jin <eric.jin@intel.com>
> ---
>  .../BlackBoxTest/VariableServicesBBTestFunction.c            | 12 +++++++-----
>  .../BlackBoxTest/VariableServicesBBTestMain.h                | 10 +++++++++-
>  2 files changed, 16 insertions(+), 6 deletions(-)

Could you follow the steps outlined in Laszlo's excellent guide
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
when generating the patches to send out?

For this specific patch, this would have made two major differences:
1) We would have seen the full path to files in the summary above,
   instead of the .../.
2) It would have ordered the files such that the .h would have been
   shown before the .c.

2 turns out to make a major difference when reviewing this particular
patch, because...

> diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c
> index d1064ce..df1bbe7 100644
> --- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c
> +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c
> @@ -1,7 +1,7 @@
>  /** @file
>  
>    Copyright 2006 - 2012 Unified EFI, Inc.<BR>
> -  Copyright (c) 2010 - 2012, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -2855,7 +2855,7 @@ HardwareErrorRecordFuncTest (
>    UINT64                RemainingVariableStorageSize;
>    UINT64                MaximumVariableSize;
>    
> -  CHAR16                HwErrRecVariableName[13];
> +  CHAR16                HwErrRecVariableName[HwErrRecVariableNameLength];

... the first thing I see when I look at this patch is
"Hang on, are we permitting runtime-sized arrays?"
This could have sent me off on a 15-90 minute task of
- triple-checking against the coding style document.
- search the mailing list (I recalled _some_ discussions around
  this recently, but that may have been for Linux, and variable-length
  arrays)
- search the edk2 codebase for precedent.
- testing with various toolchains whether they were happy compiling
  it, getting surprised when it worked in some of them, and then spent
  additional time figuring out why.

It could have, but it didn't. Because the patch is small enough that I
decided to read through it in its entirety before going off on
investigations.

More below.

>    CHAR16                HwErrRecVariable[] = L"This is a HwErrRec variable!";
>    
>    CHAR16                GetVariableName[MAX_BUFFER_SIZE];
> @@ -3015,6 +3015,7 @@ HardwareErrorRecordFuncTest (
>    HwErrRecVariableName[0] = L'\0';
>    SctStrCat ( HwErrRecVariableName, L"HwErrRec" );
>    Myitox( MaxNum, HwErrRecVariableName+8 );
> +  HwErrRecVariableName[HwErrRecVariableNameLength-1] = L'\0';
>    
>    //
>    // Set the new HwErrRec variable to the global variable
> @@ -3036,8 +3037,8 @@ HardwareErrorRecordFuncTest (
>    // Write reset record
>    //
>    RecoveryData[0] = 2;
> -  SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, 12 );
> -  RecoveryLib->WriteResetRecord( RecoveryLib, 13*sizeof(CHAR16)+2, RecoveryData );
> +  SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, HwErrRecVariableNameLength-1 );
> +  RecoveryLib->WriteResetRecord( RecoveryLib, HwErrRecVariableNameLength*sizeof(CHAR16)+2, RecoveryData );
>    
>    //
>    // Prompt the user about the cold reset and reset the system
> @@ -3052,7 +3053,8 @@ HardwareErrorRecordFuncTest (
>    //
>  step2:
>    DataSize = 255;
> -  SctStrnCpy ( HwErrRecVariableName, (CHAR16*)(RecoveryData+2), 12 );
> +  HwErrRecVariableName[HwErrRecVariableNameLength-1] = L'\0';
> +  SctStrnCpy ( HwErrRecVariableName, (CHAR16*)(RecoveryData+2), HwErrRecVariableNameLength-1 );
>    Status = RT->GetVariable (
>                          HwErrRecVariableName,
>                          &gHwErrRecGuid,
> diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h
> index 051ae6f..b645b55 100644
> --- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h
> +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h
> @@ -1,7 +1,7 @@
>  /** @file
>  
>    Copyright 2006 - 2016 Unified EFI, Inc.<BR>
> -  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
>  
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -125,6 +125,14 @@ Abstract:
>  #endif
>  
>  //
> +// The Variable Name of Hardware Error Record Variables 
> +// defined in the UEFI Spec is HwErrRec####. For example,
> +// HwErrRec0001, HwErrRec0002, HwErrRecF31A, etc. 
> +// Consider the tail of string, the length is 13.
> +//
> +#define HwErrRecVariableNameLength 13

But then I get here, and I see that HwErrRecVariableNameLength is a
statically defined macro. At which point my comment on this patch
narrows down to:

Please name macros appropriately.
HwErrRecVariableNameLength is the name of a variable.
The appropriate name for this macro would be
HW_ERR_ERC_VARIABLE_NAME_LENGTH.

This all according to section 4.3.5 - Type and Macro Names, in the
coding standards specification.

Regards,

Leif