public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
@ 2019-11-21  2:32 Kubacki, Michael A
  2019-11-21  2:52 ` [edk2-devel] " Liming Gao
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Kubacki, Michael A @ 2019-11-21  2:32 UTC (permalink / raw)
  To: devel; +Cc: Liming Gao, Michael D Kinney, Jian J Wang, Hao A Wu

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
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));
+
   // 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
-- 
2.16.2.windows.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
  2019-11-21  2:32 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable Kubacki, Michael A
@ 2019-11-21  2:52 ` Liming Gao
  2019-11-21  3:26 ` Wang, Jian J
  2019-11-21 16:16 ` [edk2-devel] " Philippe Mathieu-Daudé
  2 siblings, 0 replies; 6+ messages in thread
From: Liming Gao @ 2019-11-21  2:52 UTC (permalink / raw)
  To: devel@edk2.groups.io, Kubacki, Michael A
  Cc: Kinney, Michael D, Wang, Jian J, Wu, Hao A

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>Kubacki, Michael A
>Sent: Thursday, November 21, 2019 10:33 AM
>To: devel@edk2.groups.io
>Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
><michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
>Hao A <hao.a.wu@intel.com>
>Subject: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local
>variable
>
>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
>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));
>+
>   // 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/VariableSmmRuntimeDx
>e.c
>index d525998ae3..2cf0ed32ae 100644
>---
>a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe
>.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
>--
>2.16.2.windows.1
>
>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
  2019-11-21  2:32 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable Kubacki, Michael A
  2019-11-21  2:52 ` [edk2-devel] " Liming Gao
@ 2019-11-21  3:26 ` Wang, Jian J
  2019-11-21 16:16 ` [edk2-devel] " Philippe Mathieu-Daudé
  2 siblings, 0 replies; 6+ messages in thread
From: Wang, Jian J @ 2019-11-21  3:26 UTC (permalink / raw)
  To: Kubacki, Michael A, devel@edk2.groups.io
  Cc: Gao, Liming, Kinney, Michael D, Wu, Hao A



Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> Sent: Thursday, November 21, 2019 10:33 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
> A <hao.a.wu@intel.com>
> Subject: [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
> 
> 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
> 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));
> +
>    // 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
> --
> 2.16.2.windows.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
  2019-11-21  2:32 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable Kubacki, Michael A
  2019-11-21  2:52 ` [edk2-devel] " Liming Gao
  2019-11-21  3:26 ` Wang, Jian J
@ 2019-11-21 16:16 ` Philippe Mathieu-Daudé
  2019-11-21 19:22   ` Kubacki, Michael A
  2 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-21 16:16 UTC (permalink / raw)
  To: devel, michael.a.kubacki
  Cc: Liming Gao, Michael D Kinney, Jian J Wang, Hao A Wu

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>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
  2019-11-21 16:16 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2019-11-21 19:22   ` Kubacki, Michael A
  2019-11-22 11:22     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 6+ messages in thread
From: Kubacki, Michael A @ 2019-11-21 19:22 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, devel@edk2.groups.io
  Cc: Gao, Liming, Kinney, Michael D, Wang, Jian J, Wu, Hao A

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é <philmd@redhat.com>
> Sent: Thursday, November 21, 2019 8:16 AM
> To: devel@edk2.groups.io; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
> Hao A <hao.a.wu@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.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/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 <philmd@redhat.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable
  2019-11-21 19:22   ` Kubacki, Michael A
@ 2019-11-22 11:22     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-22 11:22 UTC (permalink / raw)
  To: Kubacki, Michael A, devel@edk2.groups.io
  Cc: Gao, Liming, Kinney, Michael D, Wang, Jian J, Wu, Hao A

On 11/21/19 8:22 PM, Kubacki, Michael A wrote:
> Hi Philippe,
> 
> I will split the change into two patches.

Thanks.

> 
> 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.

Yes you are right.

> 
> Thanks,
> Michael
> 
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Sent: Thursday, November 21, 2019 8:16 AM
>> To: devel@edk2.groups.io; Kubacki, Michael A
>> <michael.a.kubacki@intel.com>
>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu,
>> Hao A <hao.a.wu@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.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/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 <philmd@redhat.com>
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-11-22 11:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-21  2:32 [PATCH V1 1/1] MdeModulePkg/Variable: Initialize local variable Kubacki, Michael A
2019-11-21  2:52 ` [edk2-devel] " Liming Gao
2019-11-21  3:26 ` Wang, Jian J
2019-11-21 16:16 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-11-21 19:22   ` Kubacki, Michael A
2019-11-22 11:22     ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox