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; Thu, 26 Sep 2019 05:08:54 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 24C6B30ADBA6; Thu, 26 Sep 2019 12:08:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id DAE655D6B0; Thu, 26 Sep 2019 12:08:52 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 30/35] ShellPkg: stop taking EFI_HANDLE in place of SHELL_FILE_HANDLE To: devel@edk2.groups.io, zhichao.gao@intel.com Cc: "Carsey, Jaben" , "Ni, Ray" References: <20190917194935.24322-1-lersek@redhat.com> <20190917194935.24322-31-lersek@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B83F0FF@SHSMSX101.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: <8a6e416e-30d3-13d8-f089-f43740fbe5ac@redhat.com> Date: Thu, 26 Sep 2019 14:08:51 +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: <3CE959C139B4C44DBEA1810E3AA6F9000B83F0FF@SHSMSX101.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Thu, 26 Sep 2019 12:08:54 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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] >> -=-=-=-=-=-= > > > >