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
