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!
Laszlo
+ return Status;
+}
--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
