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

[Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>]

Hi Eric,

I could not find any reference regarding "magic numbers" in edk2 C coding standards document.
So, I guess I am fine with documenting "magic numbers" with comments.

Thanks,
Supreeth

-----Original Message-----
From: Jin, Eric <eric.jin@intel.com>
Sent: Wednesday, October 31, 2018 9:09 PM
To: Supreeth Venkatesh <Supreeth.Venkatesh@arm.com>; edk2-devel@lists.01.org
Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName

Hi Supreeth,

Got your worry. It is difficult to define the clear/meaningful macro to express the implication and avoid the ambiguity in my eye.
How about to document the clear comments in the key process?  It is more help for reading by someone new and sustaining.


Best Regards
Eric

-----Original Message-----
From: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
Sent: Thursday, November 1, 2018 12:24 AM
To: Jin, Eric <eric.jin@intel.com>; edk2-devel@lists.01.org
Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName

On Wed, 2018-10-31 at 02:29 +0000, Jin, Eric wrote:
> Hi Supreeth,
Hi Eric,
>
> Thank for the comments.
> I will re-create the patch to add the definition of the
> HwErrRecVariableNamePrefixLength(8) and
> HwErrRecVariableNameIndexLength(4).
Thank you.

>
> There are two meanings to 2. To record the step number(2) used  by the
> recoverylib or address (byte[2]) to save the recovery data
> (HwErrRecVariableName)
> It is not applicable macro definition and just code logic here. What
> is your opinion?
Array Index [2] - In general one iterates over an array, performing some operation on each element, because one doesn't know how long the array is and you can't just access it in a hardcoded manner.
Does index "2" have special meaning, since this index is chosen rather than 0?

RecoveryData[0] = 2;
Does only "2" have special meaning?

Looks like we can define (or similar)
#define SecondResetRecord 2 (or something more meaningful)

and for array
#define HwErrRecIndex 2 (or something more meaningful)

No strong opinion - but someone new who starts developing test on top of this file will have a proper context and documentation to get going.

>
>
> Best Regards
> Eric
>
> -----Original Message-----
> From: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> Sent: Wednesday, October 31, 2018 1:00 AM
> To: Jin, Eric <eric.jin@intel.com>; edk2-devel@lists.01.org
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; supreeth.venkatesh@arm.com
> Subject: Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail
> of the HwErrRecVariableName
>
> Reviewed-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com> If the
> below magic number comments(inline) are fixed before commit.
>
> On Tue, 2018-10-30 at 16:38 +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>
> > 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(-)
> >
> > diff --git a/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/VariableServicesBBTestFunction.c b/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/VariableServicesBBTestFunction.c
> > index d1064ce..df1bbe7 100644
> > --- a/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/VariableServicesBBTestFunction.c
> > +++ b/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/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[HwErrRecVariableNameL
> > en
> > gth];
> >    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 );
>
> I understand this line is not part of this patch, but however can we
> define "#define" for magic number 8, while we are touching this file.
>
> > +  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;
>
> I understand this line is not part of this patch, but however can we
> define "#define" for magic number 2, while we are touching this file.
>
> > -  SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName,
> > 12
> > );
> > -  RecoveryLib->WriteResetRecord( RecoveryLib, 13*sizeof(CHAR16)+2,
> > RecoveryData );
> > +  SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName,
> > HwErrRecVariableNameLength-1 );
>
> "#define" for magic number 2
>
> > +  RecoveryLib->WriteResetRecord( RecoveryLib,
> > HwErrRecVariableNameLength*sizeof(CHAR16)+2, RecoveryData );
>
> "#define" for magic number 2
>
> >
> >    //
> >    // 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),
>
> "#define" for magic number 2
>
> > HwErrRecVariableNameLength-1 );
> >    Status = RT->GetVariable (
> >                          HwErrRecVariableName,
> >                          &gHwErrRecGuid, diff --git a/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/VariableServicesBBTestMain.h b/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/VariableServicesBBTestMain.h
> > index 051ae6f..b645b55 100644
> > --- a/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/VariableServicesBBTestMain.h
> > +++ b/uefi-
> > sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/Black
> > Bo
> > xTest/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.
> > +//
>
> Good documentation.
>
> > +#define HwErrRecVariableNameLength 13
> > +
> > +//
> >  // Global Variables
> >  //
> >
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.