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 <[email protected]> Thanks Liming >-----Original Message----- >From: Laszlo Ersek [mailto:[email protected]] >Sent: Monday, July 30, 2018 10:14 PM >To: Ni, Ruiyu <[email protected]>; edk2-devel-01 <[email protected]> >Cc: Zhang, Chao B <[email protected]>; Dong, Eric ><[email protected]>; Carsey, Jaben <[email protected]>; Wu, Jiaxin ><[email protected]>; Yao, Jiewen <[email protected]>; Gao, Liming ><[email protected]>; Kinney, Michael D <[email protected]>; >Roman Bacik <[email protected]>; Fu, Siyuan ><[email protected]>; Zeng, Star <[email protected]> >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 <[email protected]> > >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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
