On 09/26/19 16:43, Gao, Zhichao wrote: > OK. I didn't view the whole calling stack. Thanks for your clear explain. > Then why we need two exact same handle type?
Unfortunately, I have no clue. > May be we should keep only one of them. Same with the DEBUG_XXX and EFI_D_XXX. In the long term, EFI_FILE_HANDLE should likely be eliminated completely. It starts with the EFI_ prefix, suggesting it's a standard type. But none of the UEFI, PI, and Shell specs define EFI_FILE_HANDLE. The Shell spec does define SHELL_FILE_HANDLE, so that should be preserved in the long term. Luckily, that's what the patch uses anyway. > > Back to the patch, it is OK to me now. Reviewed-by: Zhichao Gao > <zhichao....@intel.com> Thank you! Laszlo > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Laszlo Ersek >> Sent: Thursday, September 26, 2019 8:09 PM >> To: devel@edk2.groups.io; Gao, Zhichao <zhichao....@intel.com> >> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray <ray...@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@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >>>> Laszlo Ersek >>>> Sent: Wednesday, September 18, 2019 3:50 AM >>>> To: edk2-devel-groups-io <devel@edk2.groups.io> >>>> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray >>>> <ray...@intel.com>; Gao, Zhichao <zhichao....@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.car...@intel.com> >>>> Cc: Ray Ni <ray...@intel.com> >>>> Cc: Zhichao Gao <zhichao....@intel.com> >>>> Signed-off-by: Laszlo Ersek <ler...@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+ow...@edk2.groups.io >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>>> [zhichao....@intel.com] >>>> -=-=-=-=-=-= >>> >>> >>> >>> >> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48296): https://edk2.groups.io/g/devel/message/48296 Mute This Topic: https://groups.io/mt/34180233/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
