From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::441; helo=mail-wr1-x441.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id AF18B2116527B for ; Fri, 9 Nov 2018 08:01:40 -0800 (PST) Received: by mail-wr1-x441.google.com with SMTP id e3-v6so2435813wrs.5 for ; Fri, 09 Nov 2018 08:01:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=lDObcBE5WW5PTg4PmC8DkeG7+uEG1vSn4DFdMHkOnAU=; b=g9EiEO032Gbqck/XLQwym94wShvT2WFK2Qwey9YxybkM4oULIBRAK6NwO6fXqbdjSs mCfq3/CFEMRVkdJ/ukLUgs/nYsN1xdDiILBhnJ1/grbxFv7mTbYHIN2/atayKTodq0W1 OqKBt9HrqrKAS1b+hroPT6FgpyLMeyX/eR6h0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=lDObcBE5WW5PTg4PmC8DkeG7+uEG1vSn4DFdMHkOnAU=; b=WQ6ogaygdNVuil9IXcUBDqoZ2pEpR5JK9UQEOj+kKqWMaN+ajxn+RcLEiDQ3EJhpGo UI7ToulpO2eXMmRDbCbkmA5WT4PzsDbTuAQVqRsadR4FkU0AJ+fdkbM/xurc4FNAwwvi t8QjRbSBxbLzzQU2J78yazrvlKNUlT2cdvBb30P7DMVkv3bd4H4Wm+LACatk2RbCtbWE NTUE3lPujZR5sabE4idELAglQa2Kbxeh6zZCAe8vawbfE4Pmfx0QqW8Fr5m/pVdOQgs7 f7nqzGjh3IwqG37h4QE2nHFwy6fUdXH10wIDKMBzeBo2OifEFHJRklKbuHTcC7suQDCc 6Nbw== X-Gm-Message-State: AGRZ1gKq/5gXXuRlnXAFbCZJqz/NFs0yJQ//B9+ywNzEBWDpcYuDNjZL f5pTdInz80qTXHsC87C+Xol6lQ== X-Google-Smtp-Source: AJdET5dm5RKZkRhaYAiyDREiRKhJDun19PqxpL//qAC1+YWwSC6IoaCr1ywwkXjcC/XvXHWTWj9U/w== X-Received: by 2002:a5d:40cc:: with SMTP id b12-v6mr9092663wrq.133.1541779299352; Fri, 09 Nov 2018 08:01:39 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id h9-v6sm9951886wrw.90.2018.11.09.08.01.37 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 09 Nov 2018 08:01:37 -0800 (PST) Date: Fri, 9 Nov 2018 16:01:35 +0000 From: Leif Lindholm To: "Gao, Liming" Cc: Laszlo Ersek , "Kinney, Michael D" , "edk2-devel@lists.01.org" Message-ID: <20181109160135.qvx5aznwr5mx2ten@bivouac.eciton.net> 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> <4A89E2EF3DFEDB4C8BFDE51014F606A14E3682A7@SHSMSX104.ccr.corp.intel.com> MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E3682A7@SHSMSX104.ccr.corp.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) 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 16:01:42 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Nov 09, 2018 at 03:32:03PM +0000, Gao, Liming wrote: > Laszlo and Leif: > I suggest to continue to update wiki page with more > information. If so, we can avoid such case again. Agreed, we need to be able to interpret what the process says identically. > For this change, it has no real functionality impact. But this is my point: we should not be making judgement calls this late in the process. If I look at that patch, sure, it looks fine to me. I still don't want it going in during freeze, because I don't _know_ it has no real functionality impact. I may be missing something. And even if I am not missing anything, the reshuffle may still be sufficient to change compiler behaviour, exposing a previously undetected bug. > If you think roll back is better than keep it, I am OK. That would be my preference. I have zero objection to it going back in immediately after the stable tag is made. Regards, Leif > Thanks > Liming > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek > > Sent: Friday, November 9, 2018 7:02 PM > > To: Leif Lindholm ; Gao, Liming > > Cc: Kinney, Michael D ; edk2-devel@lists.01.org > > Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless NULL ptr check for NewPos > > > > 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 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel