Hi Shenglei, Your explanation makes sense.
Reviewed-by: Nate DeSimone <nathaniel.l.desim...@intel.com> Thanks, Nate -----Original Message----- From: Zhang, Shenglei <shenglei.zh...@intel.com> Sent: Monday, September 16, 2019 7:48 PM To: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; devel@edk2.groups.io Cc: Kubacki, Michael A <michael.a.kuba...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>; Gao, Liming <liming....@intel.com> 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 <shenglei.zh...@intel.com>; devel@edk2.groups.io > Cc: Kubacki, Michael A <michael.a.kuba...@intel.com>; Chiu, Chasel > <chasel.c...@intel.com>; Gao, Liming <liming....@intel.com> > Subject: RE: [PATCH v2] MinPlatformPkg/TestPointCheckLib: Add return > value when OutTable is NULL > > Hi Shenglei, > > I don't see how this patch is at all related to the previous version of this > patch. What is different from the previous version is that this patch update the copyright year and the "if...else" coding style. > Also, you are introducing yet another new bug with this patch. > Moreover, this bug is unrelated to the previous bug. > > Please take a look at the function TestPointGetAcpi(). With your > change added, this function is now broken since Table is a stack > variable and it is not being initialized to zero. This function > assumes that > DumpAcpiXsdt()/DumpAcpiRsdt() will do the initialization to zero on > 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 reference code is "*OutTable = NULL"(line 667). And it will be assigned a value next. The reference code is "*OutTable = Table"(line 689). And with my changes, the stack variable Table(equivalent to *OutTable in DumpAcpiXsdt/ DumpAcpiRsdt) can still be initialized to NULL or assigned a value. What I did is intended to check the address of "Table", since there is no point to perform operations to "Table" if its address is NULL. Thanks, Shenglei > > Both this patch and the previous patch have been made carelessly and I > am not impressed. > > Thanks, > Nate > > -----Original Message----- > From: Zhang, Shenglei <shenglei.zh...@intel.com> > Sent: Sunday, September 15, 2019 6:09 PM > To: devel@edk2.groups.io > Cc: Kubacki, Michael A <michael.a.kuba...@intel.com>; Chiu, Chasel > <chasel.c...@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com>; Gao, Liming <liming....@intel.com> > Subject: [PATCH v2] MinPlatformPkg/TestPointCheckLib: Add return value > when OutTable is NULL > > Currently there is no check for the parameter OutTable. > So add the logic that return value EFI_INVALID_PARAMETER when the > OutTable is NULL. > > Cc: Michael Kubacki <michael.a.kuba...@intel.com> > Cc: Chasel Chiu <chasel.c...@intel.com> > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Signed-off-by: Shenglei Zhang <shenglei.zh...@intel.com> > --- > > v2:Update the copyright and the if...else statement coding style. > > .../Test/Library/TestPointCheckLib/DxeCheckAcpi.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > 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 > > -Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2017 - 2019, Intel Corporation. All rights > +reserved.<BR> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -610,6 +610,8 @@ DumpAcpiRsdt ( > > if (OutTable != NULL) { > *OutTable = NULL; > + } else{ > + return EFI_INVALID_PARAMETER; > } > > ReturnStatus = EFI_SUCCESS; > @@ -663,6 +665,8 @@ DumpAcpiXsdt ( > > if (OutTable != NULL) { > *OutTable = NULL; > + } else{ > + return EFI_INVALID_PARAMETER; > } > > ReturnStatus = EFI_SUCCESS; > -- > 2.18.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47436): https://edk2.groups.io/g/devel/message/47436 Mute This Topic: https://groups.io/mt/34159564/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-