Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic
Reviewed-by: Eric Dong > -Original Message- > From: Bi, Dandan > Sent: Monday, October 17, 2016 5:08 PM > To: Laszlo Ersek; edk2-de...@ml01.01.org > Cc: Dong, Eric; Gao, Liming > Subject: RE: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the > codes logic > > Hi Laszlo, > > Thank you very much for your comments! > I have split this patch into 5 independent patches with following subject : > [patch 1/5] MdeModulePkg/BMMUI: ... > [patch 2/5] MdeModulePkg/BMMUI: ... > [patch 3/5] MdeModulePkg/BMMUI: ... > [patch 4/5] MdeModulePkg/BMMUI: ... > [patch 5/5] MdeModulePkg/BMMUI: ... > > Hi Liming/Eric > Please review the new patches and ignore this one. Sorry for any > inconvenience. > > > Regards, > Dandan > > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, October 14, 2016 8:34 PM > To: Bi, Dandan ; edk2-de...@ml01.01.org > Cc: Dong, Eric ; Gao, Liming > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the > codes logic > > On 10/14/16 08:43, Dandan Bi wrote: > > This patch is mainly to: > > 1. Enhance the error handling codes when set variable fail. > > 2. Enhance the logic to fix some incorrect UI behaviors. > > My apologies, but both the subject line and the commit message are mostly > impenetrable. > > This patch should be split up into a series of two patches, minimally > (according to the two goals above that it implements), and each > change should be described correctly in both the subject line and in the > commit message. > > If I got a bug report for OVMF that I managed to bisect back to this patch, > I'd be *completely* helpless figuring out what it does. > > What kind of variables are set by the code? What happens now if setting those > variables fails? What is the expected behavior instead that > the > (first) patch implements? > > What are those incorrect UI behaviors? When do they happen? What does the > second patch do to address those issues? > > Dear Developers, please *stop* writing subject lines like > > "Enhance the code in DNS driver" > "Enhance the codes logic" > > those subject lines are *completely* useless. You could replace all those > subject lines, without any loss of information, with the following > one: > > "Do Work" > > Please spend time thinking about the granularity, the focus of your patches, > as a *standalone activity* during development. Ask yourselves, > "Is this patch small enough? Am I doing two or more independent things here? > Is the subject line clear enough? If a person sees the code > for the first time, will my commit message help them?" > > You don't write the commit message for yourselves only, you write it for > other developers who might have absolutely no clue what's going > on in your module. > > Thanks > Laszlo > > > V2: Update the Console/Terminal menu when the related question changed. > > > > Cc: Liming Gao > > Cc: Eric Dong > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Dandan Bi > > --- > > .../BootMaintenanceManagerUiLib/BootMaintenance.c | 390 > > - > > .../BootMaintenanceManagerUiLib/UpdatePage.c | 42 ++- > > .../Library/BootMaintenanceManagerUiLib/Variable.c | 28 +- > > 3 files changed, 357 insertions(+), 103 deletions(-) > > > > diff --git > > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > > b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > > index a190596..924eb49 100644 > > --- > > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > > +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance > > +++ .c > > @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle ( > >return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle), > > FALSE, FALSE); > > > > } > > > > /** > > + Converts the unicode character of the string from uppercase to lowercase. > > + This is a internal function. > > + > > + @param ConfigString String to be converted > > + > > +**/ > > +VOID > > +HiiToLower ( > > + IN EFI_STRING ConfigString > > + ) > > +{ > > + EFI_STRING String; > > + BOOLEAN Lower; > > + > > + ASSERT (ConfigString != NULL); > > + > > + // > > + // Convert all hex digits in range [A-F] in the configuration > > +header to [a-f] > > + // > > + for (String = Co
Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic
On 10/17/16 11:07, Bi, Dandan wrote: > Hi Laszlo, > > Thank you very much for your comments! > I have split this patch into 5 independent patches with following subject : > [patch 1/5] MdeModulePkg/BMMUI: ... > [patch 2/5] MdeModulePkg/BMMUI: ... > [patch 3/5] MdeModulePkg/BMMUI: ... > [patch 4/5] MdeModulePkg/BMMUI: ... > [patch 5/5] MdeModulePkg/BMMUI: ... Thank you very much -- the new subjects look much-much better. Also, I think it's a nice trick to shorten "Boot Maintenance Manager UI" as "BMMUI", it's understandable and it saves quite a few characters in the subjects. Thanks! Laszlo > Hi Liming/Eric > Please review the new patches and ignore this one. Sorry for any > inconvenience. > > > Regards, > Dandan > > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Friday, October 14, 2016 8:34 PM > To: Bi, Dandan ; edk2-de...@ml01.01.org > Cc: Dong, Eric ; Gao, Liming > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the > codes logic > > On 10/14/16 08:43, Dandan Bi wrote: >> This patch is mainly to: >> 1. Enhance the error handling codes when set variable fail. >> 2. Enhance the logic to fix some incorrect UI behaviors. > > My apologies, but both the subject line and the commit message are mostly > impenetrable. > > This patch should be split up into a series of two patches, minimally > (according to the two goals above that it implements), and each change should > be described correctly in both the subject line and in the commit message. > > If I got a bug report for OVMF that I managed to bisect back to this patch, > I'd be *completely* helpless figuring out what it does. > > What kind of variables are set by the code? What happens now if setting those > variables fails? What is the expected behavior instead that the > (first) patch implements? > > What are those incorrect UI behaviors? When do they happen? What does the > second patch do to address those issues? > > Dear Developers, please *stop* writing subject lines like > > "Enhance the code in DNS driver" > "Enhance the codes logic" > > those subject lines are *completely* useless. You could replace all those > subject lines, without any loss of information, with the following > one: > > "Do Work" > > Please spend time thinking about the granularity, the focus of your patches, > as a *standalone activity* during development. Ask yourselves, "Is this patch > small enough? Am I doing two or more independent things here? Is the subject > line clear enough? If a person sees the code for the first time, will my > commit message help them?" > > You don't write the commit message for yourselves only, you write it for > other developers who might have absolutely no clue what's going on in your > module. > > Thanks > Laszlo > >> V2: Update the Console/Terminal menu when the related question changed. >> >> Cc: Liming Gao >> Cc: Eric Dong >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Dandan Bi >> --- >> .../BootMaintenanceManagerUiLib/BootMaintenance.c | 390 >> - >> .../BootMaintenanceManagerUiLib/UpdatePage.c | 42 ++- >> .../Library/BootMaintenanceManagerUiLib/Variable.c | 28 +- >> 3 files changed, 357 insertions(+), 103 deletions(-) >> >> diff --git >> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c >> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c >> index a190596..924eb49 100644 >> --- >> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c >> +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance >> +++ .c >> @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle ( >>return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle), >> FALSE, FALSE); >> >> } >> >> /** >> + Converts the unicode character of the string from uppercase to lowercase. >> + This is a internal function. >> + >> + @param ConfigString String to be converted >> + >> +**/ >> +VOID >> +HiiToLower ( >> + IN EFI_STRING ConfigString >> + ) >> +{ >> + EFI_STRING String; >> + BOOLEAN Lower; >> + >> + ASSERT (ConfigString != NULL); >> + >> + // >> + // Convert all hex digits in range [A-F] in the configuration >> +header to [a-f] >> + // >> + for (String = ConfigString, Lower = FALSE; *String != L'\0'; St
Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic
Hi Laszlo, Thank you very much for your comments! I have split this patch into 5 independent patches with following subject : [patch 1/5] MdeModulePkg/BMMUI: ... [patch 2/5] MdeModulePkg/BMMUI: ... [patch 3/5] MdeModulePkg/BMMUI: ... [patch 4/5] MdeModulePkg/BMMUI: ... [patch 5/5] MdeModulePkg/BMMUI: ... Hi Liming/Eric Please review the new patches and ignore this one. Sorry for any inconvenience. Regards, Dandan -Original Message- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Friday, October 14, 2016 8:34 PM To: Bi, Dandan ; edk2-de...@ml01.01.org Cc: Dong, Eric ; Gao, Liming Subject: Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic On 10/14/16 08:43, Dandan Bi wrote: > This patch is mainly to: > 1. Enhance the error handling codes when set variable fail. > 2. Enhance the logic to fix some incorrect UI behaviors. My apologies, but both the subject line and the commit message are mostly impenetrable. This patch should be split up into a series of two patches, minimally (according to the two goals above that it implements), and each change should be described correctly in both the subject line and in the commit message. If I got a bug report for OVMF that I managed to bisect back to this patch, I'd be *completely* helpless figuring out what it does. What kind of variables are set by the code? What happens now if setting those variables fails? What is the expected behavior instead that the (first) patch implements? What are those incorrect UI behaviors? When do they happen? What does the second patch do to address those issues? Dear Developers, please *stop* writing subject lines like "Enhance the code in DNS driver" "Enhance the codes logic" those subject lines are *completely* useless. You could replace all those subject lines, without any loss of information, with the following one: "Do Work" Please spend time thinking about the granularity, the focus of your patches, as a *standalone activity* during development. Ask yourselves, "Is this patch small enough? Am I doing two or more independent things here? Is the subject line clear enough? If a person sees the code for the first time, will my commit message help them?" You don't write the commit message for yourselves only, you write it for other developers who might have absolutely no clue what's going on in your module. Thanks Laszlo > V2: Update the Console/Terminal menu when the related question changed. > > Cc: Liming Gao > Cc: Eric Dong > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Dandan Bi > --- > .../BootMaintenanceManagerUiLib/BootMaintenance.c | 390 > - > .../BootMaintenanceManagerUiLib/UpdatePage.c | 42 ++- > .../Library/BootMaintenanceManagerUiLib/Variable.c | 28 +- > 3 files changed, 357 insertions(+), 103 deletions(-) > > diff --git > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > index a190596..924eb49 100644 > --- > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance > +++ .c > @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle ( >return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle), > FALSE, FALSE); > > } > > /** > + Converts the unicode character of the string from uppercase to lowercase. > + This is a internal function. > + > + @param ConfigString String to be converted > + > +**/ > +VOID > +HiiToLower ( > + IN EFI_STRING ConfigString > + ) > +{ > + EFI_STRING String; > + BOOLEAN Lower; > + > + ASSERT (ConfigString != NULL); > + > + // > + // Convert all hex digits in range [A-F] in the configuration > +header to [a-f] > + // > + for (String = ConfigString, Lower = FALSE; *String != L'\0'; String++) { > +if (*String == L'=') { > + Lower = TRUE; > +} else if (*String == L'&') { > + Lower = FALSE; > +} else if (Lower && *String >= L'A' && *String <= L'F') { > + *String = (CHAR16) (*String - L'A' + L'a'); > +} > + } > +} > + > +/** > + Update the progress string through the offset value. > + > + @param Offset The offset value > + @param ConfigurationPoint to the configuration string. > + > +**/ > +EFI_STRING > +UpdateProgress( > + IN UINTN Offset, > + IN EFI_STRING Configuration > +) > +{ > + UINTN Length; > + EFI_STRING StringPtr; > + EFI_STRING ReturnString; > + > + StringPtr= NULL; &g
Re: [edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic
On 10/14/16 08:43, Dandan Bi wrote: > This patch is mainly to: > 1. Enhance the error handling codes when set variable fail. > 2. Enhance the logic to fix some incorrect UI behaviors. My apologies, but both the subject line and the commit message are mostly impenetrable. This patch should be split up into a series of two patches, minimally (according to the two goals above that it implements), and each change should be described correctly in both the subject line and in the commit message. If I got a bug report for OVMF that I managed to bisect back to this patch, I'd be *completely* helpless figuring out what it does. What kind of variables are set by the code? What happens now if setting those variables fails? What is the expected behavior instead that the (first) patch implements? What are those incorrect UI behaviors? When do they happen? What does the second patch do to address those issues? Dear Developers, please *stop* writing subject lines like "Enhance the code in DNS driver" "Enhance the codes logic" those subject lines are *completely* useless. You could replace all those subject lines, without any loss of information, with the following one: "Do Work" Please spend time thinking about the granularity, the focus of your patches, as a *standalone activity* during development. Ask yourselves, "Is this patch small enough? Am I doing two or more independent things here? Is the subject line clear enough? If a person sees the code for the first time, will my commit message help them?" You don't write the commit message for yourselves only, you write it for other developers who might have absolutely no clue what's going on in your module. Thanks Laszlo > V2: Update the Console/Terminal menu when the related question changed. > > Cc: Liming Gao > Cc: Eric Dong > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Dandan Bi > --- > .../BootMaintenanceManagerUiLib/BootMaintenance.c | 390 > - > .../BootMaintenanceManagerUiLib/UpdatePage.c | 42 ++- > .../Library/BootMaintenanceManagerUiLib/Variable.c | 28 +- > 3 files changed, 357 insertions(+), 103 deletions(-) > > diff --git > a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > index a190596..924eb49 100644 > --- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c > @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle ( >return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle), FALSE, > FALSE); > > } > > /** > + Converts the unicode character of the string from uppercase to lowercase. > + This is a internal function. > + > + @param ConfigString String to be converted > + > +**/ > +VOID > +HiiToLower ( > + IN EFI_STRING ConfigString > + ) > +{ > + EFI_STRING String; > + BOOLEAN Lower; > + > + ASSERT (ConfigString != NULL); > + > + // > + // Convert all hex digits in range [A-F] in the configuration header to > [a-f] > + // > + for (String = ConfigString, Lower = FALSE; *String != L'\0'; String++) { > +if (*String == L'=') { > + Lower = TRUE; > +} else if (*String == L'&') { > + Lower = FALSE; > +} else if (Lower && *String >= L'A' && *String <= L'F') { > + *String = (CHAR16) (*String - L'A' + L'a'); > +} > + } > +} > + > +/** > + Update the progress string through the offset value. > + > + @param Offset The offset value > + @param ConfigurationPoint to the configuration string. > + > +**/ > +EFI_STRING > +UpdateProgress( > + IN UINTN Offset, > + IN EFI_STRING Configuration > +) > +{ > + UINTN Length; > + EFI_STRING StringPtr; > + EFI_STRING ReturnString; > + > + StringPtr= NULL; > + ReturnString = NULL; > + > + // > + // &OFFSET= followed by a Null-terminator. > + // Length = StrLen (L"&OFFSET=") + 4 + 1 > + // > + Length= StrLen (L"&OFFSET=") + 4 + 1; > + > + StringPtr = AllocateZeroPool (Length * sizeof (CHAR16)); > + > + if (StringPtr == NULL) { > +return NULL; > + } > + > + UnicodeSPrint ( > +StringPtr, > +(8 + 4 + 1) * sizeof (CHAR16), > +L"&OFFSET=%04x", > +Offset > +); > + > + ReturnString = StrStr (Configuration, StringPtr); > + > + if (ReturnString == NULL) { > +// > +// If doesn't find the string in Configuration, convert the string to > lower case then search again. > +// > +HiiToLower (StringPtr); > +ReturnString = StrStr (Configuration, StringPtr); > + } > + > + FreePool (StringPtr); > + > + return ReturnString; > +} > + > +/** > + Update the terminal content in TerminalMenu. > + > + @param BmmData The BMM fake NV data. > + > +**/ > +VOID > +UpdateTerminalContent ( > + IN BMM_FAKE_NV_DATA *BmmData > + ) > +{ > + UINT16 Index; > + BM_TERMINAL_CONTEXT
[edk2] [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic
This patch is mainly to: 1. Enhance the error handling codes when set variable fail. 2. Enhance the logic to fix some incorrect UI behaviors. V2: Update the Console/Terminal menu when the related question changed. Cc: Liming Gao Cc: Eric Dong Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Dandan Bi --- .../BootMaintenanceManagerUiLib/BootMaintenance.c | 390 - .../BootMaintenanceManagerUiLib/UpdatePage.c | 42 ++- .../Library/BootMaintenanceManagerUiLib/Variable.c | 28 +- 3 files changed, 357 insertions(+), 103 deletions(-) diff --git a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c index a190596..924eb49 100644 --- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenance.c @@ -442,10 +442,197 @@ BmmExtractDevicePathFromHiiHandle ( return ConvertDevicePathToText(DevicePathFromHandle (DriverHandle), FALSE, FALSE); } /** + Converts the unicode character of the string from uppercase to lowercase. + This is a internal function. + + @param ConfigString String to be converted + +**/ +VOID +HiiToLower ( + IN EFI_STRING ConfigString + ) +{ + EFI_STRING String; + BOOLEAN Lower; + + ASSERT (ConfigString != NULL); + + // + // Convert all hex digits in range [A-F] in the configuration header to [a-f] + // + for (String = ConfigString, Lower = FALSE; *String != L'\0'; String++) { +if (*String == L'=') { + Lower = TRUE; +} else if (*String == L'&') { + Lower = FALSE; +} else if (Lower && *String >= L'A' && *String <= L'F') { + *String = (CHAR16) (*String - L'A' + L'a'); +} + } +} + +/** + Update the progress string through the offset value. + + @param Offset The offset value + @param ConfigurationPoint to the configuration string. + +**/ +EFI_STRING +UpdateProgress( + IN UINTN Offset, + IN EFI_STRING Configuration +) +{ + UINTN Length; + EFI_STRING StringPtr; + EFI_STRING ReturnString; + + StringPtr= NULL; + ReturnString = NULL; + + // + // &OFFSET= followed by a Null-terminator. + // Length = StrLen (L"&OFFSET=") + 4 + 1 + // + Length= StrLen (L"&OFFSET=") + 4 + 1; + + StringPtr = AllocateZeroPool (Length * sizeof (CHAR16)); + + if (StringPtr == NULL) { +return NULL; + } + + UnicodeSPrint ( +StringPtr, +(8 + 4 + 1) * sizeof (CHAR16), +L"&OFFSET=%04x", +Offset +); + + ReturnString = StrStr (Configuration, StringPtr); + + if (ReturnString == NULL) { +// +// If doesn't find the string in Configuration, convert the string to lower case then search again. +// +HiiToLower (StringPtr); +ReturnString = StrStr (Configuration, StringPtr); + } + + FreePool (StringPtr); + + return ReturnString; +} + +/** + Update the terminal content in TerminalMenu. + + @param BmmData The BMM fake NV data. + +**/ +VOID +UpdateTerminalContent ( + IN BMM_FAKE_NV_DATA *BmmData + ) +{ + UINT16 Index; + BM_TERMINAL_CONTEXT *NewTerminalContext; + BM_MENU_ENTRY *NewMenuEntry; + + for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) { +NewMenuEntry = BOpt_GetMenuEntry (&TerminalMenu, Index); +ASSERT (NewMenuEntry != NULL); +NewTerminalContext = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext; +NewTerminalContext->BaudRateIndex = BmmData->COMBaudRate[Index]; +ASSERT (BmmData->COMBaudRate[Index] < (sizeof (BaudRateList) / sizeof (BaudRateList[0]))); +NewTerminalContext->BaudRate = BaudRateList[BmmData->COMBaudRate[Index]].Value; +NewTerminalContext->DataBitsIndex = BmmData->COMDataRate[Index]; +ASSERT (BmmData->COMDataRate[Index] < (sizeof (DataBitsList) / sizeof (DataBitsList[0]))); +NewTerminalContext->DataBits = (UINT8) DataBitsList[BmmData->COMDataRate[Index]].Value; +NewTerminalContext->StopBitsIndex = BmmData->COMStopBits[Index]; +ASSERT (BmmData->COMStopBits[Index] < (sizeof (StopBitsList) / sizeof (StopBitsList[0]))); +NewTerminalContext->StopBits = (UINT8) StopBitsList[BmmData->COMStopBits[Index]].Value; +NewTerminalContext->ParityIndex = BmmData->COMParity[Index]; +ASSERT (BmmData->COMParity[Index] < (sizeof (ParityList) / sizeof (ParityList[0]))); +NewTerminalContext->Parity= (UINT8) ParityList[BmmData->COMParity[Index]].Value; +NewTerminalContext->TerminalType = BmmData->COMTerminalType[Index]; +NewTerminalContext->FlowControl = BmmData->COMFlowControl[Index]; +ChangeTerminalDevicePath ( + NewTerminalContext->DevicePath, + FALSE + ); + } +} + +/** + Update the console content in ConsoleMenu. + + @param BmmData The BMM fake NV data. + +**/ +VOID +UpdateConsoleContent( + IN CHAR16 *ConsoleName, + IN BMM_FAK