Re: [PATCH 2/2] MdeModulePkg Variable: Correct the returned EFI_STATUS in the UpdateVariableStore().

["Zeng, Star" <star.zeng@intel.com>]

My understanding is GetFvbInfoByAddress() is to *Get* something, then EFI_NOT_FOUND is better.
But UpdateVariableStore() is to *Update* something, I think EFI_UNSUPPORTED is better.


Thanks,
Star
-----Original Message-----
From: Shia, Cinnamon [mailto:cinnamon.shia@hpe.com] 
Sent: Friday, May 11, 2018 4:43 PM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: lersek@redhat.com; Huang, Ansen <ansen.huang@hpe.com>
Subject: RE: [PATCH 2/2] MdeModulePkg Variable: Correct the returned EFI_STATUS in the UpdateVariableStore().

Hi Star,

Thanks for your comments.

About returning EFI_NOT_FOUND for the case (Fvb == NULL), the idea is from GetFvbInfoByAddress().
Do you think we should apply the same logic to GetFvbInfoByAddress()?

Thanks,
Cinnamon Shia

-----Original Message-----
From: Zeng, Star [mailto:star.zeng@intel.com] 
Sent: Friday, May 11, 2018 4:37 PM
To: Shia, Cinnamon <cinnamon.shia@hpe.com>; edk2-devel@lists.01.org
Cc: lersek@redhat.com; Huang, Ansen <ansen.huang@hpe.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH 2/2] MdeModulePkg Variable: Correct the returned EFI_STATUS in the UpdateVariableStore().

Good patch. I have two minor comments.
1. Please reduce the title length to <= 72 chars as I know it is requirement for patch check (PatchCheck.py in BaseTools) and push to server.
2. I prefer to return EFI_UNSUPPORTED instead of EFI_NOT_FOUND for the case (Fvb == NULL).

@retval EFI_UNSUPPORTED          Fvb is a NULL for Non-Volatile variable update.


Thanks,
Star
-----Original Message-----
From: cinnamon shia [mailto:cinnamon.shia@hpe.com] 
Sent: Friday, May 11, 2018 12:18 PM
To: edk2-devel@lists.01.org
Cc: lersek@redhat.com; Zeng, Star <star.zeng@intel.com>; cinnamon shia <cinnamon.shia@hpe.com>; Ansen Huang <ansen.huang@hpe.com>
Subject: [PATCH 2/2] MdeModulePkg Variable: Correct the returned EFI_STATUS in the UpdateVariableStore().

If Fvb is a NULL, EFI_NOT_FOUND should be returned.
If the remaining size is not enough, EFI_OUT_OF_RESOURCES should be returned.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: cinnamon shia <cinnamon.shia@hpe.com>
Signed-off-by: Ansen Huang <ansen.huang@hpe.com>
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
index 7303681aaa..fc10cd9e18 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -238,6 +238,8 @@ IsValidVariableHeader (
   @param Buffer                  Pointer to the buffer from which data is written.
 
   @retval EFI_INVALID_PARAMETER  Parameters not valid.
+  @retval EFI_NOT_FOUND          Fvb is a NULL.
+  @retval EFI_OUT_OF_RESOURCES   The remaining size is not enough.
   @retval EFI_SUCCESS            Variable store successfully updated.
 
 **/
@@ -274,7 +276,7 @@ UpdateVariableStore (
   //
   if (!Volatile) {
     if (Fvb == NULL) {
-      return EFI_INVALID_PARAMETER;
+      return EFI_NOT_FOUND;
     }
     Status = Fvb->GetPhysicalAddress(Fvb, &FvVolHdr);
     ASSERT_EFI_ERROR (Status);
@@ -289,7 +291,7 @@ UpdateVariableStore (
     }
 
     if ((DataPtr + DataSize) > ((EFI_PHYSICAL_ADDRESS) (UINTN) ((UINT8 *) FwVolHeader + FwVolHeader->FvLength))) {
-      return EFI_INVALID_PARAMETER;
+      return EFI_OUT_OF_RESOURCES;
     }
   } else {
     //
@@ -302,7 +304,7 @@ UpdateVariableStore (
     }
 
     if ((DataPtr + DataSize) > ((UINTN) ((UINT8 *) VolatileBase + VolatileBase->Size))) {
-      return EFI_INVALID_PARAMETER;
+      return EFI_OUT_OF_RESOURCES;
     }
 
     //
-- 
2.16.1.windows.4