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

["Laszlo Ersek" <lersek@redhat.com>]

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