* [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim()
@ 2020-07-10 3:04 Michael D Kinney
2020-07-14 2:19 ` Wu, Hao A
2020-07-14 2:28 ` [edk2-devel] " Wang, Jian J
0 siblings, 2 replies; 3+ messages in thread
From: Michael D Kinney @ 2020-07-10 3:04 UTC (permalink / raw)
To: devel; +Cc: Hao A Wu, Liming Gao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2844
Update Reclaim() to return the error status from the reclaim
operation and not the status of SynchronizeRuntimeVariableCache()
that can be EFI_SUCCESS even through the status from reclaim
is an error. Without this change, the return status from
SetVariable() can be EFI_SUCCESS even though the variable was
not actually set. This occurs if the variable store is full
and a Reclaim() is invoked to free up space and even after all
possible space is freed, there is still not enough room for
the variable being set. This condition should return
EFI_OUT_OF_RESOURCES.
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
---
.../Universal/Variable/RuntimeDxe/Variable.c | 30 +++++++++++--------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 1e71fc642c..41f8ff4ceb 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -531,6 +531,7 @@ Reclaim (
VOID *Point1;
BOOLEAN FoundAdded;
EFI_STATUS Status;
+ EFI_STATUS DoneStatus;
UINTN CommonVariableTotalSize;
UINTN CommonUserVariableTotalSize;
UINTN HwErrVariableTotalSize;
@@ -774,25 +775,30 @@ Reclaim (
}
Done:
+ DoneStatus = EFI_SUCCESS;
if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
- Status = SynchronizeRuntimeVariableCache (
- &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
- 0,
- VariableStoreHeader->Size
- );
- ASSERT_EFI_ERROR (Status);
+ DoneStatus = SynchronizeRuntimeVariableCache (
+ &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
+ 0,
+ VariableStoreHeader->Size
+ );
+ ASSERT_EFI_ERROR (DoneStatus);
FreePool (ValidBuffer);
} else {
//
// For NV variable reclaim, we use mNvVariableCache as the buffer, so copy the data back.
//
CopyMem (mNvVariableCache, (UINT8 *) (UINTN) VariableBase, VariableStoreHeader->Size);
- Status = SynchronizeRuntimeVariableCache (
- &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
- 0,
- VariableStoreHeader->Size
- );
- ASSERT_EFI_ERROR (Status);
+ DoneStatus = SynchronizeRuntimeVariableCache (
+ &mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
+ 0,
+ VariableStoreHeader->Size
+ );
+ ASSERT_EFI_ERROR (DoneStatus);
+ }
+
+ if (!EFI_ERROR (Status) && EFI_ERROR (DoneStatus)) {
+ Status = DoneStatus;
}
return Status;
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim()
2020-07-10 3:04 [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim() Michael D Kinney
@ 2020-07-14 2:19 ` Wu, Hao A
2020-07-14 2:28 ` [edk2-devel] " Wang, Jian J
1 sibling, 0 replies; 3+ messages in thread
From: Wu, Hao A @ 2020-07-14 2:19 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io; +Cc: Gao, Liming
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, July 10, 2020 11:05 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from
> Reclaim()
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2844
>
> Update Reclaim() to return the error status from the reclaim operation and
> not the status of SynchronizeRuntimeVariableCache() that can be
> EFI_SUCCESS even through the status from reclaim is an error. Without this
> change, the return status from
> SetVariable() can be EFI_SUCCESS even though the variable was not actually
> set. This occurs if the variable store is full and a Reclaim() is invoked to free
> up space and even after all possible space is freed, there is still not enough
> room for the variable being set. This condition should return
> EFI_OUT_OF_RESOURCES.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
> .../Universal/Variable/RuntimeDxe/Variable.c | 30 +++++++++++--------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 1e71fc642c..41f8ff4ceb 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -531,6 +531,7 @@ Reclaim (
> VOID *Point1;
> BOOLEAN FoundAdded;
> EFI_STATUS Status;
> + EFI_STATUS DoneStatus;
> UINTN CommonVariableTotalSize;
> UINTN CommonUserVariableTotalSize;
> UINTN HwErrVariableTotalSize;
> @@ -774,25 +775,30 @@ Reclaim (
> }
>
> Done:
> + DoneStatus = EFI_SUCCESS;
> if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
> - Status = SynchronizeRuntimeVariableCache (
> - &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCach
> e,
> - 0,
> - VariableStoreHeader->Size
> - );
> - ASSERT_EFI_ERROR (Status);
> + DoneStatus = SynchronizeRuntimeVariableCache (
> + &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCach
> e,
> + 0,
> + VariableStoreHeader->Size
> + );
> + ASSERT_EFI_ERROR (DoneStatus);
> FreePool (ValidBuffer);
> } else {
> //
> // For NV variable reclaim, we use mNvVariableCache as the buffer, so
> copy the data back.
> //
> CopyMem (mNvVariableCache, (UINT8 *) (UINTN) VariableBase,
> VariableStoreHeader->Size);
> - Status = SynchronizeRuntimeVariableCache (
> - &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> - 0,
> - VariableStoreHeader->Size
> - );
> - ASSERT_EFI_ERROR (Status);
> + DoneStatus = SynchronizeRuntimeVariableCache (
> + &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> + 0,
> + VariableStoreHeader->Size
> + );
> + ASSERT_EFI_ERROR (DoneStatus);
> + }
> +
> + if (!EFI_ERROR (Status) && EFI_ERROR (DoneStatus)) {
> + Status = DoneStatus;
> }
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>
Best Regards,
Hao Wu
>
> return Status;
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [edk2-devel] [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim()
2020-07-10 3:04 [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim() Michael D Kinney
2020-07-14 2:19 ` Wu, Hao A
@ 2020-07-14 2:28 ` Wang, Jian J
1 sibling, 0 replies; 3+ messages in thread
From: Wang, Jian J @ 2020-07-14 2:28 UTC (permalink / raw)
To: devel@edk2.groups.io, Kinney, Michael D; +Cc: Wu, Hao A, Gao, Liming
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>
Regards,
Jian
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael D
> Kinney
> Sent: Friday, July 10, 2020 11:05 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return
> status from Reclaim()
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2844
>
> Update Reclaim() to return the error status from the reclaim
> operation and not the status of SynchronizeRuntimeVariableCache()
> that can be EFI_SUCCESS even through the status from reclaim
> is an error. Without this change, the return status from
> SetVariable() can be EFI_SUCCESS even though the variable was
> not actually set. This occurs if the variable store is full
> and a Reclaim() is invoked to free up space and even after all
> possible space is freed, there is still not enough room for
> the variable being set. This condition should return
> EFI_OUT_OF_RESOURCES.
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
> ---
> .../Universal/Variable/RuntimeDxe/Variable.c | 30 +++++++++++--------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 1e71fc642c..41f8ff4ceb 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -531,6 +531,7 @@ Reclaim (
> VOID *Point1;
> BOOLEAN FoundAdded;
> EFI_STATUS Status;
> + EFI_STATUS DoneStatus;
> UINTN CommonVariableTotalSize;
> UINTN CommonUserVariableTotalSize;
> UINTN HwErrVariableTotalSize;
> @@ -774,25 +775,30 @@ Reclaim (
> }
>
> Done:
> + DoneStatus = EFI_SUCCESS;
> if (IsVolatile || mVariableModuleGlobal->VariableGlobal.EmuNvMode) {
> - Status = SynchronizeRuntimeVariableCache (
> - &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
> - 0,
> - VariableStoreHeader->Size
> - );
> - ASSERT_EFI_ERROR (Status);
> + DoneStatus = SynchronizeRuntimeVariableCache (
> + &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache,
> + 0,
> + VariableStoreHeader->Size
> + );
> + ASSERT_EFI_ERROR (DoneStatus);
> FreePool (ValidBuffer);
> } else {
> //
> // For NV variable reclaim, we use mNvVariableCache as the buffer, so copy
> the data back.
> //
> CopyMem (mNvVariableCache, (UINT8 *) (UINTN) VariableBase,
> VariableStoreHeader->Size);
> - Status = SynchronizeRuntimeVariableCache (
> - &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> - 0,
> - VariableStoreHeader->Size
> - );
> - ASSERT_EFI_ERROR (Status);
> + DoneStatus = SynchronizeRuntimeVariableCache (
> + &mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> + 0,
> + VariableStoreHeader->Size
> + );
> + ASSERT_EFI_ERROR (DoneStatus);
> + }
> +
> + if (!EFI_ERROR (Status) && EFI_ERROR (DoneStatus)) {
> + Status = DoneStatus;
> }
>
> return Status;
> --
> 2.21.0.windows.1
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-14 2:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-10 3:04 [Patch] MdeModulePkg/Variable/RuntimeDxe: Fix return status from Reclaim() Michael D Kinney
2020-07-14 2:19 ` Wu, Hao A
2020-07-14 2:28 ` [edk2-devel] " Wang, Jian J
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox