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]
-=-=-=-=-=-=-=-=-=-=-=-
