On Fri, Nov 09, 2018 at 07:56:07AM +0100, Ard Biesheuvel wrote: > On 9 November 2018 at 01:19, Gao, Liming <[email protected]> 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.) Regards, Leif > >>-----Original Message----- > >>From: Ard Biesheuvel [mailto:[email protected]] > >>Sent: Friday, November 09, 2018 2:25 AM > >>To: Zeng, Star <[email protected]> > >>Cc: Bi, Dandan <[email protected]>; [email protected]; Wu, Hao A > >><[email protected]>; Dong, Eric <[email protected]>; Gao, Liming > >><[email protected]> > >>Subject: Re: [edk2] [patch] MdeModulePkg/DisplayEngine: Remove useless > >>NULL ptr check for NewPos > >> > >>On 8 November 2018 at 02:09, Zeng, Star <[email protected]> wrote: > >>> Reviewed-by: Star Zeng <[email protected]> > >>> > >>> -----Original Message----- > >>> From: Bi, Dandan > >>> Sent: Wednesday, November 7, 2018 10:53 PM > >>> To: [email protected] > >>> Cc: Gao, Liming <[email protected]>; Dong, Eric <[email protected]>; > >>Zeng, Star <[email protected]>; Wu, Hao A <[email protected]> > >>> 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 <[email protected]> > >>> Cc: Eric Dong <[email protected]> > >>> Cc: Star Zeng <[email protected]> > >>> Cc: Hao Wu <[email protected]> > >>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>> Signed-off-by: Dandan Bi <[email protected]> > >> > >>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 > >>> [email protected] > >>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
