Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-07-12 Thread Carsey, Jaben 
That sounds like the best way to proceed.

I do not think there will be other issues.

We will have to update both the library and the package GUID/Version since this 
is non backwards compatible.

-Jaben

> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, July 12, 2016 12:13 AM
> To: Carsey, Jaben <jaben.car...@intel.com>
> Cc: edk2-devel-01 <edk2-de...@ml01.01.org>; Laszlo Ersek
> <ler...@redhat.com>; Qiu, Shumin <shumin@intel.com>
> Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side
> effects in ASSERT_EFI_ERROR()
> Importance: High
> 
> Jaben,
> If and Else are using the same API.
> I would like to remove CommandInit() completely from the library and
> update the callers for code cleanup.
> Do you know whether there will be any potential issue for this update?
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: Carsey, Jaben
> > Sent: Thursday, July 7, 2016 10:42 PM
> > To: Ni, Ruiyu <ruiyu...@intel.com>
> > Cc: edk2-devel-01 <edk2-de...@ml01.01.org>; Laszlo Ersek
> > <ler...@redhat.com>; Qiu, Shumin <shumin....@intel.com>; Carsey,
> Jaben
> > <jaben.car...@intel.com>
> > Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side
> > effects in ASSERT_EFI_ERROR()
> >
> > We should see why If and Else are using this different API for sure.  I 
> > don't
> > remember why.
> >
> > I would be happy to remove it completely if there is no objection.
> >
> > -Jaben
> >
> > > -Original Message-
> > > From: Ni, Ruiyu
> > > Sent: Thursday, July 07, 2016 1:20 AM
> > > To: Carsey, Jaben <jaben.car...@intel.com>
> > > Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01  > > de...@ml01.01.org>; Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin
> > > <shumin@intel.com>
> > > Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with
> > > side effects in ASSERT_EFI_ERROR()
> > > Importance: High
> > >
> > > Jaben,
> > > CommandInit() API initializes gUnicodeCollation pointer.
> > > But ShellCommandLibConstructor() also initializes the
> > > gUnicodeCollation pointer.
> > >
> > > Can we just remove CommandInit() from this library? And update all
> callers?
> > >
> > > Thanks/Ray
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > > > Of Carsey, Jaben
> > > > Sent: Thursday, June 30, 2016 11:13 PM
> > > > To: Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin
> > > <shumin@intel.com>
> > > > Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01  > > > de...@ml01.01.org>
> > > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with
> > > > side effects in ASSERT_EFI_ERROR()
> > > >
> > > > Reviewed-by: Jaben Carsey <jaben.car...@intel.com>
> > > >
> > > > > -Original Message-
> > > > > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > > > > Sent: Thursday, June 30, 2016 4:07 AM
> > > > > To: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin
> > > > > <shumin@intel.com>
> > > > > Cc: edk2-devel-01 <edk2-de...@ml01.01.org>
> > > > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions
> > > > > with side effects in ASSERT_EFI_ERROR()
> > > > > Importance: High
> > > > >
> > > > > Jaben, Shumin,
> > > > >
> > > > > On 06/28/16 15:25, Laszlo Ersek wrote:
> > > > > > When ASSERT_EFI_ERROR() is compiled out, dependent on build
> > > > > > flags, only the status checking should be removed; the function
> > > > > > calls should
> > > > stay.
> > > > > >
> > > > > > Cc: Jaben Carsey <jaben.car...@intel.com>
> > > > > > Cc: Shumin Qiu <shumin@intel.com>
> > > > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > > > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > > > > > ---
> > > > > >
> > > > > > Notes:
> > > > > > build tested
> > > > > >
> > > > > >  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 -
> -
> &

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-07-12 Thread Ni, Ruiyu 
Jaben,
If and Else are using the same API.
I would like to remove CommandInit() completely from the library and update the 
callers for code cleanup.
Do you know whether there will be any potential issue for this update?

Thanks/Ray

> -Original Message-
> From: Carsey, Jaben
> Sent: Thursday, July 7, 2016 10:42 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>
> Cc: edk2-devel-01 <edk2-de...@ml01.01.org>; Laszlo Ersek
> <ler...@redhat.com>; Qiu, Shumin <shumin@intel.com>; Carsey, Jaben
> <jaben.car...@intel.com>
> Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side
> effects in ASSERT_EFI_ERROR()
> 
> We should see why If and Else are using this different API for sure.  I don't
> remember why.
> 
> I would be happy to remove it completely if there is no objection.
> 
> -Jaben
> 
> > -Original Message-
> > From: Ni, Ruiyu
> > Sent: Thursday, July 07, 2016 1:20 AM
> > To: Carsey, Jaben <jaben.car...@intel.com>
> > Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01  > de...@ml01.01.org>; Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin
> > <shumin@intel.com>
> > Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with
> > side effects in ASSERT_EFI_ERROR()
> > Importance: High
> >
> > Jaben,
> > CommandInit() API initializes gUnicodeCollation pointer.
> > But ShellCommandLibConstructor() also initializes the
> > gUnicodeCollation pointer.
> >
> > Can we just remove CommandInit() from this library? And update all callers?
> >
> > Thanks/Ray
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > > Of Carsey, Jaben
> > > Sent: Thursday, June 30, 2016 11:13 PM
> > > To: Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin
> > <shumin@intel.com>
> > > Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01  > > de...@ml01.01.org>
> > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with
> > > side effects in ASSERT_EFI_ERROR()
> > >
> > > Reviewed-by: Jaben Carsey <jaben.car...@intel.com>
> > >
> > > > -Original Message-
> > > > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > > > Sent: Thursday, June 30, 2016 4:07 AM
> > > > To: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin
> > > > <shumin@intel.com>
> > > > Cc: edk2-devel-01 <edk2-de...@ml01.01.org>
> > > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions
> > > > with side effects in ASSERT_EFI_ERROR()
> > > > Importance: High
> > > >
> > > > Jaben, Shumin,
> > > >
> > > > On 06/28/16 15:25, Laszlo Ersek wrote:
> > > > > When ASSERT_EFI_ERROR() is compiled out, dependent on build
> > > > > flags, only the status checking should be removed; the function
> > > > > calls should
> > > stay.
> > > > >
> > > > > Cc: Jaben Carsey <jaben.car...@intel.com>
> > > > > Cc: Shumin Qiu <shumin@intel.com>
> > > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > build tested
> > > > >
> > > > >  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
> > > > >  ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
> > > > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > Can I please get a maintainer review for this patch?
> > > >
> > > > Thanks
> > > > Laszlo
> > > >
> > > > > diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > > b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > > > index 7abfd8944b92..dc96bffde7d3 100644
> > > > > --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > > > +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > > > @@ -991,8 +991,11 @@ ShellCommandRunElse (
> > > > >IN EFI_SYSTEM_TABLE  *SystemTable
> > > > >)
> > > > >  {
> > > > > +  EFI_STATUS  Status;
> > > > >SCRIPT_FILE *CurrentScriptFile;
> > > > > -  ASSERT_EFI_ERROR(CommandInit());
>

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-07-07 Thread Carsey, Jaben 
We should see why If and Else are using this different API for sure.  I don't 
remember why.

I would be happy to remove it completely if there is no objection.

-Jaben

> -Original Message-
> From: Ni, Ruiyu
> Sent: Thursday, July 07, 2016 1:20 AM
> To: Carsey, Jaben <jaben.car...@intel.com>
> Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01  de...@ml01.01.org>; Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin
> <shumin....@intel.com>
> Subject: RE: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side
> effects in ASSERT_EFI_ERROR()
> Importance: High
> 
> Jaben,
> CommandInit() API initializes gUnicodeCollation pointer.
> But ShellCommandLibConstructor() also initializes the gUnicodeCollation
> pointer.
> 
> Can we just remove CommandInit() from this library? And update all callers?
> 
> Thanks/Ray
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Carsey, Jaben
> > Sent: Thursday, June 30, 2016 11:13 PM
> > To: Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin
> <shumin@intel.com>
> > Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01  > de...@ml01.01.org>
> > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side
> > effects in ASSERT_EFI_ERROR()
> >
> > Reviewed-by: Jaben Carsey <jaben.car...@intel.com>
> >
> > > -Original Message-
> > > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > > Sent: Thursday, June 30, 2016 4:07 AM
> > > To: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin
> > > <shumin@intel.com>
> > > Cc: edk2-devel-01 <edk2-de...@ml01.01.org>
> > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with
> > > side effects in ASSERT_EFI_ERROR()
> > > Importance: High
> > >
> > > Jaben, Shumin,
> > >
> > > On 06/28/16 15:25, Laszlo Ersek wrote:
> > > > When ASSERT_EFI_ERROR() is compiled out, dependent on build flags,
> > > > only the status checking should be removed; the function calls should
> > stay.
> > > >
> > > > Cc: Jaben Carsey <jaben.car...@intel.com>
> > > > Cc: Shumin Qiu <shumin@intel.com>
> > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > > > ---
> > > >
> > > > Notes:
> > > > build tested
> > > >
> > > >  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
> > > >  ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
> > > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > Can I please get a maintainer review for this patch?
> > >
> > > Thanks
> > > Laszlo
> > >
> > > > diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > > index 7abfd8944b92..dc96bffde7d3 100644
> > > > --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > > +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > > @@ -991,8 +991,11 @@ ShellCommandRunElse (
> > > >IN EFI_SYSTEM_TABLE  *SystemTable
> > > >)
> > > >  {
> > > > +  EFI_STATUS  Status;
> > > >SCRIPT_FILE *CurrentScriptFile;
> > > > -  ASSERT_EFI_ERROR(CommandInit());
> > > > +
> > > > +  Status = CommandInit ();
> > > > +  ASSERT_EFI_ERROR (Status);
> > > >
> > > >if (gEfiShellParametersProtocol->Argc > 1) {
> > > >  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> > > gShellLevel1HiiHandle, L"if");
> > > > @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
> > > >IN EFI_SYSTEM_TABLE  *SystemTable
> > > >)
> > > >  {
> > > > +  EFI_STATUS  Status;
> > > >SCRIPT_FILE *CurrentScriptFile;
> > > > -  ASSERT_EFI_ERROR(CommandInit());
> > > > +
> > > > +  Status = CommandInit ();
> > > > +  ASSERT_EFI_ERROR (Status);
> > > >
> > > >if (gEfiShellParametersProtocol->Argc > 1) {
> > > >  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> > > gShellLevel1HiiHandle, L"if");
> > > > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > > b/ShellPkg/Library/Uef

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-07-07 Thread Ni, Ruiyu 
Jaben,
CommandInit() API initializes gUnicodeCollation pointer.
But ShellCommandLibConstructor() also initializes the gUnicodeCollation pointer.

Can we just remove CommandInit() from this library? And update all callers?

Thanks/Ray

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Carsey, Jaben
> Sent: Thursday, June 30, 2016 11:13 PM
> To: Laszlo Ersek <ler...@redhat.com>; Qiu, Shumin <shumin@intel.com>
> Cc: Carsey, Jaben <jaben.car...@intel.com>; edk2-devel-01  de...@ml01.01.org>
> Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side
> effects in ASSERT_EFI_ERROR()
> 
> Reviewed-by: Jaben Carsey <jaben.car...@intel.com>
> 
> > -Original Message-
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Thursday, June 30, 2016 4:07 AM
> > To: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin
> > <shumin@intel.com>
> > Cc: edk2-devel-01 <edk2-de...@ml01.01.org>
> > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with
> > side effects in ASSERT_EFI_ERROR()
> > Importance: High
> >
> > Jaben, Shumin,
> >
> > On 06/28/16 15:25, Laszlo Ersek wrote:
> > > When ASSERT_EFI_ERROR() is compiled out, dependent on build flags,
> > > only the status checking should be removed; the function calls should
> stay.
> > >
> > > Cc: Jaben Carsey <jaben.car...@intel.com>
> > > Cc: Shumin Qiu <shumin@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > > ---
> > >
> > > Notes:
> > > build tested
> > >
> > >  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
> > >  ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
> > >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> > Can I please get a maintainer review for this patch?
> >
> > Thanks
> > Laszlo
> >
> > > diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > index 7abfd8944b92..dc96bffde7d3 100644
> > > --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > > @@ -991,8 +991,11 @@ ShellCommandRunElse (
> > >IN EFI_SYSTEM_TABLE  *SystemTable
> > >)
> > >  {
> > > +  EFI_STATUS  Status;
> > >SCRIPT_FILE *CurrentScriptFile;
> > > -  ASSERT_EFI_ERROR(CommandInit());
> > > +
> > > +  Status = CommandInit ();
> > > +  ASSERT_EFI_ERROR (Status);
> > >
> > >if (gEfiShellParametersProtocol->Argc > 1) {
> > >  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> > gShellLevel1HiiHandle, L"if");
> > > @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
> > >IN EFI_SYSTEM_TABLE  *SystemTable
> > >)
> > >  {
> > > +  EFI_STATUS  Status;
> > >SCRIPT_FILE *CurrentScriptFile;
> > > -  ASSERT_EFI_ERROR(CommandInit());
> > > +
> > > +  Status = CommandInit ();
> > > +  ASSERT_EFI_ERROR (Status);
> > >
> > >if (gEfiShellParametersProtocol->Argc > 1) {
> > >  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> > gShellLevel1HiiHandle, L"if");
> > > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > > index cf89a4ac87ed..35a1a7169c8b 100644
> > > --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > > +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > > @@ -373,6 +373,8 @@ EFIAPI
> > >  ShellInitialize (
> > >)
> > >  {
> > > +  EFI_STATUS Status;
> > > +
> > >//
> > >// if auto initialize is not false then skip
> > >//
> > > @@ -383,7 +385,8 @@ ShellInitialize (
> > >//
> > >// deinit the current stuff
> > >//
> > > -  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));
> > > +  Status = ShellLibDestructor (gImageHandle, gST);
> > > + ASSERT_EFI_ERROR (Status);
> > >
> > >//
> > >// init the new stuff
> > >
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-06-30 Thread Carsey, Jaben 
Reviewed-by: Jaben Carsey <jaben.car...@intel.com>

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, June 30, 2016 4:07 AM
> To: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin
> <shumin@intel.com>
> Cc: edk2-devel-01 <edk2-de...@ml01.01.org>
> Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side
> effects in ASSERT_EFI_ERROR()
> Importance: High
> 
> Jaben, Shumin,
> 
> On 06/28/16 15:25, Laszlo Ersek wrote:
> > When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
> > the status checking should be removed; the function calls should stay.
> >
> > Cc: Jaben Carsey <jaben.car...@intel.com>
> > Cc: Shumin Qiu <shumin@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> > ---
> >
> > Notes:
> > build tested
> >
> >  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
> >  ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> Can I please get a maintainer review for this patch?
> 
> Thanks
> Laszlo
> 
> > diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > index 7abfd8944b92..dc96bffde7d3 100644
> > --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> > @@ -991,8 +991,11 @@ ShellCommandRunElse (
> >IN EFI_SYSTEM_TABLE  *SystemTable
> >)
> >  {
> > +  EFI_STATUS  Status;
> >SCRIPT_FILE *CurrentScriptFile;
> > -  ASSERT_EFI_ERROR(CommandInit());
> > +
> > +  Status = CommandInit ();
> > +  ASSERT_EFI_ERROR (Status);
> >
> >if (gEfiShellParametersProtocol->Argc > 1) {
> >  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> gShellLevel1HiiHandle, L"if");
> > @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
> >IN EFI_SYSTEM_TABLE  *SystemTable
> >)
> >  {
> > +  EFI_STATUS  Status;
> >SCRIPT_FILE *CurrentScriptFile;
> > -  ASSERT_EFI_ERROR(CommandInit());
> > +
> > +  Status = CommandInit ();
> > +  ASSERT_EFI_ERROR (Status);
> >
> >if (gEfiShellParametersProtocol->Argc > 1) {
> >  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> gShellLevel1HiiHandle, L"if");
> > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > index cf89a4ac87ed..35a1a7169c8b 100644
> > --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > @@ -373,6 +373,8 @@ EFIAPI
> >  ShellInitialize (
> >)
> >  {
> > +  EFI_STATUS Status;
> > +
> >//
> >// if auto initialize is not false then skip
> >//
> > @@ -383,7 +385,8 @@ ShellInitialize (
> >//
> >// deinit the current stuff
> >//
> > -  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));
> > +  Status = ShellLibDestructor (gImageHandle, gST);
> > +  ASSERT_EFI_ERROR (Status);
> >
> >//
> >// init the new stuff
> >

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-06-30 Thread Laszlo Ersek 
Jaben, Shumin,

On 06/28/16 15:25, Laszlo Ersek wrote:
> When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
> the status checking should be removed; the function calls should stay.
> 
> Cc: Jaben Carsey 
> Cc: Shumin Qiu 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
> 
> Notes:
> build tested
> 
>  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
>  ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
>  2 files changed, 12 insertions(+), 3 deletions(-)

Can I please get a maintainer review for this patch?

Thanks
Laszlo

> diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c 
> b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> index 7abfd8944b92..dc96bffde7d3 100644
> --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> @@ -991,8 +991,11 @@ ShellCommandRunElse (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> +  EFI_STATUS  Status;
>SCRIPT_FILE *CurrentScriptFile;
> -  ASSERT_EFI_ERROR(CommandInit());
> +
> +  Status = CommandInit ();
> +  ASSERT_EFI_ERROR (Status);
>  
>if (gEfiShellParametersProtocol->Argc > 1) {
>  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
> gShellLevel1HiiHandle, L"if");  
> @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> +  EFI_STATUS  Status;
>SCRIPT_FILE *CurrentScriptFile;
> -  ASSERT_EFI_ERROR(CommandInit());
> +
> +  Status = CommandInit ();
> +  ASSERT_EFI_ERROR (Status);
>  
>if (gEfiShellParametersProtocol->Argc > 1) {
>  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
> gShellLevel1HiiHandle, L"if");  
> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
> b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> index cf89a4ac87ed..35a1a7169c8b 100644
> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> @@ -373,6 +373,8 @@ EFIAPI
>  ShellInitialize (
>)
>  {
> +  EFI_STATUS Status;
> +
>//
>// if auto initialize is not false then skip
>//
> @@ -383,7 +385,8 @@ ShellInitialize (
>//
>// deinit the current stuff
>//
> -  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));
> +  Status = ShellLibDestructor (gImageHandle, gST);
> +  ASSERT_EFI_ERROR (Status);
>  
>//
>// init the new stuff
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-06-29 Thread Ni, Ruiyu 
Laszlo,
Thank you for the sharing. Your patches really help to improve the EDKII code 
quality.

Regards,
Ray

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Wednesday, June 29, 2016 7:50 PM
>To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel-01 <edk2-de...@ml01.01.org>
>Cc: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin <shumin....@intel.com>
>Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side 
>effects in ASSERT_EFI_ERROR()
>
>On 06/29/16 08:36, Ni, Ruiyu wrote:
>> Laszlo,
>> Thanks for fixing such a big bug.
>>
>> I am curious how you detect such buggy code? By code review?
>
>Yes.
>
>I described this in the 0/6 message
><http://thread.gmane.org/gmane.comp.bios.edk2.devel/13736>, but I'm
>happy to elaborate. :)
>
>So, Gerd has a Jenkins Continuous Integration environment that regularly
>fetches new edk2 commits and build OvmfPkg and ArmVirtPkg platforms.
>Gerd has recently updated his Jenkins instance to gcc-6, and gcc-6 flags
>some C language constructs -- with new warnings -- that used to "stay
>under the radar" with earlier gcc releases.
>
>So, one of the erroneous constructs that gcc-6 finds is:
>
>  ASSERT_EFI_ERROR (BooleanExpression)
>
>Clearly, in this case the programmer meant
>
>  ASSERT (BooleanExpression)
>
>and ASSERT_EFI_ERROR() is just a typo.
>
>gcc-6 finds this issue because:
>- ASSERT_EFI_ERROR() expands to EFI_ERROR(),
>- EFI_ERROR() expands to RETURN_ERROR(),
>- and RETURN_ERROR() checks if the argument, first converted to
>  RETURN_STATUS (== UINTN), then converted to INTN, is negative,
>- when a boolean expression is converted to UINTN, then INTN, the
>  result is always 0 or 1 -- it can never be negative.
>
>Therefore the incorrect form
>
>  ASSERT_EFI_ERROR (BooleanExpression)
>
>never fires, and gcc-6 finds it with the "-Wbool-compare".
>
>Now, Gerd's build stopped with the first such error, and because I
>hadn't set up gcc-6 yet, I decided to audit all ASSERT_EFI_ERROR()
>applications in edk2. I used the following command as first step:
>
>  git grep -Enw ASSERT_EFI_ERROR \
>  | grep -E -v 'ASSERT_EFI_ERROR( )?\(Status\)'
>
>because I wanted to see all invocations of this macro, *except* the
>following two form:
>
>  ASSERT_EFI_ERROR(Status)
>  ASSERT_EFI_ERROR (Status)
>
>Those two are by far the most frequent ones, and I trusted that a
>variable called "Status" would always have type RETURN_STATUS or EFI_STATUS.
>
>The rest of the macro invocations I audited one by one. Most of the
>errors that I found were of the kind
>
>  ASSERT_EFI_ERROR (BooleanExpression)
>
>which I simply fixed by removing the "_EFI_ERROR" part.
>
>The code fixed by this patch had a different kind of error, but the way
>I found those locations was the same: just looking at ASSERT_EFI_ERROR
>macro invocations.
>
>Thanks!
>Laszlo
>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>>> Laszlo Ersek
>>> Sent: Tuesday, June 28, 2016 9:26 PM
>>> To: edk2-devel-01 <edk2-de...@ml01.01.org>
>>> Cc: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin 
>>> <shumin@intel.com>
>>> Subject: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side 
>>> effects in ASSERT_EFI_ERROR()
>>>
>>> When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
>>> the status checking should be removed; the function calls should stay.
>>>
>>> Cc: Jaben Carsey <jaben.car...@intel.com>
>>> Cc: Shumin Qiu <shumin@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>>> ---
>>>
>>> Notes:
>>>build tested
>>>
>>> ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
>>> ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
>>> 2 files changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c 
>>> b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>>> index 7abfd8944b92..dc96bffde7d3 100644
>>> --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>>> +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>>> @@ -991,8 +991,11 @@ ShellCommandRunElse (
>>>   IN EFI_SYSTEM_TABLE  *SystemTable
>>>   )
>>> {
>>> +  EFI_STATUS  Status;
>>>   SCRIPT_FILE *Curren

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-06-29 Thread Laszlo Ersek 
On 06/29/16 08:36, Ni, Ruiyu wrote:
> Laszlo,
> Thanks for fixing such a big bug.
> 
> I am curious how you detect such buggy code? By code review?

Yes.

I described this in the 0/6 message
<http://thread.gmane.org/gmane.comp.bios.edk2.devel/13736>, but I'm
happy to elaborate. :)

So, Gerd has a Jenkins Continuous Integration environment that regularly
fetches new edk2 commits and build OvmfPkg and ArmVirtPkg platforms.
Gerd has recently updated his Jenkins instance to gcc-6, and gcc-6 flags
some C language constructs -- with new warnings -- that used to "stay
under the radar" with earlier gcc releases.

So, one of the erroneous constructs that gcc-6 finds is:

  ASSERT_EFI_ERROR (BooleanExpression)

Clearly, in this case the programmer meant

  ASSERT (BooleanExpression)

and ASSERT_EFI_ERROR() is just a typo.

gcc-6 finds this issue because:
- ASSERT_EFI_ERROR() expands to EFI_ERROR(),
- EFI_ERROR() expands to RETURN_ERROR(),
- and RETURN_ERROR() checks if the argument, first converted to
  RETURN_STATUS (== UINTN), then converted to INTN, is negative,
- when a boolean expression is converted to UINTN, then INTN, the
  result is always 0 or 1 -- it can never be negative.

Therefore the incorrect form

  ASSERT_EFI_ERROR (BooleanExpression)

never fires, and gcc-6 finds it with the "-Wbool-compare".

Now, Gerd's build stopped with the first such error, and because I
hadn't set up gcc-6 yet, I decided to audit all ASSERT_EFI_ERROR()
applications in edk2. I used the following command as first step:

  git grep -Enw ASSERT_EFI_ERROR \
  | grep -E -v 'ASSERT_EFI_ERROR( )?\(Status\)'

because I wanted to see all invocations of this macro, *except* the
following two form:

  ASSERT_EFI_ERROR(Status)
  ASSERT_EFI_ERROR (Status)

Those two are by far the most frequent ones, and I trusted that a
variable called "Status" would always have type RETURN_STATUS or EFI_STATUS.

The rest of the macro invocations I audited one by one. Most of the
errors that I found were of the kind

  ASSERT_EFI_ERROR (BooleanExpression)

which I simply fixed by removing the "_EFI_ERROR" part.

The code fixed by this patch had a different kind of error, but the way
I found those locations was the same: just looking at ASSERT_EFI_ERROR
macro invocations.

Thanks!
Laszlo

>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
>> Laszlo Ersek
>> Sent: Tuesday, June 28, 2016 9:26 PM
>> To: edk2-devel-01 <edk2-de...@ml01.01.org>
>> Cc: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin 
>> <shumin@intel.com>
>> Subject: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects 
>> in ASSERT_EFI_ERROR()
>>
>> When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
>> the status checking should be removed; the function calls should stay.
>>
>> Cc: Jaben Carsey <jaben.car...@intel.com>
>> Cc: Shumin Qiu <shumin@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> Notes:
>>build tested
>>
>> ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
>> ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
>> 2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c 
>> b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>> index 7abfd8944b92..dc96bffde7d3 100644
>> --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>> +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>> @@ -991,8 +991,11 @@ ShellCommandRunElse (
>>   IN EFI_SYSTEM_TABLE  *SystemTable
>>   )
>> {
>> +  EFI_STATUS  Status;
>>   SCRIPT_FILE *CurrentScriptFile;
>> -  ASSERT_EFI_ERROR(CommandInit());
>> +
>> +  Status = CommandInit ();
>> +  ASSERT_EFI_ERROR (Status);
>>
>>   if (gEfiShellParametersProtocol->Argc > 1) {
>> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
>> gShellLevel1HiiHandle, L"if");
>> @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
>>   IN EFI_SYSTEM_TABLE  *SystemTable
>>   )
>> {
>> +  EFI_STATUS  Status;
>>   SCRIPT_FILE *CurrentScriptFile;
>> -  ASSERT_EFI_ERROR(CommandInit());
>> +
>> +  Status = CommandInit ();
>> +  ASSERT_EFI_ERROR (Status);
>>
>>   if (gEfiShellParametersProtocol->Argc > 1) {
>> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
>> gShellLevel1HiiHandle, L"if");
>> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
>> b/ShellPkg/Library/UefiShellLi

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-06-29 Thread Ni, Ruiyu 
Laszlo,
Thanks for fixing such a big bug.

I am curious how you detect such buggy code? By code review?

Regards,
Ray

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
>Ersek
>Sent: Tuesday, June 28, 2016 9:26 PM
>To: edk2-devel-01 <edk2-de...@ml01.01.org>
>Cc: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin <shumin....@intel.com>
>Subject: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects 
>in ASSERT_EFI_ERROR()
>
>When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
>the status checking should be removed; the function calls should stay.
>
>Cc: Jaben Carsey <jaben.car...@intel.com>
>Cc: Shumin Qiu <shumin@intel.com>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>---
>
>Notes:
>build tested
>
> ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
> ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
>diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c 
>b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>index 7abfd8944b92..dc96bffde7d3 100644
>--- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>+++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
>@@ -991,8 +991,11 @@ ShellCommandRunElse (
>   IN EFI_SYSTEM_TABLE  *SystemTable
>   )
> {
>+  EFI_STATUS  Status;
>   SCRIPT_FILE *CurrentScriptFile;
>-  ASSERT_EFI_ERROR(CommandInit());
>+
>+  Status = CommandInit ();
>+  ASSERT_EFI_ERROR (Status);
>
>   if (gEfiShellParametersProtocol->Argc > 1) {
> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
> gShellLevel1HiiHandle, L"if");
>@@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
>   IN EFI_SYSTEM_TABLE  *SystemTable
>   )
> {
>+  EFI_STATUS  Status;
>   SCRIPT_FILE *CurrentScriptFile;
>-  ASSERT_EFI_ERROR(CommandInit());
>+
>+  Status = CommandInit ();
>+  ASSERT_EFI_ERROR (Status);
>
>   if (gEfiShellParametersProtocol->Argc > 1) {
> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
> gShellLevel1HiiHandle, L"if");
>diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
>b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
>index cf89a4ac87ed..35a1a7169c8b 100644
>--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
>+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
>@@ -373,6 +373,8 @@ EFIAPI
> ShellInitialize (
>   )
> {
>+  EFI_STATUS Status;
>+
>   //
>   // if auto initialize is not false then skip
>   //
>@@ -383,7 +385,8 @@ ShellInitialize (
>   //
>   // deinit the current stuff
>   //
>-  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));
>+  Status = ShellLibDestructor (gImageHandle, gST);
>+  ASSERT_EFI_ERROR (Status);
>
>   //
>   // init the new stuff
>--
>1.8.3.1
>
>
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-06-28 Thread Mudusuru, Giri P 
Reviewed-by: Giri P Mudusuru <giri.p.mudus...@intel.com>


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, June 28, 2016 6:26 AM
> To: edk2-devel-01 <edk2-de...@ml01.01.org>
> Cc: Carsey, Jaben <jaben.car...@intel.com>; Qiu, Shumin
> <shumin....@intel.com>
> Subject: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects 
> in
> ASSERT_EFI_ERROR()
> 
> When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
> the status checking should be removed; the function calls should stay.
> 
> Cc: Jaben Carsey <jaben.car...@intel.com>
> Cc: Shumin Qiu <shumin@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> 
> Notes:
> build tested
> 
>  ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
>  ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> index 7abfd8944b92..dc96bffde7d3 100644
> --- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> +++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
> @@ -991,8 +991,11 @@ ShellCommandRunElse (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> +  EFI_STATUS  Status;
>SCRIPT_FILE *CurrentScriptFile;
> -  ASSERT_EFI_ERROR(CommandInit());
> +
> +  Status = CommandInit ();
> +  ASSERT_EFI_ERROR (Status);
> 
>if (gEfiShellParametersProtocol->Argc > 1) {
>  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> gShellLevel1HiiHandle, L"if");
> @@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> +  EFI_STATUS  Status;
>SCRIPT_FILE *CurrentScriptFile;
> -  ASSERT_EFI_ERROR(CommandInit());
> +
> +  Status = CommandInit ();
> +  ASSERT_EFI_ERROR (Status);
> 
>if (gEfiShellParametersProtocol->Argc > 1) {
>  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY),
> gShellLevel1HiiHandle, L"if");
> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> index cf89a4ac87ed..35a1a7169c8b 100644
> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> @@ -373,6 +373,8 @@ EFIAPI
>  ShellInitialize (
>)
>  {
> +  EFI_STATUS Status;
> +
>//
>// if auto initialize is not false then skip
>//
> @@ -383,7 +385,8 @@ ShellInitialize (
>//
>// deinit the current stuff
>//
> -  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));
> +  Status = ShellLibDestructor (gImageHandle, gST);
> +  ASSERT_EFI_ERROR (Status);
> 
>//
>// init the new stuff
> --
> 1.8.3.1
> 
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()

2016-06-28 Thread Laszlo Ersek 
When ASSERT_EFI_ERROR() is compiled out, dependent on build flags, only
the status checking should be removed; the function calls should stay.

Cc: Jaben Carsey 
Cc: Shumin Qiu 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek 
---

Notes:
build tested

 ShellPkg/Library/UefiShellLevel1CommandsLib/If.c | 10 --
 ShellPkg/Library/UefiShellLib/UefiShellLib.c |  5 -
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c 
b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
index 7abfd8944b92..dc96bffde7d3 100644
--- a/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
+++ b/ShellPkg/Library/UefiShellLevel1CommandsLib/If.c
@@ -991,8 +991,11 @@ ShellCommandRunElse (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  EFI_STATUS  Status;
   SCRIPT_FILE *CurrentScriptFile;
-  ASSERT_EFI_ERROR(CommandInit());
+
+  Status = CommandInit ();
+  ASSERT_EFI_ERROR (Status);
 
   if (gEfiShellParametersProtocol->Argc > 1) {
 ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
gShellLevel1HiiHandle, L"if");  
@@ -1066,8 +1069,11 @@ ShellCommandRunEndIf (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
+  EFI_STATUS  Status;
   SCRIPT_FILE *CurrentScriptFile;
-  ASSERT_EFI_ERROR(CommandInit());
+
+  Status = CommandInit ();
+  ASSERT_EFI_ERROR (Status);
 
   if (gEfiShellParametersProtocol->Argc > 1) {
 ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_TOO_MANY), 
gShellLevel1HiiHandle, L"if");  
diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index cf89a4ac87ed..35a1a7169c8b 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -373,6 +373,8 @@ EFIAPI
 ShellInitialize (
   )
 {
+  EFI_STATUS Status;
+
   //
   // if auto initialize is not false then skip
   //
@@ -383,7 +385,8 @@ ShellInitialize (
   //
   // deinit the current stuff
   //
-  ASSERT_EFI_ERROR(ShellLibDestructor(gImageHandle, gST));
+  Status = ShellLibDestructor (gImageHandle, gST);
+  ASSERT_EFI_ERROR (Status);
 
   //
   // init the new stuff
-- 
1.8.3.1


___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel