From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 87AC22118DC51 for ; Fri, 9 Nov 2018 03:01:44 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C44A230001DC; Fri, 9 Nov 2018 11:01:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-96.rdu2.redhat.com [10.10.120.96]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1956819C65; Fri, 9 Nov 2018 11:01:41 +0000 (UTC) To: Leif Lindholm , "Gao, Liming" Cc: Ard Biesheuvel , "edk2-devel@lists.01.org" , Andrew Fish , Michael D Kinney 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> From: Laszlo Ersek Message-ID: <1af49731-99eb-fc56-94d1-e9ac2d8db1a7@redhat.com> Date: Fri, 9 Nov 2018 12:01:41 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181109104905.7vowaikh3ifvubdq@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Fri, 09 Nov 2018 11:01:44 +0000 (UTC) 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 11:01:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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.) 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 ...". 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.). Thanks Laszlo >>>> -----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, Hao A >>>> ; Dong, Eric ; Gao, Liming >>>> >>>> Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless >>>> 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 ptr >>>> check for NewPos >>>>> >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1306 >>>>> >>>>> In function UiDisplayMenu, the NewPos ptr which used to point to the >>>> highlight menu entry. It will always point to the menu entry which need to be >>>> highlighted or the gMenuOption menu if the highlight menu is not found. >>>>> 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 NULL >>>> 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 = CfUpdateHelpString; >>>>> >>>>> + ASSERT (NewPos != NULL); >>>>> UpdateHighlightMenuInfo(NewPos, TopOfScreen, SkipValue); >>>>> >>>>> if (SkipHighLight) { >>>>> SkipHighLight = FALSE; >>>>> MenuOption = SavedMenuOption; >>>>> @@ -2908,11 +2909,11 @@ UiDisplayMenu ( >>>>> Temp2 = SkipValue; >>>>> } else { >>>>> Temp2 = 0; >>>>> } >>>>> >>>>> - if (NewPos != NULL && (MenuOption == NULL || NewPos != >>>> &MenuOption->Link)) { >>>>> + if (MenuOption == NULL || NewPos != &MenuOption->Link) { >>>>> if (MenuOption != NULL) { >>>>> // >>>>> // Remove the old highlight menu. >>>>> // >>>>> Status = 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