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

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

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.

> +
> +    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.

> +
> +    //
> +    // 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);

> +  return Status;
> +}
> 


-- 
Thanks,
Ray