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 <[email protected]> > Cc: edk2-devel-01 <[email protected]>; Laszlo Ersek > <[email protected]>; Qiu, Shumin <[email protected]> > 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 <[email protected]> > > Cc: edk2-devel-01 <[email protected]>; Laszlo Ersek > > <[email protected]>; Qiu, Shumin <[email protected]>; Carsey, > Jaben > > <[email protected]> > > 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 <[email protected]> > > > Cc: Carsey, Jaben <[email protected]>; edk2-devel-01 <edk2- > > > [email protected]>; Laszlo Ersek <[email protected]>; Qiu, Shumin > > > <[email protected]> > > > 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:[email protected]] On Behalf > > > > Of Carsey, Jaben > > > > Sent: Thursday, June 30, 2016 11:13 PM > > > > To: Laszlo Ersek <[email protected]>; Qiu, Shumin > > > <[email protected]> > > > > Cc: Carsey, Jaben <[email protected]>; edk2-devel-01 <edk2- > > > > [email protected]> > > > > Subject: Re: [edk2] [PATCH 4/6] ShellPkg: don't call functions with > > > > side effects in ASSERT_EFI_ERROR() > > > > > > > > Reviewed-by: Jaben Carsey <[email protected]> > > > > > > > > > -----Original Message----- > > > > > From: Laszlo Ersek [mailto:[email protected]] > > > > > Sent: Thursday, June 30, 2016 4:07 AM > > > > > To: Carsey, Jaben <[email protected]>; Qiu, Shumin > > > > > <[email protected]> > > > > > Cc: edk2-devel-01 <[email protected]> > > > > > 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 <[email protected]> > > > > > > Cc: Shumin Qiu <[email protected]> > > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > > > > Signed-off-by: Laszlo Ersek <[email protected]> > > > > > > --- > > > > > > > > > > > > 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 > > > > [email protected] > > > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
