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.venkat...@arm.com> Sent: Thursday, November 1, 2018 12:24 AM To: Jin, Eric <eric....@intel.com>; edk2-devel@lists.01.org Cc: Wu, Jiaxin <jiaxin...@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.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
