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

2019-09-26 Thread 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 


> -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 
> Cc: Carsey, Jaben ; Ni, Ray 
> 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 
> >> Cc: Carsey, Jaben ; Ni, Ray
> >> ; Gao, Zhichao 
> >> 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 
> >> Cc: Ray Ni 
> >> Cc: Zhichao Gao 
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>
> >> 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/UefiShellLevel3CommandsLi

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

2019-09-26 Thread Laszlo Ersek 
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 
>> Cc: Carsey, Jaben ; Ni, Ray ;
>> Gao, Zhichao 
>> 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 
>> Cc: Ray Ni 
>> Cc: Zhichao Gao 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> 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_STATUSStatus;
>> --
>> 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 (#48092): https://edk2.groups.io/g/devel/message/48092
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2019-09-23 Thread Philippe Mathieu-Daudé 
On 9/17/19 9:49 PM, Laszlo Ersek wrote:
> 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 
> Cc: Ray Ni 
> Cc: Zhichao Gao 
> Signed-off-by: Laszlo Ersek 
> ---
> 
> 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
>)
>  {
>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
>)
>  {
>EFI_STATUSStatus;
> 

Reviewed-by: Philippe Mathieu-Daude 

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

View/Reply Online (#47846): https://edk2.groups.io/g/devel/message/47846
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

2019-09-17 Thread Laszlo Ersek 
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 
Cc: Ray Ni 
Cc: Zhichao Gao 
Signed-off-by: Laszlo Ersek 
---

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
   )
 {
   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
   )
 {
   EFI_STATUSStatus;
-- 
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-