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.venkat...@arm.com> > Sent: Wednesday, October 31, 2018 1:00 AM > To: Jin, Eric <eric....@intel.com>; edk2-devel@lists.01.org > Cc: Wu, Jiaxin <jiaxin...@intel.com>; supreeth.venkat...@arm.com > Subject: Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail > of the HwErrRecVariableName > > Reviewed-by: Supreeth Venkatesh <supreeth.venkat...@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.venkat...@arm.com> > > Cc: Jiaxin Wu <jiaxin...@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Jin <eric....@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 > > // > > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
