From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=liming.gao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3BB2D21A07A80 for ; Fri, 9 Nov 2018 07:32:07 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Nov 2018 07:32:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,483,1534834800"; d="scan'208";a="272742733" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga005.jf.intel.com with ESMTP; 09 Nov 2018 07:32:07 -0800 Received: from fmsmsx117.amr.corp.intel.com (10.18.116.17) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 9 Nov 2018 07:32:06 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx117.amr.corp.intel.com (10.18.116.17) with Microsoft SMTP Server (TLS) id 14.3.408.0; Fri, 9 Nov 2018 07:32:06 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.117]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.102]) with mapi id 14.03.0415.000; Fri, 9 Nov 2018 23:32:04 +0800 From: "Gao, Liming" To: Laszlo Ersek , Leif Lindholm CC: "Kinney, Michael D" , "edk2-devel@lists.01.org" Thread-Topic: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos Thread-Index: AQHUdqmiCNK2ekw/m0KKS894fU9KF6VEjAgAgAEhQ4CAAOarIP//6y6AgABBF4CAAAOFgIAAzDpA Date: Fri, 9 Nov 2018 15:32:03 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14E3682A7@SHSMSX104.ccr.corp.intel.com> References: <20181107145311.42488-1-dandan.bi@intel.com> <0C09AFA07DD0434D9E2A0C6AEB048310401EC2D7@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E367B56@SHSMSX104.ccr.corp.intel.com> <20181109104905.7vowaikh3ifvubdq@bivouac.eciton.net> <1af49731-99eb-fc56-94d1-e9ac2d8db1a7@redhat.com> In-Reply-To: <1af49731-99eb-fc56-94d1-e9ac2d8db1a7@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOWIzZmY0MjctOGVkYy00NDU4LTkzMWYtNzQxMDdjMTNmNzQ5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiNjVGTHBXeFFScFRyU1RSckw2TVZqTkpKTmFzUnRObUVQaG5tOE1LSU16Y0p0SGRja0dhNTJMalZ4T3hRWWdGViJ9 dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Nov 2018 15:32:08 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo and Leif: I suggest to continue to update wiki page with more information. If so, w= e can avoid such case again.=20 For this change, it has no real functionality impact. If you think roll b= ack is better than keep it, I am OK.=20 Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of La= szlo Ersek > Sent: Friday, November 9, 2018 7:02 PM > To: Leif Lindholm ; Gao, Liming > Cc: Kinney, Michael D ; edk2-devel@lists.01.o= rg > Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless NU= LL ptr check for NewPos >=20 > On 11/09/18 11:49, Leif Lindholm wrote: > > On Fri, Nov 09, 2018 at 07:56:07AM +0100, Ard Biesheuvel wrote: > >> On 9 November 2018 at 01:19, Gao, Liming wrote: > >>> Ard: > >>> This is a small fix. And, this patch is sent before the hard > >>> freeze. It is the low risk for this release. So, I push it. > >> > >> OK, fair enough. > > > > I don't agree actually. > > > > https://github.com/tianocore/tianocore.github.io/wiki/HardFeatureFreeze > > specifies clearly that only bug fixes are permitted in during hard > > freeze. Maybe we could document that a bit more explicitly, but this > > patch was no bugfix. It should not have gone in. > > > > By my interpretation, it would not even fulfill the requirements for > > https://github.com/lersek/edk2/wiki/SoftFeatureFreeze: > > "By the date of the soft feature freeze, developers must have sent > > their patches to the mailing list and received positive maintainer > > reviews." > > Soft feature freeze was 1 November. The patch was sent out 7 November. > > It received reviews 8 November (after the start of the hard freeze). > > > > The point of these freezes is that sometimes patches are wrong. And > > sometimes patches that look correct, are not correct. If we start > > making exceptions because "oh, it's trivial", that means we get these > > patches into the tree with much reduced time for anyone to catch any > > adverse effects before we make the stable tag. And at that point, the > > stable tag no longer has value. > > > > (I am much more flexible on the topic of updating documentation, like > > Maintainers.txt, but even there we must be very careful.) >=20 > I haven't been following this specific patch, but now it does not look > like a bugfix to me. Without applying the patch, there is no bug > actually, functional or performance. The subject says, "Remove useless ..= .". >=20 > Optimizations, simplifications, refactorings, features, and so on, are > not bugfixes. They should not go in after the hard freeze. Even after > the soft freeze, they should only go in if the only remaining step is > the push (i.e. they should be ready for pushing before the soft freeze, > sufficiently reviewed.). >=20 > Thanks > Laszlo >=20 > >>>> -----Original Message----- > >>>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > >>>> Sent: Friday, November 09, 2018 2:25 AM > >>>> To: Zeng, Star > >>>> Cc: Bi, Dandan ; edk2-devel@lists.01.org; Wu, H= ao A > >>>> ; Dong, Eric ; Gao, Liming > >>>> > >>>> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove usele= ss > >>>> NULL ptr check for NewPos > >>>> > >>>> On 8 November 2018 at 02:09, Zeng, Star wrote: > >>>>> Reviewed-by: Star Zeng > >>>>> > >>>>> -----Original Message----- > >>>>> From: Bi, Dandan > >>>>> Sent: Wednesday, November 7, 2018 10:53 PM > >>>>> To: edk2-devel@lists.01.org > >>>>> Cc: Gao, Liming ; Dong, Eric ; > >>>> Zeng, Star ; Wu, Hao A > >>>>> Subject: [patch] MdeModulePkg/DisplayEngine: Remove useless NULL pt= r > >>>> check for NewPos > >>>>> > >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1306 > >>>>> > >>>>> In function UiDisplayMenu, the NewPos ptr which used to point to th= e > >>>> highlight menu entry. It will always point to the menu entry which n= eed to be > >>>> highlighted or the gMenuOption menu if the highlight menu is not fou= nd. > >>>>> So we can remove the NULL ptr check for NewPos in this function. > >>>>> And add the ASSERT code to avoid if any false positive reports of N= ULL > >>>> pointer dereference issue raised from static analysis. > >>>>> > >>>>> Cc: Liming Gao > >>>>> Cc: Eric Dong > >>>>> Cc: Star Zeng > >>>>> Cc: Hao Wu > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>> Signed-off-by: Dandan Bi > >>>> > >>>> Why was this patch merged today? Surely, this doesn't meet the hard > >>>> freeze requirements ? > >>>> > >>>>> --- > >>>>> MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c | 3 ++- > >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c > >>>> b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c > >>>>> index 7390f954b6..44f087fe01 100644 > >>>>> --- a/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c > >>>>> +++ b/MdeModulePkg/Universal/DisplayEngineDxe/FormDisplay.c > >>>>> @@ -2880,10 +2880,11 @@ UiDisplayMenu ( > >>>>> // MenuOption is set to NULL in Repaint > >>>>> // NewPos: Current menu option that need to hilight > >>>>> // > >>>>> ControlFlag =3D CfUpdateHelpString; > >>>>> > >>>>> + ASSERT (NewPos !=3D NULL); > >>>>> UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue); > >>>>> > >>>>> if (SkipHighLight) { > >>>>> SkipHighLight =3D FALSE; > >>>>> MenuOption =3D SavedMenuOption; > >>>>> @@ -2908,11 +2909,11 @@ UiDisplayMenu ( > >>>>> Temp2 =3D SkipValue; > >>>>> } else { > >>>>> Temp2 =3D 0; > >>>>> } > >>>>> > >>>>> - if (NewPos !=3D NULL && (MenuOption =3D=3D NULL || NewPos != =3D > >>>> &MenuOption->Link)) { > >>>>> + if (MenuOption =3D=3D NULL || NewPos !=3D &MenuOption->Link)= { > >>>>> if (MenuOption !=3D NULL) { > >>>>> // > >>>>> // Remove the old highlight menu. > >>>>> // > >>>>> Status =3D DisplayOneMenu (MenuOption, > >>>>> -- > >>>>> 2.18.0.windows.1 > >>>>> > >>>>> _______________________________________________ > >>>>> edk2-devel mailing list > >>>>> edk2-devel@lists.01.org > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel