Sorry for miss this. The comment is nice to help understand the type conversion.
Reviewed-by: Zhichao Gao <zhichao....@intel.com> > -----Original Message----- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Thursday, September 26, 2019 8:47 PM > To: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@intel.com>; > Gao, Zhichao <zhichao....@intel.com> > Cc: edk2-devel-groups-io <devel@edk2.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 <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 (#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] -=-=-=-=-=-=-=-=-=-=-=-
