From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: nathaniel.l.desimone@intel.com) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by groups.io with SMTP; Tue, 17 Sep 2019 14:46:53 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Sep 2019 14:46:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,518,1559545200"; d="scan'208";a="361985591" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by orsmga005.jf.intel.com with ESMTP; 17 Sep 2019 14:46:52 -0700 Received: from orsmsx157.amr.corp.intel.com (10.22.240.23) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Sep 2019 14:46:52 -0700 Received: from orsmsx114.amr.corp.intel.com ([169.254.8.55]) by ORSMSX157.amr.corp.intel.com ([169.254.9.194]) with mapi id 14.03.0439.000; Tue, 17 Sep 2019 14:46:52 -0700 From: "Nate DeSimone" To: "Zhang, Shenglei" , "devel@edk2.groups.io" CC: "Kubacki, Michael A" , "Chiu, Chasel" , "Gao, Liming" Subject: Re: [PATCH v2] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL Thread-Topic: [PATCH v2] MinPlatformPkg/TestPointCheckLib: Add return value when OutTable is NULL Thread-Index: AQHVbCtZhN3Yp8lbakmLW+XYWv13Nacuq/mwgAD1zoCAAMg90A== Date: Tue, 17 Sep 2019 21:46:51 +0000 Message-ID: <02A34F284D1DA44BB705E61F7180EF0AAEEEFDCB@ORSMSX114.amr.corp.intel.com> References: <20190916010857.42264-1-shenglei.zhang@intel.com> <02A34F284D1DA44BB705E61F7180EF0AAEED9252@ORSMSX114.amr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTI4ZTlkNjAtMzBmNS00YjUxLWE5ZWEtZTViODc3Zjk5ZGZiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoib2V5Uml3TmUzYlRCZWhQYlU2Y2FEdFcyWjZKYzFCSG9XN25lRTFGcVkrcjMzaGJDSXljcEdXQUlESXZNUzRCYSJ9 x-ctpclassification: CTP_NT x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Return-Path: nathaniel.l.desimone@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Shenglei, Your explanation makes sense. Reviewed-by: Nate DeSimone Thanks, Nate -----Original Message----- From: Zhang, Shenglei =20 Sent: Monday, September 16, 2019 7:48 PM To: Desimone, Nathaniel L ; devel@edk2.grou= ps.io Cc: Kubacki, Michael A ; Chiu, Chasel ; Gao, Liming Subject: RE: [PATCH v2] MinPlatformPkg/TestPointCheckLib: Add return value = when OutTable is NULL Hi Nathaniel, Thanks for your comments and below is my response. > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Tuesday, September 17, 2019 3:25 AM > To: Zhang, Shenglei ; devel@edk2.groups.io > Cc: Kubacki, Michael A ; Chiu, Chasel=20 > ; Gao, Liming > Subject: RE: [PATCH v2] MinPlatformPkg/TestPointCheckLib: Add return=20 > value when OutTable is NULL >=20 > Hi Shenglei, >=20 > I don't see how this patch is at all related to the previous version of t= his patch. What is different from the previous version is that this patch update the c= opyright year and the "if...else" coding style. > Also, you are introducing yet another new bug with this patch.=20 > Moreover, this bug is unrelated to the previous bug. >=20 > Please take a look at the function TestPointGetAcpi(). With your=20 > change added, this function is now broken since Table is a stack=20 > variable and it is not being initialized to zero. This function=20 > assumes that > DumpAcpiXsdt()/DumpAcpiRsdt() will do the initialization to zero on=20 > it's behalf, you have broken this assumption with your change. I have taken a look at the function TestPointGetAcpi(). The variable Table = is first initialized to NULL by DumpAcpiXsdt()/DumpAcpiRsdt(). The referenc= e code is "*OutTable =3D NULL"(line 667). And it will be assigned a value n= ext. The reference code is "*OutTable =3D Table"(line 689). And with my changes, the stack variable Table(equivalent to *OutTable in D= umpAcpiXsdt/ DumpAcpiRsdt) can still be initialized to NULL or assigned a v= alue. What I did is intended to check the address of "Table", since there i= s no point to perform operations to "Table" if its address is NULL. Thanks, Shenglei >=20 > Both this patch and the previous patch have been made carelessly and I=20 > am not impressed. >=20 > Thanks, > Nate >=20 > -----Original Message----- > From: Zhang, Shenglei > Sent: Sunday, September 15, 2019 6:09 PM > To: devel@edk2.groups.io > Cc: Kubacki, Michael A ; Chiu, Chasel=20 > ; Desimone, Nathaniel L=20 > ; Gao, Liming > Subject: [PATCH v2] MinPlatformPkg/TestPointCheckLib: Add return value=20 > when OutTable is NULL >=20 > Currently there is no check for the parameter OutTable. > So add the logic that return value EFI_INVALID_PARAMETER when the=20 > OutTable is NULL. >=20 > Cc: Michael Kubacki > Cc: Chasel Chiu > Cc: Nate DeSimone > Cc: Liming Gao > Signed-off-by: Shenglei Zhang > --- >=20 > v2:Update the copyright and the if...else statement coding style. >=20 > .../Test/Library/TestPointCheckLib/DxeCheckAcpi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) >=20 > diff --git > a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec > k > Acpi.c > b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec > k > Acpi.c > index 263781a2..83736bf3 100644 > --- > a/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeChec > k > Acpi.c > +++ > b/Platform/Intel/MinPlatformPkg/Test/Library/TestPointCheckLib/DxeCh > +++ eckAcpi.c > @@ -1,6 +1,6 @@ > /** @file >=20 > -Copyright (c) 2017, Intel Corporation. All rights reserved.
> +Copyright (c) 2017 - 2019, Intel Corporation. All rights=20 > +reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > **/ > @@ -610,6 +610,8 @@ DumpAcpiRsdt ( >=20 > if (OutTable !=3D NULL) { > *OutTable =3D NULL; > + } else{ > + return EFI_INVALID_PARAMETER; > } >=20 > ReturnStatus =3D EFI_SUCCESS; > @@ -663,6 +665,8 @@ DumpAcpiXsdt ( >=20 > if (OutTable !=3D NULL) { > *OutTable =3D NULL; > + } else{ > + return EFI_INVALID_PARAMETER; > } >=20 > ReturnStatus =3D EFI_SUCCESS; > -- > 2.18.0.windows.1