Re: [edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug

2019-09-26 Thread Carsey, Jaben 
Reviewed-by: Jaben Carsey 

Given the time gap between now and when this library was originally written, I 
think that we should revisit maintaining support for EFI Shell. Do we still 
need to have UEFI Applications that can go between EFI and UEFI versions of the 
shell? Are there really many EFI Shell instances that currently developed 
applications need to maintain support for?

Thanks
-Jaben

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, September 26, 2019 5:47 AM
> To: Carsey, Jaben ; Ni, Ray ;
> Gao, Zhichao 
> Cc: edk2-devel-groups-io 
> Subject: Re: [edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify
> workaround for unfixable EdkShell bug
> 
> Jaben, Ray, Zhichao,
> 
> can one of you guys please review this patch?
> 
> Thanks
> Laszlo
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
> > The EDK 1 Shell (available at <https://github.com/tianocore/edk-Shell>)
> > has a bug in its EFI_SHELL_ENVIRONMENT2.Execute() implementation that
> > edk2's UefiShellLib has no choice but to work around.
> >
> > Improve the explanation in the code. Also, document the implicit
> > EFI_HANDLE -> (EFI_HANDLE*) conversion, which happens implicitly after
> > dereferencing ParentHandle, with an explicit cast.
> >
> > In practice, this patch is a no-op.
> >
> > Cc: Jaben Carsey 
> > Cc: Ray Ni 
> > Cc: Zhichao Gao 
> > Signed-off-by: Laszlo Ersek 
> > ---
> >
> > Notes:
> > build-tested only
> >
> >  ShellPkg/Library/UefiShellLib/UefiShellLib.c | 22 ++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > index 835d0f88ca74..9f07a58eb23d 100644
> > --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > @@ -1291,9 +1291,27 @@ ShellExecute (
> >if (mEfiShellEnvironment2 != NULL) {
> >  //
> >  // Call EFI Shell version.
> > -// Due to oddity in the EFI shell we want to dereference the
> ParentHandle here
> >  //
> > -CmdStatus = (mEfiShellEnvironment2->Execute(*ParentHandle,
> > +// Due to an unfixable bug in the EdkShell implementation, we must
> > +// dereference "ParentHandle" here:
> > +//
> > +// 1. The EFI shell installs the EFI_SHELL_ENVIRONMENT2 protocol,
> > +//identified by gEfiShellEnvironment2Guid.
> > +// 2. The Execute() member function takes "ParentImageHandle" as
> first
> > +//parameter, with type (EFI_HANDLE*).
> > +// 3. In the EdkShell implementation, SEnvExecute() implements the
> > +//Execute() member function. It passes "ParentImageHandle"
> correctly to
> > +//SEnvDoExecute().
> > +// 4. SEnvDoExecute() takes the (EFI_HANDLE*), and passes it directly 
> > --
> > +//without de-referencing -- to the HandleProtocol() boot service.
> > +// 5. But HandleProtocol() takes an EFI_HANDLE.
> > +//
> > +// Therefore we must
> > +// - de-reference "ParentHandle" here, to mask the bug in
> > +//   SEnvDoExecute(), and
> > +// - pass the resultant EFI_HANDLE as an (EFI_HANDLE*).
> > +//
> > +CmdStatus = (mEfiShellEnvironment2->Execute((EFI_HANDLE
> *)*ParentHandle,
> >CommandLine,
> >Output));
> >  //
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48120): https://edk2.groups.io/g/devel/message/48120
Mute This Topic: https://groups.io/mt/34180236/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug

2019-09-26 Thread Gao, Zhichao 
Sorry for miss this.
The comment is nice to help understand the type conversion.

Reviewed-by: Zhichao Gao 

> -Original Message-
> From: Laszlo Ersek 
> Sent: Thursday, September 26, 2019 8:47 PM
> To: Carsey, Jaben ; Ni, Ray ;
> Gao, Zhichao 
> Cc: edk2-devel-groups-io 
> Subject: Re: [edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify
> workaround for unfixable EdkShell bug
> 
> Jaben, Ray, Zhichao,
> 
> can one of you guys please review this patch?
> 
> Thanks
> Laszlo
> 
> On 09/17/19 21:49, Laszlo Ersek wrote:
> > The EDK 1 Shell (available at
> > <https://github.com/tianocore/edk-Shell>)
> > has a bug in its EFI_SHELL_ENVIRONMENT2.Execute() implementation that
> > edk2's UefiShellLib has no choice but to work around.
> >
> > Improve the explanation in the code. Also, document the implicit
> > EFI_HANDLE -> (EFI_HANDLE*) conversion, which happens implicitly after
> > dereferencing ParentHandle, with an explicit cast.
> >
> > In practice, this patch is a no-op.
> >
> > Cc: Jaben Carsey 
> > Cc: Ray Ni 
> > Cc: Zhichao Gao 
> > Signed-off-by: Laszlo Ersek 
> > ---
> >
> > Notes:
> > build-tested only
> >
> >  ShellPkg/Library/UefiShellLib/UefiShellLib.c | 22
> > ++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > index 835d0f88ca74..9f07a58eb23d 100644
> > --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> > @@ -1291,9 +1291,27 @@ ShellExecute (
> >if (mEfiShellEnvironment2 != NULL) {
> >  //
> >  // Call EFI Shell version.
> > -// Due to oddity in the EFI shell we want to dereference the
> ParentHandle here
> >  //
> > -CmdStatus = (mEfiShellEnvironment2->Execute(*ParentHandle,
> > +// Due to an unfixable bug in the EdkShell implementation, we must
> > +// dereference "ParentHandle" here:
> > +//
> > +// 1. The EFI shell installs the EFI_SHELL_ENVIRONMENT2 protocol,
> > +//identified by gEfiShellEnvironment2Guid.
> > +// 2. The Execute() member function takes "ParentImageHandle" as
> first
> > +//parameter, with type (EFI_HANDLE*).
> > +// 3. In the EdkShell implementation, SEnvExecute() implements the
> > +//Execute() member function. It passes "ParentImageHandle"
> correctly to
> > +//SEnvDoExecute().
> > +// 4. SEnvDoExecute() takes the (EFI_HANDLE*), and passes it directly 
> > --
> > +//without de-referencing -- to the HandleProtocol() boot service.
> > +// 5. But HandleProtocol() takes an EFI_HANDLE.
> > +//
> > +// Therefore we must
> > +// - de-reference "ParentHandle" here, to mask the bug in
> > +//   SEnvDoExecute(), and
> > +// - pass the resultant EFI_HANDLE as an (EFI_HANDLE*).
> > +//
> > +CmdStatus = (mEfiShellEnvironment2->Execute((EFI_HANDLE
> > + *)*ParentHandle,
> >CommandLine,
> >Output));
> >  //
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48112): https://edk2.groups.io/g/devel/message/48112
Mute This Topic: https://groups.io/mt/34180236/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug

2019-09-26 Thread Laszlo Ersek 
Jaben, Ray, Zhichao,

can one of you guys please review this patch?

Thanks
Laszlo

On 09/17/19 21:49, Laszlo Ersek wrote:
> The EDK 1 Shell (available at )
> has a bug in its EFI_SHELL_ENVIRONMENT2.Execute() implementation that
> edk2's UefiShellLib has no choice but to work around.
> 
> Improve the explanation in the code. Also, document the implicit
> EFI_HANDLE -> (EFI_HANDLE*) conversion, which happens implicitly after
> dereferencing ParentHandle, with an explicit cast.
> 
> In practice, this patch is a no-op.
> 
> Cc: Jaben Carsey 
> Cc: Ray Ni 
> Cc: Zhichao Gao 
> Signed-off-by: Laszlo Ersek 
> ---
> 
> Notes:
> build-tested only
> 
>  ShellPkg/Library/UefiShellLib/UefiShellLib.c | 22 ++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
> b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> index 835d0f88ca74..9f07a58eb23d 100644
> --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
> @@ -1291,9 +1291,27 @@ ShellExecute (
>if (mEfiShellEnvironment2 != NULL) {
>  //
>  // Call EFI Shell version.
> -// Due to oddity in the EFI shell we want to dereference the 
> ParentHandle here
>  //
> -CmdStatus = (mEfiShellEnvironment2->Execute(*ParentHandle,
> +// Due to an unfixable bug in the EdkShell implementation, we must
> +// dereference "ParentHandle" here:
> +//
> +// 1. The EFI shell installs the EFI_SHELL_ENVIRONMENT2 protocol,
> +//identified by gEfiShellEnvironment2Guid.
> +// 2. The Execute() member function takes "ParentImageHandle" as first
> +//parameter, with type (EFI_HANDLE*).
> +// 3. In the EdkShell implementation, SEnvExecute() implements the
> +//Execute() member function. It passes "ParentImageHandle" correctly 
> to
> +//SEnvDoExecute().
> +// 4. SEnvDoExecute() takes the (EFI_HANDLE*), and passes it directly --
> +//without de-referencing -- to the HandleProtocol() boot service.
> +// 5. But HandleProtocol() takes an EFI_HANDLE.
> +//
> +// Therefore we must
> +// - de-reference "ParentHandle" here, to mask the bug in
> +//   SEnvDoExecute(), and
> +// - pass the resultant EFI_HANDLE as an (EFI_HANDLE*).
> +//
> +CmdStatus = (mEfiShellEnvironment2->Execute((EFI_HANDLE *)*ParentHandle,
>CommandLine,
>Output));
>  //
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48106): https://edk2.groups.io/g/devel/message/48106
Mute This Topic: https://groups.io/mt/34180236/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 32/35] ShellPkg/UefiShellLib: clarify workaround for unfixable EdkShell bug

2019-09-17 Thread Laszlo Ersek 
The EDK 1 Shell (available at )
has a bug in its EFI_SHELL_ENVIRONMENT2.Execute() implementation that
edk2's UefiShellLib has no choice but to work around.

Improve the explanation in the code. Also, document the implicit
EFI_HANDLE -> (EFI_HANDLE*) conversion, which happens implicitly after
dereferencing ParentHandle, with an explicit cast.

In practice, this patch is a no-op.

Cc: Jaben Carsey 
Cc: Ray Ni 
Cc: Zhichao Gao 
Signed-off-by: Laszlo Ersek 
---

Notes:
build-tested only

 ShellPkg/Library/UefiShellLib/UefiShellLib.c | 22 ++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c 
b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
index 835d0f88ca74..9f07a58eb23d 100644
--- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c
+++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c
@@ -1291,9 +1291,27 @@ ShellExecute (
   if (mEfiShellEnvironment2 != NULL) {
 //
 // Call EFI Shell version.
-// Due to oddity in the EFI shell we want to dereference the ParentHandle 
here
 //
-CmdStatus = (mEfiShellEnvironment2->Execute(*ParentHandle,
+// Due to an unfixable bug in the EdkShell implementation, we must
+// dereference "ParentHandle" here:
+//
+// 1. The EFI shell installs the EFI_SHELL_ENVIRONMENT2 protocol,
+//identified by gEfiShellEnvironment2Guid.
+// 2. The Execute() member function takes "ParentImageHandle" as first
+//parameter, with type (EFI_HANDLE*).
+// 3. In the EdkShell implementation, SEnvExecute() implements the
+//Execute() member function. It passes "ParentImageHandle" correctly to
+//SEnvDoExecute().
+// 4. SEnvDoExecute() takes the (EFI_HANDLE*), and passes it directly --
+//without de-referencing -- to the HandleProtocol() boot service.
+// 5. But HandleProtocol() takes an EFI_HANDLE.
+//
+// Therefore we must
+// - de-reference "ParentHandle" here, to mask the bug in
+//   SEnvDoExecute(), and
+// - pass the resultant EFI_HANDLE as an (EFI_HANDLE*).
+//
+CmdStatus = (mEfiShellEnvironment2->Execute((EFI_HANDLE *)*ParentHandle,
   CommandLine,
   Output));
 //
-- 
2.19.1.3.g30247aa5d201



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47419): https://edk2.groups.io/g/devel/message/47419
Mute This Topic: https://groups.io/mt/34180236/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-