From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.93; helo=mga11.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E0411210DA163 for ; Sun, 5 Aug 2018 19:03:39 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Aug 2018 19:03:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,450,1526367600"; d="scan'208";a="72959822" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.4]) ([10.239.9.4]) by orsmga003.jf.intel.com with ESMTP; 05 Aug 2018 19:03:38 -0700 To: Laszlo Ersek , edk2-devel-01 Cc: Jaben Carsey References: <20180803121537.32123-1-lersek@redhat.com> <20180803121537.32123-8-lersek@redhat.com> From: "Ni, Ruiyu" Message-ID: <0b5ca4e2-f532-c1c9-9709-a013a6df4e4a@Intel.com> Date: Mon, 6 Aug 2018 10:04:16 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180803121537.32123-8-lersek@redhat.com> Subject: Re: [PATCH v2 7/7] ShellPkg/UefiShellLib: rebase ShellOpenFileByDevicePath() to UefiLib API X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Aug 2018 02:03:40 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 8/3/2018 8:15 PM, Laszlo Ersek wrote: > Replace the "old shell method" implementation in > ShellOpenFileByDevicePath() with EfiOpenFileByDevicePath() from UefiLib, > correcting the following issues: > > - code duplication between this module and other modules, > - local variable name "EfiSimpleFileSystemProtocol" starting with "Efi" > prefix, > - bogus "FileHandle = NULL" assignments, > - leaking "Handle1" when the device path type/subtype check or the > realignment-motivated AllocateCopyPool() fails in the loop. > > Cc: Jaben Carsey > Cc: Ruiyu Ni > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1008 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > Reviewed-by: Jaben Carsey > --- > > Notes: > v2: > - pick up Jaben's R-b > > ShellPkg/Library/UefiShellLib/UefiShellLib.inf | 1 - > ShellPkg/Library/UefiShellLib/UefiShellLib.c | 113 +------------------- > 2 files changed, 3 insertions(+), 111 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.inf b/ShellPkg/Library/UefiShellLib/UefiShellLib.inf > index 38d9a4b81f5f..aacddbbf9765 100644 > --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.inf > +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.inf > @@ -51,7 +51,6 @@ [LibraryClasses] > SortLib > > [Protocols] > - gEfiSimpleFileSystemProtocolGuid ## SOMETIMES_CONSUMES > gEfiUnicodeCollation2ProtocolGuid ## CONSUMES > > # shell 2.0 > diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c > index 18c3be4a8bc7..f04adbb63ffe 100644 > --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c > +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c > @@ -504,12 +504,7 @@ ShellOpenFileByDevicePath( > { > CHAR16 *FileName; > EFI_STATUS Status; > - EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *EfiSimpleFileSystemProtocol; > - EFI_FILE_PROTOCOL *Handle1; > - EFI_FILE_PROTOCOL *Handle2; > - CHAR16 *FnafPathName; > - UINTN PathLen; > - EFI_HANDLE DeviceHandle; > + EFI_FILE_PROTOCOL *File; > > if (FilePath == NULL || FileHandle == NULL) { > return (EFI_INVALID_PARAMETER); > @@ -535,117 +530,15 @@ ShellOpenFileByDevicePath( > // > // use old shell method. > // > - Status = gBS->LocateDevicePath (&gEfiSimpleFileSystemProtocolGuid, > - FilePath, > - &DeviceHandle); > + Status = EfiOpenFileByDevicePath (FilePath, &File, OpenMode, Attributes); > if (EFI_ERROR (Status)) { > return Status; > } > - Status = gBS->OpenProtocol(DeviceHandle, > - &gEfiSimpleFileSystemProtocolGuid, > - (VOID**)&EfiSimpleFileSystemProtocol, > - gImageHandle, > - NULL, > - EFI_OPEN_PROTOCOL_GET_PROTOCOL); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - Status = EfiSimpleFileSystemProtocol->OpenVolume(EfiSimpleFileSystemProtocol, &Handle1); > - if (EFI_ERROR (Status)) { > - FileHandle = NULL; > - return Status; > - } > - > - // > - // go down directories one node at a time. > - // > - while (!IsDevicePathEnd (*FilePath)) { > - // > - // For file system access each node should be a file path component > - // > - if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH || > - DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP > - ) { > - FileHandle = NULL; > - return (EFI_INVALID_PARAMETER); > - } > - // > - // Open this file path node > - // > - Handle2 = Handle1; > - Handle1 = NULL; > - > - // > - // File Name Alignment Fix (FNAF) > - // Handle2->Open may be incapable of handling a unaligned CHAR16 data. > - // The structure pointed to by FilePath may be not CHAR16 aligned. > - // This code copies the potentially unaligned PathName data from the > - // FilePath structure to the aligned FnafPathName for use in the > - // calls to Handl2->Open. > - // > - > - // > - // Determine length of PathName, in bytes. > - // > - PathLen = DevicePathNodeLength (*FilePath) - SIZE_OF_FILEPATH_DEVICE_PATH; > - > - // > - // Allocate memory for the aligned copy of the string Extra allocation is to allow for forced alignment > - // Copy bytes from possibly unaligned location to aligned location > - // > - FnafPathName = AllocateCopyPool(PathLen, (UINT8 *)((FILEPATH_DEVICE_PATH*)*FilePath)->PathName); > - if (FnafPathName == NULL) { > - return EFI_OUT_OF_RESOURCES; > - } > - > - // > - // Try to test opening an existing file > - // > - Status = Handle2->Open ( > - Handle2, > - &Handle1, > - FnafPathName, > - OpenMode &~EFI_FILE_MODE_CREATE, > - 0 > - ); > - > - // > - // see if the error was that it needs to be created > - // > - if ((EFI_ERROR (Status)) && (OpenMode != (OpenMode &~EFI_FILE_MODE_CREATE))) { > - Status = Handle2->Open ( > - Handle2, > - &Handle1, > - FnafPathName, > - OpenMode, > - Attributes > - ); > - } > - > - // > - // Free the alignment buffer > - // > - FreePool(FnafPathName); > - > - // > - // Close the last node > - // > - Handle2->Close (Handle2); > - > - if (EFI_ERROR(Status)) { > - return (Status); > - } > - > - // > - // Get the next node > - // > - *FilePath = NextDevicePathNode (*FilePath); > - } > > // > // This is a weak spot since if the undefined SHELL_FILE_HANDLE format changes this must change also! > // > - *FileHandle = (VOID*)Handle1; > + *FileHandle = (VOID*)File; > return (EFI_SUCCESS); > } > > Reviewed-by: Ruiyu Ni -- Thanks, Ray