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 <jaben.car...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Zhichao Gao <zhichao....@intel.com> > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > > 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] -=-=-=-=-=-=-=-=-=-=-=-
