Hi Philippe, I will split the change into two patches.
In the current implementation of VariableSmmRuntimeDxe.c there is not an actual problem. The Status variable is set to EFI_NOT_FOUND and RtPtrTrack is only used if Status is not set to an error code which requires the RtPtrTrack variable to be set. Given that this is a local variable, the initial value is undefined so I'd prefer to solve this by initializing the variable value. This will better accommodate future changes to the function by having a reasonable default value. Regarding your suggestion, the expected behavior is that RtPtrTrack.CurrPtr is assigned in the for loop and the value is used in the if block after the for block so jumping to the Done label would not be appropriate. The variable would also still be uninitialized. Thanks, Michael > -----Original Message----- > From: Philippe Mathieu-Daudé <phi...@redhat.com> > Sent: Thursday, November 21, 2019 8:16 AM > To: devel@edk2.groups.io; Kubacki, Michael A > <michael.a.kuba...@intel.com> > Cc: Gao, Liming <liming....@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Wu, > Hao A <hao.a...@intel.com> > Subject: Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize > local variable > > 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....@intel.com> > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > > Cc: Jian J Wang <jian.j.w...@intel.com> > > Cc: Hao A Wu <hao.a...@intel.com> > > Signed-off-by: Michael Kubacki <michael.a.kuba...@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/VariableSmmRuntimeDx > e.c > > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e.c > > index d525998ae3..2cf0ed32ae 100644 > > --- > > > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e.c > > +++ > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e > > +++ .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/VariableSmmRuntimeDx > e.c > +++ > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e.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 <phi...@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#51109): https://edk2.groups.io/g/devel/message/51109 Mute This Topic: https://groups.io/mt/60998486/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
