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.115; helo=mga14.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 A5A3521BADAB2 for ; Fri, 27 Jul 2018 02:28:15 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Jul 2018 02:28:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,408,1526367600"; d="scan'208";a="75005571" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.4]) ([10.239.9.4]) by fmsmga004.fm.intel.com with ESMTP; 27 Jul 2018 02:28:12 -0700 To: Laszlo Ersek , edk2-devel-01 Cc: Chao Zhang , Eric Dong , Jaben Carsey , Jiaxin Wu , Jiewen Yao , Liming Gao , Michael D Kinney , Roman Bacik , Siyuan Fu , Star Zeng References: <20180718205043.17574-1-lersek@redhat.com> <20180718205043.17574-2-lersek@redhat.com> From: "Ni, Ruiyu" Message-ID: <425d0b30-a322-23c7-6d56-8f23696b86cd@Intel.com> Date: Fri, 27 Jul 2018 17:28:47 +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: <20180718205043.17574-2-lersek@redhat.com> Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath() 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: Fri, 27 Jul 2018 09:28:15 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 7/19/2018 4:50 AM, Laszlo Ersek wrote: > + // > + // Traverse the device path nodes relative to the filesystem. > + // > + while (!IsDevicePathEnd (*FilePath)) { > + // > + // Keep local variables that relate to the current device path node tightly > + // scoped. > + // > + FILEPATH_DEVICE_PATH *FilePathNode; > + CHAR16 *AlignedPathName; > + CHAR16 *PathName; > + EFI_FILE_PROTOCOL *NextFile; 1. Not sure if it follows the coding style. I would prefer to move the definition to the beginning of the function. > + > + if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH || > + DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) { > + Status = EFI_INVALID_PARAMETER; > + goto CloseLastFile; > + } > + FilePathNode = (FILEPATH_DEVICE_PATH *)*FilePath; > + > + // > + // FilePathNode->PathName may be unaligned, and the UEFI specification > + // requires pointers that are passed to protocol member functions to be > + // aligned. Create an aligned copy of the pathname if necessary. > + // > + if ((UINTN)FilePathNode->PathName % sizeof *FilePathNode->PathName == 0) { > + AlignedPathName = NULL; > + PathName = FilePathNode->PathName; > + } else { > + AlignedPathName = AllocateCopyPool ( > + (DevicePathNodeLength (FilePathNode) - > + SIZE_OF_FILEPATH_DEVICE_PATH), > + FilePathNode->PathName > + ); > + if (AlignedPathName == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto CloseLastFile; > + } > + PathName = AlignedPathName; > + } > + > + // > + // Open the next pathname fragment with EFI_FILE_MODE_CREATE masked out and > + // with Attributes set to 0. > + // > + Status = LastFile->Open ( > + LastFile, > + &NextFile, > + PathName, > + OpenMode & ~(UINT64)EFI_FILE_MODE_CREATE, > + 0 > + ); 2. As I said in previous mail, is it really needed? Per spec it's not required. Per FAT driver implementation, it's also not required. > + > + // > + // Retry with EFI_FILE_MODE_CREATE and the original Attributes if the first > + // attempt failed, and the caller specified EFI_FILE_MODE_CREATE. > + // > + if (EFI_ERROR (Status) && (OpenMode & EFI_FILE_MODE_CREATE) != 0) { > + Status = LastFile->Open ( > + LastFile, > + &NextFile, > + PathName, > + OpenMode, > + Attributes > + ); > + } > + > + // > + // Release any AlignedPathName on both error and success paths; PathName is > + // no longer needed. > + // > + if (AlignedPathName != NULL) { > + FreePool (AlignedPathName); > + } > + if (EFI_ERROR (Status)) { > + goto CloseLastFile; > + } > + > + // > + // Advance to the next device path node. > + // > + LastFile->Close (LastFile); > + LastFile = NextFile; > + *FilePath = NextDevicePathNode (FilePathNode); > + } > + > + *File = LastFile; > + return EFI_SUCCESS; > + > +CloseLastFile: > + LastFile->Close (LastFile); > + > + ASSERT (EFI_ERROR (Status)); 3. ASSERT_EFI_ERROR (Status); > + return Status; > +} > -- Thanks, Ray