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


Supreeth Venkatesh
 

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
//

Join devel@edk2.groups.io to automatically receive all group messages.