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

["Laszlo Ersek" <lersek@redhat.com>]

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.gao@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.gao@intel.com>
>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@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.carsey@intel.com>; Ni, Ray
>>>> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@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@intel.com>
>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>> Cc: Zhichao Gao <zhichao.gao@intel.com>
>>>> Signed-off-by: Laszlo Ersek <lersek@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@edk2.groups.io
>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>>>> [zhichao.gao@intel.com]
>>>> -=-=-=-=-=-=
>>>
>>>
>>>
>>>
>>
>>
>> 
>