From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (using TLSv1 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0923D1A1E46 for ; Mon, 24 Oct 2016 16:05:20 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP; 24 Oct 2016 16:05:20 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,543,1473145200"; d="scan'208";a="23194311" Received: from orsmsx106.amr.corp.intel.com ([10.22.225.133]) by fmsmga005.fm.intel.com with ESMTP; 24 Oct 2016 16:05:19 -0700 Received: from orsmsx158.amr.corp.intel.com (10.22.240.20) by ORSMSX106.amr.corp.intel.com (10.22.225.133) with Microsoft SMTP Server (TLS) id 14.3.248.2; Mon, 24 Oct 2016 16:05:19 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.50]) by ORSMSX158.amr.corp.intel.com ([10.22.240.20]) with mapi id 14.03.0248.002; Mon, 24 Oct 2016 16:05:18 -0700 From: "Kinney, Michael D" To: Laszlo Ersek , "Gao, Liming" , "Kinney, Michael D" CC: edk2-devel-01 Thread-Topic: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR() Thread-Index: AQHSK+H8HrgS1BpVOk2rwDlzLm52ZqC4kKAA//+qaGA= Date: Mon, 24 Oct 2016 23:05:17 +0000 Message-ID: References: <20161021212737.15974-1-lersek@redhat.com> <20161021212737.15974-2-lersek@redhat.com> <567fc440-ce97-548d-e645-6ae84dfbf488@redhat.com> In-Reply-To: <567fc440-ce97-548d-e645-6ae84dfbf488@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTYwOWYzNzctNzU2NS00ZGI4LTg4ODAtN2JlOWVkNTZjY2RiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlNcL3lDQ1Zmd0V0V0pmbjlYbkszZndcL2xMdTNQSkhiVHB1elVzejkwaXJ4ST0ifQ== x-originating-ip: [10.22.254.140] 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: Mon, 24 Oct 2016 23:05:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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=20 for libraries of type BASE. I am surprised we did not run into an issue before that would have required the introduction of this macro earlier. Unless the workaround has been to add #include of=20 , 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 feedback I was considering was=20 to implement ASSERT_EFI_ERROR() using=20 ASSERT_RETURN_ERROR(), but that might not always be the right mapping because the RETURN_* 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 > > Cc: edk2-devel-01 > Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ER= ROR() >=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 less > > convenient and less informative: ASSERT_EFI_ERROR() prints the actual > > StatusParameter. > > > > Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy t= he > > 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 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 DEBUG_PROPERTY_DEBUG_ASSERT_= ENABLED > > + bit of PcdDebugProperyMask is set, then this macro evaluates the > > + RETURN_STATUS value specified by StatusParameter. If StatusParamete= r is an > > + error code, then DebugAssert() is called passing in the source filen= ame, > > + source line number, and StatusParameter. > > + > > + @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 > series is ready to go, but it depends on this patch. >=20 > Thanks! > Laszlo