* [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName
@ 2018-10-30 8:38 Eric Jin
2018-10-30 10:43 ` Leif Lindholm
2018-10-30 16:59 ` Supreeth Venkatesh
0 siblings, 2 replies; 7+ messages in thread
From: Eric Jin @ 2018-10-30 8:38 UTC (permalink / raw)
To: edk2-devel; +Cc: Supreeth Venkatesh, Jiaxin Wu
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/BlackBoxTest/VariableServicesBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c
index d1064ce..df1bbe7 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/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[HwErrRecVariableNameLength];
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 );
+ 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;
- SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, 12 );
- RecoveryLib->WriteResetRecord( RecoveryLib, 13*sizeof(CHAR16)+2, RecoveryData );
+ SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, HwErrRecVariableNameLength-1 );
+ RecoveryLib->WriteResetRecord( RecoveryLib, HwErrRecVariableNameLength*sizeof(CHAR16)+2, RecoveryData );
//
// 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), HwErrRecVariableNameLength-1 );
Status = RT->GetVariable (
HwErrRecVariableName,
&gHwErrRecGuid,
diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h
index 051ae6f..b645b55 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/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.
+//
+#define HwErrRecVariableNameLength 13
+
+//
// Global Variables
//
--
2.9.0.windows.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName
2018-10-30 8:38 [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName Eric Jin
@ 2018-10-30 10:43 ` Leif Lindholm
2018-10-30 16:59 ` Supreeth Venkatesh
1 sibling, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2018-10-30 10:43 UTC (permalink / raw)
To: Eric Jin; +Cc: edk2-devel, Jiaxin Wu
Hi Eric,
On Tue, Oct 30, 2018 at 04:38:36PM +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>
(Sidenote - I don't see Supreeth actually on cc on this patch.)
> 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(-)
Could you follow the steps outlined in Laszlo's excellent guide
https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
when generating the patches to send out?
For this specific patch, this would have made two major differences:
1) We would have seen the full path to files in the summary above,
instead of the .../.
2) It would have ordered the files such that the .h would have been
shown before the .c.
2 turns out to make a major difference when reviewing this particular
patch, because...
> diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c
> index d1064ce..df1bbe7 100644
> --- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c
> +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/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[HwErrRecVariableNameLength];
... the first thing I see when I look at this patch is
"Hang on, are we permitting runtime-sized arrays?"
This could have sent me off on a 15-90 minute task of
- triple-checking against the coding style document.
- search the mailing list (I recalled _some_ discussions around
this recently, but that may have been for Linux, and variable-length
arrays)
- search the edk2 codebase for precedent.
- testing with various toolchains whether they were happy compiling
it, getting surprised when it worked in some of them, and then spent
additional time figuring out why.
It could have, but it didn't. Because the patch is small enough that I
decided to read through it in its entirety before going off on
investigations.
More below.
> 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 );
> + 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;
> - SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, 12 );
> - RecoveryLib->WriteResetRecord( RecoveryLib, 13*sizeof(CHAR16)+2, RecoveryData );
> + SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, HwErrRecVariableNameLength-1 );
> + RecoveryLib->WriteResetRecord( RecoveryLib, HwErrRecVariableNameLength*sizeof(CHAR16)+2, RecoveryData );
>
> //
> // 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), HwErrRecVariableNameLength-1 );
> Status = RT->GetVariable (
> HwErrRecVariableName,
> &gHwErrRecGuid,
> diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h
> index 051ae6f..b645b55 100644
> --- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h
> +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/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.
> +//
> +#define HwErrRecVariableNameLength 13
But then I get here, and I see that HwErrRecVariableNameLength is a
statically defined macro. At which point my comment on this patch
narrows down to:
Please name macros appropriately.
HwErrRecVariableNameLength is the name of a variable.
The appropriate name for this macro would be
HW_ERR_ERC_VARIABLE_NAME_LENGTH.
This all according to section 4.3.5 - Type and Macro Names, in the
coding standards specification.
Regards,
Leif
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName
2018-10-30 8:38 [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName Eric Jin
2018-10-30 10:43 ` Leif Lindholm
@ 2018-10-30 16:59 ` Supreeth Venkatesh
2018-10-31 2:29 ` Jin, Eric
1 sibling, 1 reply; 7+ messages in thread
From: Supreeth Venkatesh @ 2018-10-30 16:59 UTC (permalink / raw)
To: Eric Jin, edk2-devel; +Cc: Jiaxin Wu, supreeth.venkatesh
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/BlackBo
> xTest/VariableServicesBBTestFunction.c b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c
> index d1064ce..df1bbe7 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> 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[HwErrRecVariableNameLen
> 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/BlackBo
> xTest/VariableServicesBBTestMain.h b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h
> index 051ae6f..b645b55 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> 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
> //
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName
2018-10-30 16:59 ` Supreeth Venkatesh
@ 2018-10-31 2:29 ` Jin, Eric
2018-10-31 16:24 ` Supreeth Venkatesh
0 siblings, 1 reply; 7+ messages in thread
From: Jin, Eric @ 2018-10-31 2:29 UTC (permalink / raw)
To: Supreeth Venkatesh, edk2-devel@lists.01.org; +Cc: Wu, Jiaxin, Jin, Eric
Hi Supreeth,
Thank for the comments.
I will re-create the patch to add the definition of the HwErrRecVariableNamePrefixLength(8) and HwErrRecVariableNameIndexLength(4).
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?
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/BlackBo
> xTest/VariableServicesBBTestFunction.c b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c
> index d1064ce..df1bbe7 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> 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[HwErrRecVariableNameLen
> 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/BlackBo
> xTest/VariableServicesBBTestMain.h b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h
> index 051ae6f..b645b55 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> 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
> //
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName
2018-10-31 2:29 ` Jin, Eric
@ 2018-10-31 16:24 ` Supreeth Venkatesh
2018-11-01 2:08 ` Jin, Eric
0 siblings, 1 reply; 7+ messages in thread
From: Supreeth Venkatesh @ 2018-10-31 16:24 UTC (permalink / raw)
To: Jin, Eric, edk2-devel@lists.01.org; +Cc: Wu, Jiaxin
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
> > //
> >
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName
2018-10-31 16:24 ` Supreeth Venkatesh
@ 2018-11-01 2:08 ` Jin, Eric
2018-11-01 20:06 ` Supreeth Venkatesh
0 siblings, 1 reply; 7+ messages in thread
From: Jin, Eric @ 2018-11-01 2:08 UTC (permalink / raw)
To: Supreeth Venkatesh, edk2-devel@lists.01.org; +Cc: Wu, Jiaxin
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
> > //
> >
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName
2018-11-01 2:08 ` Jin, Eric
@ 2018-11-01 20:06 ` Supreeth Venkatesh
0 siblings, 0 replies; 7+ messages in thread
From: Supreeth Venkatesh @ 2018-11-01 20:06 UTC (permalink / raw)
To: Jin, Eric, edk2-devel@lists.01.org; +Cc: Wu, Jiaxin
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-11-01 20:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-30 8:38 [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName Eric Jin
2018-10-30 10:43 ` Leif Lindholm
2018-10-30 16:59 ` Supreeth Venkatesh
2018-10-31 2:29 ` Jin, Eric
2018-10-31 16:24 ` Supreeth Venkatesh
2018-11-01 2:08 ` Jin, Eric
2018-11-01 20:06 ` Supreeth Venkatesh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox