From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=supreeth.venkatesh@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 5BE0A2117FD5D for ; Wed, 31 Oct 2018 09:24:25 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id DCB2C341; Wed, 31 Oct 2018 09:24:24 -0700 (PDT) Received: from supven01-VirtualBox (U203142.usa.Arm.com [10.118.30.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id A1F0F3F71D; Wed, 31 Oct 2018 09:24:24 -0700 (PDT) Message-ID: <4c96cb8e4d6f712ab62aac226d076a1d6b411e7e.camel@arm.com> From: Supreeth Venkatesh To: "Jin, Eric" , "edk2-devel@lists.01.org" Cc: "Wu, Jiaxin" Date: Wed, 31 Oct 2018 11:24:25 -0500 In-Reply-To: References: <20181030083836.7184-1-eric.jin@intel.com> <7350d7ed7b696e08585dceeb88d5f379620184ee.camel@arm.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.1 Mime-Version: 1.0 Subject: Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Oct 2018 16:24:25 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit 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 > Sent: Wednesday, October 31, 2018 1:00 AM > To: Jin, Eric ; edk2-devel@lists.01.org > Cc: Wu, Jiaxin ; supreeth.venkatesh@arm.com > Subject: Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail > of the HwErrRecVariableName > > Reviewed-by: Supreeth Venkatesh 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 > > Cc: Jiaxin Wu > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Jin > > --- > > .../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.
> > - Copyright (c) 2010 - 2012, Intel Corporation. All rights > > reserved.
> > + Copyright (c) 2010 - 2018, Intel Corporation. All rights > > reserved.
> > > > 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.
> > - Copyright (c) 2010 - 2016, Intel Corporation. All rights > > reserved.
> > + Copyright (c) 2010 - 2018, Intel Corporation. All rights > > reserved.
> > > > 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 > > // > > > >