Re: [PATCH 1/6] MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()

[Laszlo Ersek <lersek@redhat.com>]

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;
>> +}
>>
> 
>