From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 945D91A1E57 for ; Tue, 25 Oct 2016 01:22:55 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP; 25 Oct 2016 01:22:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,545,1473145200"; d="scan'208";a="183503899" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 25 Oct 2016 01:22:55 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 25 Oct 2016 01:22:55 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 25 Oct 2016 01:22:54 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.206]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.2]) with mapi id 14.03.0248.002; Tue, 25 Oct 2016 16:22:52 +0800 From: "Zeng, Star" To: "Kinney, Michael D" , Laszlo Ersek , "Gao, Liming" CC: edk2-devel-01 , "Zeng, Star" Thread-Topic: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() Thread-Index: AQHSK+IJUMwS9ZUxsUig4I40c+b9SaC3lSsAgAAjIICAASEVUA== Date: Tue, 25 Oct 2016 08:22:51 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103959121C@shsmsx102.ccr.corp.intel.com> References: <20161021212737.15974-1-lersek@redhat.com> <20161021212737.15974-2-lersek@redhat.com> <567fc440-ce97-548d-e645-6ae84dfbf488@redhat.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2016 08:22:55 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable In fact, EFI_* codes are a subset of RETURN_* codes, so it seems work to im= plement ASSERT_EFI_ERROR() using new ASSERT_RETURN_ERROR(). Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinn= ey, Michael D Sent: Tuesday, October 25, 2016 7:05 AM To: Laszlo Ersek ; Gao, Liming ; K= inney, Michael D Cc: edk2-devel-01 Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERRO= R() Hi Laszlo, Sorry for the delay. I was traveling last week. I did see this and I have been thinking about it. I think it does make sense to add this new macro for libraries of type BASE= . I am surprised we did not run into an issue before that would have requi= red the introduction of this macro earlier. Unless the workaround has been= to add #include of , which makes me think we should = review BASE libraries to make sure that extra include is not present. The EFI_* error codes are mapped to RETURN_* error codes. So the only feed= back I was considering was to implement ASSERT_EFI_ERROR() using ASSERT_RET= URN_ERROR(), but that might not always be the right mapping because the RET= URN_* codes are a subset of EFI_* error codes. Reviewed-by: Michael Kinney Best regards, Mike > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Monday, October 24, 2016 2:00 PM > To: Kinney, Michael D ; Gao, Liming=20 > > Cc: edk2-devel-01 > Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add=20 > ASSERT_RETURN_ERROR() >=20 > Mike, Liming, >=20 > On 10/21/16 23:27, Laszlo Ersek wrote: > > ASSERT_EFI_ERROR() cannot be used in BASE type modules because > > - the replacement text calls EFI_ERROR(), > > - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h", > > - the inclusion of "UefiBaseType.h" is not required for BASE type modul= es. > > > > While > > > > ASSERT (!RETURN_ERROR (StatusParameter)) > > > > would be a functional statement in BASE type modules, it would be=20 > > less convenient and less informative: ASSERT_EFI_ERROR() prints the=20 > > actual StatusParameter. > > > > Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR().=20 > > Copy the original macro definition and update it as follows: > > - replace EFI with RETURN, > > - wrap overlong lines in the comment block and in the code, > > - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead. > > > > Cc: Liming Gao > > Cc: Michael D Kinney > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D166 > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Laszlo Ersek > > --- > > > > Notes: > > OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, = is > > one such BASE module. > > > > MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/MdePkg/Include/Library/DebugLib.h=20 > > b/MdePkg/Include/Library/DebugLib.h > > index 81904325703f..3a910e6a208b 100644 > > --- a/MdePkg/Include/Library/DebugLib.h > > +++ b/MdePkg/Include/Library/DebugLib.h > > @@ -348,6 +348,33 @@ DebugPrintLevelEnabled ( > > #define ASSERT_EFI_ERROR(StatusParameter) #endif > > > > +/** > > + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an er= ror code. > > + > > + If MDEPKG_NDEBUG is not defined and the=20 > > + DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED > > + bit of PcdDebugProperyMask is set, then this macro evaluates the =20 > > + RETURN_STATUS value specified by StatusParameter. If=20 > > + StatusParameter is an error code, then DebugAssert() is called=20 > > + passing in the source filename, source line number, and StatusParame= ter. > > + > > + @param StatusParameter RETURN_STATUS value to evaluate. > > + > > +**/ > > +#if !defined(MDEPKG_NDEBUG) > > + #define ASSERT_RETURN_ERROR(StatusParameter) = \ > > + do { = \ > > + if (DebugAssertEnabled ()) { = \ > > + if (RETURN_ERROR (StatusParameter)) { = \ > > + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status =3D %r)\= n", \ > > + StatusParameter)); = \ > > + _ASSERT (!RETURN_ERROR (StatusParameter)); = \ > > + } = \ > > + } = \ > > + } while (FALSE) > > +#else > > + #define ASSERT_RETURN_ERROR(StatusParameter) > > +#endif > > + > > /** > > Macro that calls DebugAssert() if a protocol is already installed in= the > > handle database. > > >=20 > can I please get a maintainer review for this patch? The rest of the=20 > series is ready to go, but it depends on this patch. >=20 > Thanks! > Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel