[edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs

Laszlo Ersek posted 7 patches 6 years, 3 months ago
Failed in applying to current master (apply log)
IntelFrameworkPkg/Library/FrameworkUefiLib/FrameworkUefiLib.inf                      |   1 +
IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c                                 | 227 ++++++++++++++++++++
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf                                |   1 -
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c                         | 140 ------------
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c                                 |   2 +-
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h                                 |  39 ----
MdePkg/Include/Library/UefiLib.h                                                     |  88 ++++++++
MdePkg/Library/UefiLib/UefiLib.c                                                     | 227 ++++++++++++++++++++
MdePkg/Library/UefiLib/UefiLib.inf                                                   |   1 +
NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf                                     |   1 -
NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c                                      | 141 +-----------
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf        |   1 -
SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c | 151 +------------
ShellPkg/Include/Library/ShellLib.h                                                  |   2 -
ShellPkg/Library/UefiShellLib/UefiShellLib.c                                         | 118 +---------
ShellPkg/Library/UefiShellLib/UefiShellLib.inf                                       |   3 +-
16 files changed, 552 insertions(+), 591 deletions(-)
[edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
Posted by Laszlo Ersek 6 years, 3 months ago
Repo:   https://github.com/lersek/edk2.git
Branch: open_file_by_devpath_tiano_1008_v2

This is version 2 of the patch set that was originally posted at:

  https://lists.01.org/pipermail/edk2-devel/2018-July/027253.html

for <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.

Changes are noted on every patch.

The cumulative code difference is very small (not counting the
FrameworkUefiLib copy of the function), so I'm including it here for
easier review:

> diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
> index 2468bf2aee80..f950f1c9c648 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -1554,9 +1554,11 @@ EfiLocateProtocolBuffer (
>    traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
>
>    @param[in,out] FilePath  On input, the device path to the file or directory
> -                           to open or create. On output, FilePath points one
> -                           past the last node in the original device path that
> -                           has been successfully processed. FilePath is set on
> +                           to open or create. The caller is responsible for
> +                           ensuring that the device path pointed-to by FilePath
> +                           is well-formed. On output, FilePath points one past
> +                           the last node in the original device path that has
> +                           been successfully processed. FilePath is set on
>                             output even if EfiOpenFileByDevicePath() returns an
>                             error.
>
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
> index d3e290178cd9..7bcac5613768 100644
> --- a/MdePkg/Library/UefiLib/UefiLib.c
> +++ b/MdePkg/Library/UefiLib/UefiLib.c
> @@ -1751,9 +1751,11 @@ EfiLocateProtocolBuffer (
>    traversal is stopped with an error, and a NULL EFI_FILE_PROTOCOL is output.
>
>    @param[in,out] FilePath  On input, the device path to the file or directory
> -                           to open or create. On output, FilePath points one
> -                           past the last node in the original device path that
> -                           has been successfully processed. FilePath is set on
> +                           to open or create. The caller is responsible for
> +                           ensuring that the device path pointed-to by FilePath
> +                           is well-formed. On output, FilePath points one past
> +                           the last node in the original device path that has
> +                           been successfully processed. FilePath is set on
>                             output even if EfiOpenFileByDevicePath() returns an
>                             error.
>
> @@ -1808,6 +1810,10 @@ EfiOpenFileByDevicePath (
>    EFI_HANDLE                      FileSystemHandle;
>    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FileSystem;
>    EFI_FILE_PROTOCOL               *LastFile;
> +  FILEPATH_DEVICE_PATH            *FilePathNode;
> +  CHAR16                          *AlignedPathName;
> +  CHAR16                          *PathName;
> +  EFI_FILE_PROTOCOL               *NextFile;
>
>    if (File == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -1854,15 +1860,6 @@ EfiOpenFileByDevicePath (
>    // 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;
> -
>      if (DevicePathType (*FilePath) != MEDIA_DEVICE_PATH ||
>          DevicePathSubType (*FilePath) != MEDIA_FILEPATH_DP) {
>        Status = EFI_INVALID_PARAMETER;
> @@ -1942,6 +1939,10 @@ EfiOpenFileByDevicePath (
>  CloseLastFile:
>    LastFile->Close (LastFile);
>
> +  //
> +  // We are on the error path; we must have set an error Status for returning
> +  // to the caller.
> +  //
>    ASSERT (EFI_ERROR (Status));
>    return Status;
>  }
> diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> index 312a92d7461a..aef85c470143 100644
> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> @@ -163,7 +163,7 @@ UpdatePage(
>
>    gSecureBootPrivateData->FileContext->FileName = FileName;
>
> -  EfiOpenFileByDevicePath(
> +  EfiOpenFileByDevicePath (
>      &FilePath,
>      &gSecureBootPrivateData->FileContext->FHandle,
>      EFI_FILE_MODE_READ,

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Roman Bacik <roman.bacik@broadcom.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Thanks,
Laszlo

Laszlo Ersek (7):
  MdePkg/UefiLib: introduce EfiOpenFileByDevicePath()
  IntelFrameworkPkg/FrameworkUefiLib: introduce
    EfiOpenFileByDevicePath()
  MdeModulePkg/RamDiskDxe: replace OpenFileByDevicePath() with UefiLib
    API
  NetworkPkg/TlsAuthConfigDxe: replace OpenFileByDevicePath() with
    UefiLib API
  SecurityPkg/SecureBootConfigDxe: replace OpenFileByDevicePath() with
    UefiLib API
  ShellPkg/UefiShellLib: drop DeviceHandle param of
    ShellOpenFileByDevicePath()
  ShellPkg/UefiShellLib: rebase ShellOpenFileByDevicePath() to UefiLib
    API

 IntelFrameworkPkg/Library/FrameworkUefiLib/FrameworkUefiLib.inf                      |   1 +
 IntelFrameworkPkg/Library/FrameworkUefiLib/UefiLib.c                                 | 227 ++++++++++++++++++++
 MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf                                |   1 -
 MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c                         | 140 ------------
 MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.c                                 |   2 +-
 MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskImpl.h                                 |  39 ----
 MdePkg/Include/Library/UefiLib.h                                                     |  88 ++++++++
 MdePkg/Library/UefiLib/UefiLib.c                                                     | 227 ++++++++++++++++++++
 MdePkg/Library/UefiLib/UefiLib.inf                                                   |   1 +
 NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf                                     |   1 -
 NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c                                      | 141 +-----------
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf        |   1 -
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c | 151 +------------
 ShellPkg/Include/Library/ShellLib.h                                                  |   2 -
 ShellPkg/Library/UefiShellLib/UefiShellLib.c                                         | 118 +---------
 ShellPkg/Library/UefiShellLib/UefiShellLib.inf                                       |   3 +-
 16 files changed, 552 insertions(+), 591 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
Posted by Laszlo Ersek 6 years, 3 months ago
On 08/03/18 14:15, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: open_file_by_devpath_tiano_1008_v2
> 
> This is version 2 of the patch set that was originally posted at:
> 
>   https://lists.01.org/pipermail/edk2-devel/2018-July/027253.html
> 
> for <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.
> 
> Changes are noted on every patch.

Liming, Mike, can you please check patches #1 and #2?

(Obviously I'll only push the series after the quiet period ends.)

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
Posted by Laszlo Ersek 6 years, 2 months ago
On 08/09/18 15:30, Laszlo Ersek wrote:
> On 08/03/18 14:15, Laszlo Ersek wrote:
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: open_file_by_devpath_tiano_1008_v2
>>
>> This is version 2 of the patch set that was originally posted at:
>>
>>   https://lists.01.org/pipermail/edk2-devel/2018-July/027253.html
>>
>> for <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.
>>
>> Changes are noted on every patch.
> 
> Liming, Mike, can you please check patches #1 and #2?
> 
> (Obviously I'll only push the series after the quiet period ends.)

pinging this set again

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
Posted by Gao, Liming 6 years, 2 months ago
Laszlo:
  Sorry to miss the patch. Thank you to update IntelFrameworkPkg UefiLib. The change in MdePkg and IntelFrameworkPkg is good to me. Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, August 15, 2018 10:21 AM
> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Carsey, Jaben
> <jaben.carsey@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
> 
> On 08/09/18 15:30, Laszlo Ersek wrote:
> > On 08/03/18 14:15, Laszlo Ersek wrote:
> >> Repo:   https://github.com/lersek/edk2.git
> >> Branch: open_file_by_devpath_tiano_1008_v2
> >>
> >> This is version 2 of the patch set that was originally posted at:
> >>
> >>   https://lists.01.org/pipermail/edk2-devel/2018-July/027253.html
> >>
> >> for <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.
> >>
> >> Changes are noted on every patch.
> >
> > Liming, Mike, can you please check patches #1 and #2?
> >
> > (Obviously I'll only push the series after the quiet period ends.)
> 
> pinging this set again
> 
> Thanks
> Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
Posted by Laszlo Ersek 6 years, 2 months ago
On 08/15/18 19:42, Gao, Liming wrote:
> Laszlo:
>   Sorry to miss the patch. Thank you to update IntelFrameworkPkg UefiLib. The change in MdePkg and IntelFrameworkPkg is good to me. Reviewed-by: Liming Gao <liming.gao@intel.com>

Thank you.

Series pushed as commit range 52047be02430..9becf2f0759e.

Laszlo

> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, August 15, 2018 10:21 AM
>> To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Wu, Jiaxin
>> <jiaxin.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>; Carsey, Jaben
>> <jaben.carsey@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: Re: [edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
>>
>> On 08/09/18 15:30, Laszlo Ersek wrote:
>>> On 08/03/18 14:15, Laszlo Ersek wrote:
>>>> Repo:   https://github.com/lersek/edk2.git
>>>> Branch: open_file_by_devpath_tiano_1008_v2
>>>>
>>>> This is version 2 of the patch set that was originally posted at:
>>>>
>>>>   https://lists.01.org/pipermail/edk2-devel/2018-July/027253.html
>>>>
>>>> for <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.
>>>>
>>>> Changes are noted on every patch.
>>>
>>> Liming, Mike, can you please check patches #1 and #2?
>>>
>>> (Obviously I'll only push the series after the quiet period ends.)
>>
>> pinging this set again
>>
>> Thanks
>> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 0/7] UefiLib: centralize OpenFileByDevicePath() and fix its bugs
Posted by Laszlo Ersek 6 years, 3 months ago
On 08/03/18 14:15, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: open_file_by_devpath_tiano_1008_v2
> 
> This is version 2 of the patch set that was originally posted at:
> 
>   https://lists.01.org/pipermail/edk2-devel/2018-July/027253.html
> 
> for <https://bugzilla.tianocore.org/show_bug.cgi?id=1008>.
> 
> Changes are noted on every patch.
> 
> The cumulative code difference is very small (not counting the
> FrameworkUefiLib copy of the function), so I'm including it here for
> easier review:
> 
>> [...]

I should use this opportunity to highlight an awesome community feature
from Paolo and Fam (CC'd): the patchew.org website has been tracking
edk2 patch submissions for a while:

https://patchew.org/
https://patchew.org/EDK2/

and now it offers side-by-side comparison between versions of the same
patch set ("interdiff"). For example, the link for this (v2) series is:

https://patchew.org/EDK2/20180803121537.32123-1-lersek@redhat.com/

and if you click the "Diff against v1" link, you get:

https://patchew.org/EDK2/20180718205043.17574-1-lersek@redhat.com/diff/20180803121537.32123-1-lersek@redhat.com/

I recommend that all edk2 reviewers make use of this feature, for
incrementally reviewing patch series.

(This is another good reason for keeping the source code lines limited
to 80 columns -- if you write 200 character long lines, you won't have a
good time looking at side-by-side diffs!)

Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel