On 08/02/18 06:06, Gao, Liming wrote: > Laszlo: > I have no other comments. IntelFrameworkPkg has another UefiLib library > instance FrameworkUefiLib. Could you help update it also? > > For MdePkg, you can add Reviewed-by: Liming Gao <liming....@intel.com>
Thanks! I have now enough feedback for posting v2. Cheers Laszlo > > Thanks > Liming >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Monday, July 30, 2018 10:14 PM >> To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org> >> Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Dong, Eric >> <eric.d...@intel.com>; Carsey, Jaben <jaben.car...@intel.com>; Wu, Jiaxin >> <jiaxin...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Gao, Liming >> <liming....@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; >> Roman Bacik <roman.ba...@broadcom.com>; Fu, Siyuan >> <siyuan...@intel.com>; Zeng, Star <star.z...@intel.com> >> Subject: Re: [PATCH 1/6] MdePkg/UefiLib: introduce >> EfiOpenFileByDevicePath() >> >> On 07/30/18 03:54, Ni, Ruiyu wrote: >>> 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 <ruiyu...@intel.com> >> >> Thanks, Ray -- looks like I've almost got enough feedback for posting >> v2; however I haven't received any MdePkg maintainer feedback (from Mike >> and/or Liming) yet. Am I to understand your review as a substitute for >> theirs, or as an addition to theirs? >> >> Thanks! >> Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
