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

[Laszlo Ersek <lersek@redhat.com>]

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.ni@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