From: "Heinrich Schuchardt" <xypron.glpk@gmx.de>
To: Grant Likely <grant.likely@arm.com>, devel@edk2.groups.io
Cc: nd@arm.com, G Edhaya Chandran <Edhaya.Chandran@arm.com>,
Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>,
Eric Jin <eric.jin@intel.com>
Subject: Re: [PATCH] Check return status on calls to GetTime()
Date: Sat, 1 Aug 2020 15:54:20 +0200 [thread overview]
Message-ID: <117e027c-3389-1b0c-4fb3-33418bf7976f@gmx.de> (raw)
In-Reply-To: <20200731171949.15748-1-grant.likely@arm.com>
On 7/31/20 7:19 PM, Grant Likely wrote:
> Not all platforms implement GetTime(), but the SCT just assumes calls to
> GetTime will be successful. If GetTime() doesn't return EFI_SUCCESS,
> then the EFI_TIME value will be uninitialized data.
>
> Fix by checking the GetTime() return code. If it doesn't return
> EFI_SUCCESS, then use the traditional 1/1/1970 epoch so that the test
> report at least looks sane, but it is obvious that we don't have a valid
> timestamp.
>
> Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=2870
>
> Cc: G Edhaya Chandran <Edhaya.Chandran@arm.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
> Signed-off-by: Grant Likely <grant.likely@arm.com>
> ---
> .../SimpleNetwork/SimpleNetworkENTSTestCase.c | 26 +++++++++++++------
> .../MiscBootServicesBBTestFunction.c | 8 ++++--
> .../DriverBindingBBTestFunction.c | 5 +++-
> .../SCT/Drivers/StandardTest/StandardTest.c | 11 +++++---
> .../Framework/ENTS/EasDispatcher/Core/Eas.c | 9 +++++--
> .../ENTS/EasDispatcher/Exec/EasCmdDisp.c | 20 +++++++++-----
> 6 files changed, 57 insertions(+), 22 deletions(-)
>
> diff --git a/uefi-sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetworkENTSTestCase.c b/uefi-sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetworkENTSTestCase.c
> index 9c8d2a70..5579be7e 100644
> --- a/uefi-sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetworkENTSTestCase.c
> +++ b/uefi-sct/SctPkg/TestCase/RIVL/Protocol/SimpleNetwork/SimpleNetworkENTSTestCase.c
> @@ -24,6 +24,8 @@ Abstract:
>
> #include "SimpleNetworkENTSTestCase.h"
>
> +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> +
> //
> // SimpleNetwork.Start
> //
> @@ -928,7 +930,8 @@ Returns:
> Status = EFI_SUCCESS;
> tBS->Stall (5000);
>
> - tRT->GetTime (&BeginTime, NULL);
> + if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> + BeginTime = Epoch;
> for (Index = 0; Index < 1;) {
> Status = SimpleNetwork->Transmit (
> SimpleNetwork,
> @@ -964,7 +967,8 @@ Returns:
> }
> }
>
> - tRT->GetTime (&BeginTime, NULL);
> + if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> + BeginTime = Epoch;
>
> for (Index = 1; Index < TransmitPattern1Number;) {
> Status = SimpleNetwork->Transmit (
> @@ -1002,7 +1006,8 @@ Returns:
> }
>
> End:
> - tRT->GetTime (&EndTime, NULL);
> + if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> + EndTime = Epoch;
>
> *TransmitPattern1Status = Status;
>
> @@ -1125,7 +1130,8 @@ Returns:
> Status = EFI_SUCCESS;
> tBS->Stall (5000);
>
> - tRT->GetTime (&BeginTime, NULL);
> + if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> + BeginTime = Epoch;
> for (Index = 0; Index < 1;) {
> Status = SimpleNetwork->Transmit (
> SimpleNetwork,
> @@ -1161,7 +1167,8 @@ Returns:
> }
> }
>
> - tRT->GetTime (&BeginTime, NULL);
> + if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> + BeginTime = Epoch;
>
> for (Index = 1; Index < TransmitPattern2Number;) {
> Status = SimpleNetwork->Transmit (
> @@ -1199,7 +1206,8 @@ Returns:
> }
>
> End:
> - tRT->GetTime (&EndTime, NULL);
> + if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> + EndTime = Epoch;
>
> *TransmitPattern1Status = Status;
>
> @@ -1326,7 +1334,8 @@ Returns:
> }
> }
>
> - tRT->GetTime (&BeginTime, NULL);
> + if (tRT->GetTime (&BeginTime, NULL) != EFI_SUCCESS)
> + BeginTime = Epoch;
>
> for (Index = 1; Index < ReceivePattern1Number;) {
> *ReceivePattern1BufferSize = BufferSizeOrg;
> @@ -1346,7 +1355,8 @@ Returns:
> }
> }
>
> - tRT->GetTime (&EndTime, NULL);
> + if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> + EndTime = Epoch;
>
> *ReceivePattern1Status = Status;
>
> diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/BlackBoxTest/MiscBootServicesBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/BlackBoxTest/MiscBootServicesBBTestFunction.c
> index 1d231d8c..3a530282 100644
> --- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/BlackBoxTest/MiscBootServicesBBTestFunction.c
> +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/BootServices/MiscBootServices/BlackBoxTest/MiscBootServicesBBTestFunction.c
> @@ -27,6 +27,8 @@ Abstract:
> #include "SctLib.h"
I tried to apply your patch with 'git am --keep-cr'. This chunk has a
problem.
While all other lines in the file have CR LF line endings this very line
has only LF in EDK2 master. But your patch has CR LF for the line.
Did you use Windows and not Linux to create the patch?
I could apply your patch after removing the extraneous CR.
With your patch applied if GetTime() returns EFI_UNSUPPORTED, the test
log is not garbled anymore:
SetVariable_Conf:
Test Finished: 01/01/70 12:00a
Elapsed Time: 00 Days 00:00:00
If GetTime() is available:
SetVariable_Conf:
Test Finished: 08/01/20 01:52p
Elapsed Time: 00 Days 00:00:04
Except for the line-endings:
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> #include "MiscBootServicesBBTestMain.h"
>
> +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> +
> /**
> * Entrypoint for gtBS->SetWatchdogTimer() Interface Test.
> * @param This a pointer of EFI_BB_TEST_PROTOCOL.
> @@ -821,13 +823,15 @@ BBTestStallInterfaceTest (
> //
> // 4.2.2.1 Stall must succeed.
> //
> - gtRT->GetTime (&StartTime, NULL);
> + if (gtRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> + StartTime = Epoch;
> OldTpl = gtBS->RaiseTPL (TplArray[Index]);
> Status = gtBS->Stall (
> 10000000
> );
> gtBS->RestoreTPL (OldTpl);
> - gtRT->GetTime (&EndTime, NULL);
> + if (gtRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> + EndTime = Epoch;
> if (Status == EFI_SUCCESS) {
> AssertionType = EFI_TEST_ASSERTION_PASSED;
> } else {
> diff --git a/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBoxTest/DriverBindingBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBoxTest/DriverBindingBBTestFunction.c
> index bf675feb..4ab52dcd 100644
> --- a/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBoxTest/DriverBindingBBTestFunction.c
> +++ b/uefi-sct/SctPkg/TestCase/UEFI/IHV/Protocol/DriverBinding/BlackBoxTest/DriverBindingBBTestFunction.c
> @@ -36,6 +36,8 @@ static const UINTN MonthLengths[2][12] = {
> { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }
> };
>
> +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> +
> #define MINS_PER_HOUR 60
> #define HOURS_PER_DAY 24
> #define SECS_PER_MIN 60
> @@ -1052,7 +1054,8 @@ EndLogging (
> WriteLogFile (Private, DashLine, SYSTEMLOG);
> WriteLogFile (Private, DashLine, CASELOG);
>
> - gtRT->GetTime (&CurrentTime, NULL);
> + if (gtRT->GetTime (&CurrentTime, NULL) != EFI_SUCCESS)
> + CurrentTime = Epoch;
> DBSPrint (Buffer, EFI_MAX_PRINT_BUFFER, L"Test Finished: %t\n", &CurrentTime);
>
> WriteLogFile (Private, Buffer, SYSTEMLOG);
> diff --git a/uefi-sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/StandardTest.c b/uefi-sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/StandardTest.c
> index 84025457..836f072a 100644
> --- a/uefi-sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/StandardTest.c
> +++ b/uefi-sct/SctPkg/TestInfrastructure/SCT/Drivers/StandardTest/StandardTest.c
> @@ -30,6 +30,8 @@ Abstract:
> #include "StandardTest.h"
> #include <Library/EntsLib.h>
>
> +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> +
> //
> // Prototypes
> //
> @@ -1081,7 +1083,8 @@ Returns:
> StslWriteLogFile (Private, Buffer);
>
> CurrentTime = &Private->StartTime;
> - tRT->GetTime (CurrentTime, NULL);
> + if (tRT->GetTime (CurrentTime, NULL) != EFI_SUCCESS)
> + *CurrentTime = Epoch;
>
> } else {
> StslWriteLogFile (Private, DashLine);
> @@ -1118,7 +1121,8 @@ Returns:
>
> StslWriteLogFileName (Private);
> CurrentTime = &Private->StartTime;
> - tRT->GetTime (CurrentTime, NULL);
> + if (tRT->GetTime (CurrentTime, NULL) != EFI_SUCCESS)
> + *CurrentTime = Epoch;
> SctSPrint (Buffer, EFI_MAX_PRINT_BUFFER, L"Test Started: %t\n", CurrentTime);
> StslWriteLogFile (Private, Buffer);
>
> @@ -1238,7 +1242,8 @@ Returns:
>
> StslWriteLogFileName (Private);
>
> - tRT->GetTime (&CurrentTime, NULL);
> + if (tRT->GetTime (&CurrentTime, NULL) != EFI_SUCCESS)
> + CurrentTime = Epoch;
>
> SecondsElapsed = SecondsElapsedFromBaseYear (
> Private->StartTime.Year,
> diff --git a/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Core/Eas.c b/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Core/Eas.c
> index 28f5ed4a..60b1c4dc 100644
> --- a/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Core/Eas.c
> +++ b/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Core/Eas.c
> @@ -23,9 +23,12 @@ Abstract:
> --*/
>
>
> +#include "Sct.h"
> #include "Sct.h"
> #include EFI_TEST_PROTOCOL_DEFINITION (EntsMonitorProtocol)
>
> +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> +
> STATIC
> EFI_STATUS
> AgentTestMain (
> @@ -310,7 +313,8 @@ DelaySctAgentCmdPost (
> }
> SctAgentCmdDelayedPost->CmdReturn = CmdReturn;
> SctAgentCmdDelayedPost->Cmd.ComdResult = CmdResult;
> - tRT->GetTime (&SctAgentCmdDelayedPost->StartTime, NULL);
> + if (tRT->GetTime (&SctAgentCmdDelayedPost->StartTime, NULL) != EFI_SUCCESS)
> + SctAgentCmdDelayedPost->StartTime = Epoch;
>
> return Status;
> }
> @@ -327,7 +331,8 @@ PostSctAgentDelayedCmd (
> return EFI_SUCCESS;
> }
>
> - tRT->GetTime (&SctAgentCmdDelayedPost->EndTime, NULL);
> + if (tRT->GetTime (&SctAgentCmdDelayedPost->EndTime, NULL) != EFI_SUCCESS)
> + SctAgentCmdDelayedPost->EndTime = Epoch;
>
> Status = RecordMessage (
> &SctAgentCmdDelayedPost->Cmd.ComdRuntimeInfo,
> diff --git a/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Exec/EasCmdDisp.c b/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Exec/EasCmdDisp.c
> index 1ff6d569..cb6f08cf 100644
> --- a/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Exec/EasCmdDisp.c
> +++ b/uefi-sct/SctPkg/TestInfrastructure/SCT/Framework/ENTS/EasDispatcher/Exec/EasCmdDisp.c
> @@ -50,6 +50,8 @@ Abstract:
>
> EFI_CPU_ARCH_PROTOCOL *Cpu = NULL;
>
> +static EFI_TIME Epoch = { .Year = 1970, .Month = 1, .Day = 1 };
> +
> //
> // Local Function Definition
> //
> @@ -132,9 +134,11 @@ Returns:
> //
> // Perform EFTP operation.
> //
> - tRT->GetTime (&StartTime, NULL);
> + if (tRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> + StartTime = Epoch;
> Status = EftpDispatchFileTransferComd (FileCmdType);
> - tRT->GetTime (&EndTime, NULL);
> + if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> + EndTime = Epoch;
>
> if (Status == EFI_OUT_OF_RESOURCES) {
> return EFI_OUT_OF_RESOURCES;
> @@ -365,9 +369,11 @@ Returns:
> //
> // Execute Shell Command
> //
> - tRT->GetTime (&StartTime, NULL);
> + if (tRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> + StartTime = Epoch;
> Status = SctShellExecute (&mImageHandle, (gEasFT->Cmd)->ComdArg, FALSE, NULL, NULL);;
> - tRT->GetTime (&EndTime, NULL);
> + if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> + EndTime = Epoch;
> EFI_ENTS_DEBUG ((EFI_ENTS_D_TRACE, L"dispatch:(%s)", (gEasFT->Cmd)->ComdArg));
> SctPrint (L"dispatch:(%s) - %r\n", (gEasFT->Cmd)->ComdArg, Status);
> if (Status == EFI_OUT_OF_RESOURCES) {
> @@ -1483,9 +1489,11 @@ Returns:
> //
> // Resume SCT execution by executing "sct -c" in sct passive mode.
> //
> - tRT->GetTime (&StartTime, NULL);
> + if (tRT->GetTime (&StartTime, NULL) != EFI_SUCCESS)
> + StartTime = Epoch;
> Status = SctShellExecute (&mImageHandle, (gEasFT->Cmd)->ComdArg, FALSE, NULL, NULL);;
> - tRT->GetTime (&EndTime, NULL);
> + if (tRT->GetTime (&EndTime, NULL) != EFI_SUCCESS)
> + EndTime = Epoch;
> EFI_ENTS_DEBUG ((EFI_ENTS_D_TRACE, L"dispatch:(%s)", (gEasFT->Cmd)->ComdArg));
> SctPrint (L"dispatch:(%s) - %r\n", (gEasFT->Cmd)->ComdArg, Status);
> if (Status == EFI_OUT_OF_RESOURCES) {
>
next prev parent reply other threads:[~2020-08-01 13:54 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-31 17:19 [PATCH] Check return status on calls to GetTime() Grant Likely
2020-08-01 13:54 ` Heinrich Schuchardt [this message]
2020-08-04 15:46 ` Grant Likely
2020-08-04 11:26 ` Samer El-Haj-Mahmoud
2020-08-06 13:31 ` Samer El-Haj-Mahmoud
2020-08-06 14:08 ` Heinrich Schuchardt
2020-08-11 19:50 ` [edk2-devel] " G Edhaya Chandran
2020-08-11 20:00 ` G Edhaya Chandran
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=117e027c-3389-1b0c-4fb3-33418bf7976f@gmx.de \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox