Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()
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 > Cc: edk2-devel-01 ; Laszlo Ersek > ; Qiu, Shumin > 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 > > Cc: edk2-devel-01 ; Laszlo Ersek > > ; Qiu, Shumin ; Carsey, > Jaben > > > > 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 > > > Cc: Carsey, Jaben ; edk2-devel-01 > > de...@ml01.01.org>; Laszlo Ersek ; Qiu, Shumin > > > > > > 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 ; Qiu, Shumin > > > > > > > Cc: Carsey, Jaben ; 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 > > > > > > > > > -Original Message- > > > > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > > > > Sent: Thursday, June 30, 2016 4:07 AM > > > > > To: Carsey, Jaben ; Qiu, Shumin > > > > > > > > > > Cc: edk2-devel-01 > > > > > 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 > > > > > > 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
Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()
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 > Cc: edk2-devel-01 ; Laszlo Ersek > ; Qiu, Shumin ; Carsey, Jaben > > 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 > > Cc: Carsey, Jaben ; edk2-devel-01 > de...@ml01.01.org>; Laszlo Ersek ; Qiu, Shumin > > > > 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 ; Qiu, Shumin > > > > > Cc: Carsey, Jaben ; 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 > > > > > > > -----Original Message- > > > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > > > Sent: Thursday, June 30, 2016 4:07 AM > > > > To: Carsey, Jaben ; Qiu, Shumin > > > > > > > > Cc: edk2-devel-01 > > > > 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 > > > > > 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 (
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 > Cc: Carsey, Jaben ; edk2-devel-01 de...@ml01.01.org>; Laszlo Ersek ; Qiu, Shumin > > 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 ; Qiu, Shumin > > > Cc: Carsey, Jaben ; 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 > > > > > -Original Message- > > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > > Sent: Thursday, June 30, 2016 4:07 AM > > > To: Carsey, Jaben ; Qiu, Shumin > > > > > > Cc: edk2-devel-01 > > > 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 > > > > 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 ___ 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()
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 ; Qiu, Shumin > Cc: Carsey, Jaben ; 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 > > > -Original Message- > > From: Laszlo Ersek [mailto:ler...@redhat.com] > > Sent: Thursday, June 30, 2016 4:07 AM > > To: Carsey, Jaben ; Qiu, Shumin > > > > Cc: edk2-devel-01 > > 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 > > > 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 ___ 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()
Reviewed-by: Jaben Carsey > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, June 30, 2016 4:07 AM > To: Carsey, Jaben ; Qiu, Shumin > > Cc: edk2-devel-01 > 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 > > 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()
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()
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 ; edk2-devel-01 >Cc: Carsey, Jaben ; Qiu, Shumin >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 >>> Cc: Carsey, Jaben ; Qiu, Shumin >>> >>> 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 >>> 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) { >>> Shel
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 >> Cc: Carsey, Jaben ; Qiu, Shumin >> >> 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 >> 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 >> +
Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with side effects in ASSERT_EFI_ERROR()
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 >Cc: Carsey, Jaben ; Qiu, Shumin >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 >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 ___ 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()
Reviewed-by: Giri P Mudusuru > -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 > Cc: Carsey, Jaben ; Qiu, Shumin > > 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 > 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 ___ 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()
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