From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 30 Sep 2019 12:52:07 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 090D910CC1FB; Mon, 30 Sep 2019 19:52:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-111.rdu2.redhat.com [10.10.121.111]) by smtp.corp.redhat.com (Postfix) with ESMTP id BAFC760BF1; Mon, 30 Sep 2019 19:52:05 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 30/35] ShellPkg: stop taking EFI_HANDLE in place of SHELL_FILE_HANDLE To: "Gao, Zhichao" , "devel@edk2.groups.io" Cc: "Carsey, Jaben" , "Ni, Ray" References: <20190917194935.24322-1-lersek@redhat.com> <20190917194935.24322-31-lersek@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B83F0FF@SHSMSX101.ccr.corp.intel.com> <8a6e416e-30d3-13d8-f089-f43740fbe5ac@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B83F4CC@SHSMSX101.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <0a2e62d1-ab60-ef1f-512c-27df61b4c592@redhat.com> Date: Mon, 30 Sep 2019 21:52:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B83F4CC@SHSMSX101.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.65]); Mon, 30 Sep 2019 19:52:07 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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 >> 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/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] >>>> -=-=-=-=-=-= >>> >>> >>> >>> >> >> >> >