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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

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!
> Laszlo
> 
>>
>>> +  return Status;
>>> +}
>>>
>>
>>
> 


-- 
Thanks,
Ray