[edk2-devel] [PATCH 30/35] ShellPkg: stop taking EFI_HANDLE in place of SHELL_FILE_HANDLE

[Gao, Zhichao]

OK. I didn't view the whole calling stack. Thanks for your clear explain.
Then why we need two exact same handle type? May be we should keep only one of them. Same with the DEBUG_XXX and EFI_D_XXX.

Back to the patch, it is OK to me now. Reviewed-by: Zhichao Gao <zhichao.gao at intel.com>

> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, September 26, 2019 8:09 PM
> To: devel at edk2.groups.io; Gao, Zhichao <zhichao.gao at intel.com>
> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Ni, Ray <ray.ni at intel.com>
> Subject: Re: [edk2-devel] [PATCH 30/35] ShellPkg: stop taking EFI_HANDLE in
> place of SHELL_FILE_HANDLE
> 
> On 09/26/19 04:53, Gao, Zhichao wrote:
> >
> >
> >> -----Original Message-----
> >> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> >> Laszlo Ersek
> >> Sent: Wednesday, September 18, 2019 3:50 AM
> >> To: edk2-devel-groups-io <devel at edk2.groups.io>
> >> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Ni, Ray
> >> <ray.ni at intel.com>; Gao, Zhichao <zhichao.gao at intel.com>
> >> Subject: [edk2-devel] [PATCH 30/35] ShellPkg: stop taking EFI_HANDLE
> >> in place of SHELL_FILE_HANDLE
> >>
> >> The TouchFileByHandle() and IsDirectoryEmpty() functions are passed
> >> SHELL_FILE_HANDLE parameters, and they use those parameters
> correctly.
> >> However, their parameter lists say EFI_HANDLE.
> >>
> >> Spell out the right type in the parameter lists.
> >>
> >> In practice, this change is a no-op (because, quite regrettably, both
> >> EFI_HANDLE and SHELL_FILE_HANDLE are specified to be typedefs of
> >> (VOID*)).
> >>
> >> Cc: Jaben Carsey <jaben.carsey at intel.com>
> >> Cc: Ray Ni <ray.ni at intel.com>
> >> Cc: Zhichao Gao <zhichao.gao at intel.com>
> >> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> >> ---
> >>
> >> Notes:
> >>     tested: rm, touch
> >>
> >>  ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c    | 2 +-
> >>  ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c
> >> b/ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c
> >> index 3a1196f1529e..59f7eec376f2 100644
> >> --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c
> >> +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Rm.c
> >> @@ -24,7 +24,7 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] =
> {  **/
> >> BOOLEAN  IsDirectoryEmpty (
> >> -  IN EFI_HANDLE   FileHandle
> >> +  IN SHELL_FILE_HANDLE   FileHandle
> >
> > Here may be EFI_FILE_HANDLE.
> 
> Yes, it *may*, but doesn't *have* to.
> 
> We have the following call tree:
> 
>   CascadeDelete()
>     IsDirectoryEmpty()
>       FileHandleFindFirstFile()
>       FileHandleFindNextFile()
> 
> With this patch, we have:
> 
>   "Node->Handle"
>     |
>     [SHELL_FILE_HANDLE]
>     |
>     v
>   CascadeDelete()
>     |
>     [SHELL_FILE_HANDLE]
>     |
>     v
>     IsDirectoryEmpty()
>       |
>       [EFI_FILE_HANDLE]
>       |
>       v
>       FileHandleFindFirstFile()
>       FileHandleFindNextFile()
> 
> with your proposal, we would have:
> 
>   "Node->Handle"
>     |
>     [SHELL_FILE_HANDLE]
>     |
>     v
>   CascadeDelete()
>     |
>     [EFI_FILE_HANDLE]
>     |
>     v
>     IsDirectoryEmpty()
>       |
>       [EFI_FILE_HANDLE]
>       |
>       v
>       FileHandleFindFirstFile()
>       FileHandleFindNextFile()
> 
> In both cases, we depend on SHELL_FILE_HANDLE being equivalent to
> EFI_FILE_HANDLE. In the end, both types are:
> 
>   (struct _EFI_FILE_PROTOCOL *)
> 
> In both cases, we go from SHELL_FILE_HANDLE to EFI_FILE_HANDLE, and
> exploit that they are identical; the difference is only *where* we exploit that.
> 
> - In this patch, we exploit the identity in IsDirectoryEmpty(): we take a
> SHELL_FILE_HANDLE, and give it to functions that take EFI_FILE_HANDLE.
> 
> - In your proposal, we would exploit the exact same identity, just in
> CascadeDelete(): "Node->Handle" is a SHELL_FILE_HANDLE (see
> EFI_SHELL_FILE_INFO.Handle), and we'd pass it to a function (namely
> IsDirectoryEmpty()) taking an EFI_FILE_HANDLE.
> 
> Given that your proposal wouldn't change our dependence on the
> SHELL_FILE_HANDLE===EFI_FILE_HANDLE identity, I prefer to stick with the
> current patch.
> 
> Thanks
> Laszlo
> 
> >
> >>    )
> >>  {
> >>    EFI_STATUS      Status;
> >> diff --git a/ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c
> >> b/ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c
> >> index 0f00344c815e..a215f5774c69 100644
> >> --- a/ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c
> >> +++ b/ShellPkg/Library/UefiShellLevel3CommandsLib/Touch.c
> >> @@ -21,7 +21,7 @@
> >>  **/
> >>  EFI_STATUS
> >>  TouchFileByHandle (
> >> -  IN EFI_HANDLE Handle
> >> +  IN SHELL_FILE_HANDLE Handle
> >
> > Here is OK.
> >
> > Thanks,
> > Zhichao
> >
> >>    )
> >>  {
> >>    EFI_STATUS    Status;
> >> --
> >> 2.19.1.3.g30247aa5d201
> >>
> >>
> >>
> >> -=-=-=-=-=-=
> >> Groups.io Links: You receive all messages sent to this group.
> >>
> >> View/Reply Online (#47417):
> >> https://edk2.groups.io/g/devel/message/47417
> >> Mute This Topic: https://groups.io/mt/34180233/1768756
> >> Group Owner: devel+owner at edk2.groups.io
> >> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> >> [zhichao.gao at intel.com]
> >> -=-=-=-=-=-=
> >
> >
> >
> >
> 
> 
> 


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

View/Reply Online (#48117): https://edk2.groups.io/g/devel/message/48117
Mute This Topic: https://groups.io/mt/34180233/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-