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.
>
>> +
>> + 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?
>
>> +
>> + //
>> + // 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.
//
Thanks!
Laszlo
>
>> + return Status;
>> +}
>>
>
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
