Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Michael,

On 11/21/19 3:32 AM, Kubacki, Michael A wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2364
> 
> Fixes a new build warning in VS2012 introduced in f8ff4cca7c.
> 
> This patch initializes the local variable "Variable" in
> VariableServiceGetNextVariableInternal () and the local
> variable "RtPtrTrack" in FindVariableInRuntimeCache ().
> This enusres the pointers in the structures are initialized

Typo "this ensures"

> in the case no variable stores exist in the list of variable
> stores.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> ---
>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c       | 2 ++
>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c | 2 ++
>   2 files changed, 4 insertions(+)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> index d458f1c608..f6d187543d 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> @@ -551,6 +551,8 @@ VariableServiceGetNextVariableInternal (
>       return EFI_INVALID_PARAMETER;
>     }
>   
> +  ZeroMem (&Variable, sizeof (Variable));
> +

I agree with this change.

>     // Check if the variable exists in the given variable store list
>     for (StoreType = (VARIABLE_STORE_TYPE) 0; StoreType < VariableStoreTypeMax; StoreType++) {
>       if (VariableStoreList[StoreType] == NULL) {
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> index d525998ae3..2cf0ed32ae 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
> @@ -590,6 +590,8 @@ FindVariableInRuntimeCache (
>       return EFI_INVALID_PARAMETER;
>     }
>   
> +  ZeroMem (&RtPtrTrack, sizeof (RtPtrTrack));
> +
>     //
>     // The UEFI specification restricts Runtime Services callers from invoking the same or certain other Runtime Service
>     // functions prior to completion and return from a previous Runtime Service call. These restrictions prevent
> 

Here this seems overkill, what about:

-- >8 --
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.c
@@ -626,6 +626,10 @@ FindVariableInRuntimeCache (
        }
      }

+    if (RtPtrTrack.CurrPtr) {
+      goto Done;
+    }
+
      if (!EFI_ERROR (Status)) {
        //
        // Get data size

---

Can you split this patch in 2?
If so you can add to the 1st part:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>