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 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]
>> -=-=-=-=-=-=
> 
> 
> 
>