1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v2 0/6] Mark the File parameter as OPTIONAL
  4. View patch

[edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.

Guomin Jiang posted 6 patches 5 years, 9 months ago
[edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
Posted by Guomin Jiang 5 years, 9 months ago 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652

According to the description, the File is OPTIONAL and can be NULL.

Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
---
 MdePkg/Include/Protocol/Security2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Include/Protocol/Security2.h b/MdePkg/Include/Protocol/Security2.h
index 2d85b4ba9f..6a63009956 100644
--- a/MdePkg/Include/Protocol/Security2.h
+++ b/MdePkg/Include/Protocol/Security2.h
@@ -80,7 +80,7 @@ typedef struct _EFI_SECURITY2_ARCH_PROTOCOL    EFI_SECURITY2_ARCH_PROTOCOL;
 **/
 typedef EFI_STATUS (EFIAPI *EFI_SECURITY2_FILE_AUTHENTICATION) (
   IN CONST EFI_SECURITY2_ARCH_PROTOCOL *This,
-  IN CONST EFI_DEVICE_PATH_PROTOCOL    *DevicePath,
+  IN CONST EFI_DEVICE_PATH_PROTOCOL    *DevicePath, OPTIONAL
   IN VOID                              *FileBuffer,
   IN UINTN                             FileSize,
   IN BOOLEAN                           BootPolicy
-- 
2.25.1.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57460): https://edk2.groups.io/g/devel/message/57460
Mute This Topic: https://groups.io/mt/73050539/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
Posted by Laszlo Ersek 5 years, 9 months ago 
On 04/16/20 09:33, Guomin Jiang wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
> 
> According to the description, the File is OPTIONAL and can be NULL.
> 
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  MdePkg/Include/Protocol/Security2.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Include/Protocol/Security2.h b/MdePkg/Include/Protocol/Security2.h
> index 2d85b4ba9f..6a63009956 100644
> --- a/MdePkg/Include/Protocol/Security2.h
> +++ b/MdePkg/Include/Protocol/Security2.h
> @@ -80,7 +80,7 @@ typedef struct _EFI_SECURITY2_ARCH_PROTOCOL    EFI_SECURITY2_ARCH_PROTOCOL;
>  **/
>  typedef EFI_STATUS (EFIAPI *EFI_SECURITY2_FILE_AUTHENTICATION) (
>    IN CONST EFI_SECURITY2_ARCH_PROTOCOL *This,
> -  IN CONST EFI_DEVICE_PATH_PROTOCOL    *DevicePath,
> +  IN CONST EFI_DEVICE_PATH_PROTOCOL    *DevicePath, OPTIONAL
>    IN VOID                              *FileBuffer,
>    IN UINTN                             FileSize,
>    IN BOOLEAN                           BootPolicy
> 

(1) I think writing

  DevicePath OPTIONAL,

is a bit more idiomatic than

  DevicePath, OPTIONAL

However, the UEFI spec contains examples for both styles of comma
placement in parameter lists; and so I think this change is valid,
albeit somewhat unusual. So that's good.

(2) Unfortunately, I don't think the patch is complete. If you look at
the entire function declaration in the header file, the documentation
*inconsistently* calls the parameter both "File" and "DevicePath". If
you search the leading comment, there are both "File" and "DevicePath"
references.

Therefore the reference in the commit message, "According to the
description, the File is OPTIONAL", is actually a dangling reference,
because *that* particular part of the documentation calls the parameter
"File".

Please rework this patch so that the documentation and the parameter
list fully agree on the parameter's name.

According to the platform init spec (1.7),
EFI_SECURITY2_ARCH_PROTOCOL.FileAuthentication(), the name of the
parameter is "DevicePath". So please replace all relevant "File"
references in the leading comment with "DevicePath".

(3) Also, I think it would be prudent to file a ticket in Mantis, to
mark "DevicePath" as OPTIONAL in the spec too.

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57620): https://edk2.groups.io/g/devel/message/57620
Mute This Topic: https://groups.io/mt/73050539/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
Posted by Liming Gao 5 years, 9 months ago 
Guomin:
  I find this parameter name doesn't match the one in function description. Can you make them be same? With this change, Reviewed-by: Liming Gao <liming.gao@intel.com>

  @param  File             A pointer to the device path of the file that is
                           being dispatched. This will optionally be used for logging.

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guomin Jiang
> Sent: Thursday, April 16, 2020 3:34 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2652
> 
> According to the description, the File is OPTIONAL and can be NULL.
> 
> Signed-off-by: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> ---
>  MdePkg/Include/Protocol/Security2.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Include/Protocol/Security2.h b/MdePkg/Include/Protocol/Security2.h
> index 2d85b4ba9f..6a63009956 100644
> --- a/MdePkg/Include/Protocol/Security2.h
> +++ b/MdePkg/Include/Protocol/Security2.h
> @@ -80,7 +80,7 @@ typedef struct _EFI_SECURITY2_ARCH_PROTOCOL    EFI_SECURITY2_ARCH_PROTOCOL;
>  **/
> 
>  typedef EFI_STATUS (EFIAPI *EFI_SECURITY2_FILE_AUTHENTICATION) (
> 
>    IN CONST EFI_SECURITY2_ARCH_PROTOCOL *This,
> 
> -  IN CONST EFI_DEVICE_PATH_PROTOCOL    *DevicePath,
> 
> +  IN CONST EFI_DEVICE_PATH_PROTOCOL    *DevicePath, OPTIONAL
> 
>    IN VOID                              *FileBuffer,
> 
>    IN UINTN                             FileSize,
> 
>    IN BOOLEAN                           BootPolicy
> 
> --
> 2.25.1.windows.1
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#57460): https://edk2.groups.io/g/devel/message/57460
> Mute This Topic: https://groups.io/mt/73050539/1759384
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [liming.gao@intel.com]
> -=-=-=-=-=-=


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#57741): https://edk2.groups.io/g/devel/message/57741
Mute This Topic: https://groups.io/mt/73050539/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.