From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.126; helo=mga18.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (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 C6628210C2829 for ; Sun, 29 Jul 2018 18:54:00 -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 orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jul 2018 18:54:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,421,1526367600"; d="scan'208";a="75595745" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.4]) ([10.239.9.4]) by fmsmga004.fm.intel.com with ESMTP; 29 Jul 2018 18:53:58 -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> <425d0b30-a322-23c7-6d56-8f23696b86cd@Intel.com> <9bb5eed1-f8b0-8d22-e801-53ba7a06cdc5@redhat.com> From: "Ni, Ruiyu" Message-ID: <57ac2b07-41fc-d01d-e20b-9be4a68a0f1b@Intel.com> Date: Mon, 30 Jul 2018 09:54:34 +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: <9bb5eed1-f8b0-8d22-e801-53ba7a06cdc5@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: Mon, 30 Jul 2018 01:54:00 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit On 7/27/2018 8:06 PM, Laszlo Ersek wrote: > On 07/27/18 11:28, Ni, Ruiyu wrote: >> 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. > > OK, will do. Thanks! > >> >>> + >>> +    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. > > I can do that, but it's out of scope for this series. The behavior that > you point out is not a functionality bug (it is not observably erroneous > behavior), just sub-optimal implementation. This series is about > unifying the implementation and fixing those issues that are actual > bugs. I suggest that we report a separate BZ about this question, > discuss it separately, and then I can send a separate patch (which will > benefit all client code at once). > > Does that sound acceptable? I agree. > >> >>> + >>> +    // >>> +    // 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); > > No, that's not correct; I *really* meant > > ASSERT (EFI_ERROR (Status)) > > Please find the explanation here: > > https://lists.01.org/pipermail/edk2-devel/2018-July/027288.html > > However, given that both Jaben and you were confused by this, I agree > that I should add a comment before the assert: > > // > // We are on the error path; we must have set an error Status for > // returning to the caller. > // I just found there is no "!" before "EFI_ERROR". It's really confusing. I agree a comment before that is better. Thanks! With the comment added, Reviewed-by: Ruiyu Ni > > Thanks! > Laszlo > >> >>> +  return Status; >>> +} >>> >> >> > -- Thanks, Ray