From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.158.1587382084890245379 for ; Mon, 20 Apr 2020 04:28:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fK/VkV49; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1587382084; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=L7pM8ZIDScAALziIdQMrgXDYtfQbK9BJ3s30fxK09qg=; b=fK/VkV49SOqL5r66U0FGZUG4dhQtspaxZdRZwS2plobGX1sgYfVTi0e1zAc2irtnlmJpgn ArUrTf6QH7wHC1vNlJ3YgrVeZmcqcrmN6SaOV0HKxsoiqaP/uoN3ERCuuTfctWshwX0GT8 D8kFoIN+CHAs2Vt7BF5JYDOPjVXTPpI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-467-SgTwo42MN3mXQ_2yRp0BzQ-1; Mon, 20 Apr 2020 07:27:58 -0400 X-MC-Unique: SgTwo42MN3mXQ_2yRp0BzQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5AF238017FF; Mon, 20 Apr 2020 11:27:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-228.ams2.redhat.com [10.36.114.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5A5FB5C1B2; Mon, 20 Apr 2020 11:27:56 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 6/6] MdePkg/Security2: Mark the File parameter as OPTIONAL. To: devel@edk2.groups.io, guomin.jiang@intel.com Cc: Michael D Kinney , Liming Gao References: <20200416073354.2232-1-guomin.jiang@intel.com> <20200416073354.2232-7-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: <666e5fce-24a0-7f00-7ad2-15e66df99a77@redhat.com> Date: Mon, 20 Apr 2020 13:27:55 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200416073354.2232-7-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 > Cc: Michael D Kinney > Cc: Liming Gao > --- > 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