Re: [PATCH v2] MdeModulePkg/BootMaintenanceUi: Enhance the codes logic

[Laszlo Ersek <lersek@redhat.com>]

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 <liming.gao@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  .../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 Configuration    Point 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=XXXX 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_FAKE_NV_DATA       *BmmData
> +  )
> +{
> +  UINT16                          Index;
> +  BM_CONSOLE_CONTEXT              *NewConsoleContext;
> +  BM_TERMINAL_CONTEXT             *NewTerminalContext;
> +  BM_MENU_ENTRY                   *NewMenuEntry;
> +
> +  if (StrCmp (ConsoleName, L"ConIn") == 0) {
> +    for (Index = 0; Index < ConsoleInpMenu.MenuNumber; Index++){
> +      NewMenuEntry                = BOpt_GetMenuEntry(&ConsoleInpMenu, Index);
> +      NewConsoleContext           = (BM_CONSOLE_CONTEXT *)NewMenuEntry->VariableContext;
> +      ASSERT (Index < MAX_MENU_NUMBER);
> +      NewConsoleContext->IsActive = BmmData->ConsoleInCheck[Index];
> +    }
> +    for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) {
> +      NewMenuEntry                = BOpt_GetMenuEntry (&TerminalMenu, Index);
> +      NewTerminalContext          = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext;
> +      ASSERT (Index + ConsoleInpMenu.MenuNumber < MAX_MENU_NUMBER);
> +      NewTerminalContext->IsConIn = BmmData->ConsoleInCheck[Index + ConsoleInpMenu.MenuNumber];
> +    }
> +  }
> +
> +  if (StrCmp (ConsoleName, L"ConOut") == 0) {
> +    for (Index = 0; Index < ConsoleOutMenu.MenuNumber; Index++){
> +      NewMenuEntry                = BOpt_GetMenuEntry(&ConsoleOutMenu, Index);
> +      NewConsoleContext           = (BM_CONSOLE_CONTEXT *)NewMenuEntry->VariableContext;
> +      ASSERT (Index < MAX_MENU_NUMBER);
> +      NewConsoleContext->IsActive = BmmData->ConsoleOutCheck[Index];
> +    }
> +    for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) {
> +      NewMenuEntry                = BOpt_GetMenuEntry (&TerminalMenu, Index);
> +      NewTerminalContext          = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext;
> +      ASSERT (Index + ConsoleOutMenu.MenuNumber < MAX_MENU_NUMBER);
> +      NewTerminalContext->IsConOut = BmmData->ConsoleOutCheck[Index + ConsoleOutMenu.MenuNumber];
> +    }
> +  }
> +  if (StrCmp (ConsoleName, L"ErrOut") == 0) {
> +    for (Index = 0; Index < ConsoleErrMenu.MenuNumber; Index++){
> +      NewMenuEntry                = BOpt_GetMenuEntry(&ConsoleErrMenu, Index);
> +      NewConsoleContext           = (BM_CONSOLE_CONTEXT *)NewMenuEntry->VariableContext;
> +      ASSERT (Index < MAX_MENU_NUMBER);
> +      NewConsoleContext->IsActive = BmmData->ConsoleErrCheck[Index];
> +    }
> +    for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) {
> +      NewMenuEntry                = BOpt_GetMenuEntry (&TerminalMenu, Index);
> +      NewTerminalContext          = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext;
> +      ASSERT (Index + ConsoleErrMenu.MenuNumber < MAX_MENU_NUMBER);
> +      NewTerminalContext->IsStdErr = BmmData->ConsoleErrCheck[Index + ConsoleErrMenu.MenuNumber];
> +    }
> +  }
> +}
> +
> +/**
>    This function allows a caller to extract the current configuration for one
>    or more named elements from the target driver.
>  
>    @param This            Points to the EFI_HII_CONFIG_ACCESS_PROTOCOL.
>    @param Request         A null-terminated Unicode string in <ConfigRequest> format.
> @@ -587,17 +774,16 @@ BootMaintRouteConfig (
>    EFI_STATUS                      Status;
>    UINTN                           BufferSize;
>    EFI_HII_CONFIG_ROUTING_PROTOCOL *ConfigRouting;
>    BMM_FAKE_NV_DATA                *NewBmmData;
>    BMM_FAKE_NV_DATA                *OldBmmData;
> -  BM_CONSOLE_CONTEXT              *NewConsoleContext;
> -  BM_TERMINAL_CONTEXT             *NewTerminalContext;
>    BM_MENU_ENTRY                   *NewMenuEntry;
>    BM_LOAD_CONTEXT                 *NewLoadContext;
>    UINT16                          Index;
>    BOOLEAN                         TerminalAttChange;
> -  BMM_CALLBACK_DATA               *Private; 
> +  BMM_CALLBACK_DATA               *Private;
> +  UINTN                           Offset;
>  
>    if (Progress == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
>    *Progress = Configuration;
> @@ -628,10 +814,11 @@ BootMaintRouteConfig (
>    // Get Buffer Storage data from EFI variable
>    //
>    BufferSize = sizeof (BMM_FAKE_NV_DATA);
>    OldBmmData = &Private->BmmOldFakeNVData;
>    NewBmmData = &Private->BmmFakeNvData;
> +  Offset     = 0;
>    //
>    // Convert <ConfigResp> to buffer data by helper function ConfigToBlock()
>    //
>    Status = ConfigRouting->ConfigToBlock (
>                              ConfigRouting,
> @@ -649,10 +836,14 @@ BootMaintRouteConfig (
>    //
>    // Check data which located in BMM main page and save the settings if need
>    //         
>    if (CompareMem (&NewBmmData->BootNext, &OldBmmData->BootNext, sizeof (NewBmmData->BootNext)) != 0) {
>      Status = Var_UpdateBootNext (Private);
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, BootNext);
> +      goto Exit;
> +    }
>    }
>  
>    //
>    // Check data which located in Boot Options Menu and save the settings if need
>    //      
> @@ -665,15 +856,23 @@ BootMaintRouteConfig (
>        NewLoadContext->Deleted = NewBmmData->BootOptionDel[Index];
>        NewBmmData->BootOptionDel[Index] = FALSE;
>        NewBmmData->BootOptionDelMark[Index] = FALSE;
>      }
>  
> -    Var_DelBootOption ();
> +    Status = Var_DelBootOption ();
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, BootOptionDel);
> +      goto Exit;
> +    }
>    }
>  
>    if (CompareMem (NewBmmData->BootOptionOrder, OldBmmData->BootOptionOrder, sizeof (NewBmmData->BootOptionOrder)) != 0) {
>      Status = Var_UpdateBootOrder (Private);
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, BootOptionOrder);
> +      goto Exit;
> +    }
>    }
>  
>    if (CompareMem (&NewBmmData->BootTimeOut, &OldBmmData->BootTimeOut, sizeof (NewBmmData->BootTimeOut)) != 0){
>      Status = gRT->SetVariable(
>                      L"Timeout",
> @@ -681,19 +880,12 @@ BootMaintRouteConfig (
>                      EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_NON_VOLATILE,
>                      sizeof(UINT16),
>                      &(NewBmmData->BootTimeOut)
>                      );
>      if (EFI_ERROR (Status)) {
> -      //
> -      // If set variable fail, and don't have the appropriate error status for RouteConfig fuction to return,
> -      // just return the EFI_NOT_FOUND.
> -      //
> -      if (Status == EFI_OUT_OF_RESOURCES) {
> -        return Status;
> -      } else {
> -        return EFI_NOT_FOUND;
> -      }
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, BootTimeOut);
> +      goto Exit;
>      }
>      Private->BmmOldFakeNVData.BootTimeOut = NewBmmData->BootTimeOut;
>    }
>  
>    //
> @@ -707,19 +899,31 @@ BootMaintRouteConfig (
>        NewLoadContext          = (BM_LOAD_CONTEXT *) NewMenuEntry->VariableContext;
>        NewLoadContext->Deleted = NewBmmData->DriverOptionDel[Index];
>        NewBmmData->DriverOptionDel[Index] = FALSE;
>        NewBmmData->DriverOptionDelMark[Index] = FALSE;
>      }
> -    Var_DelDriverOption ();  
> +    Status = Var_DelDriverOption ();
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, DriverOptionDel);
> +      goto Exit;
> +    }
>    }
>  
>    if (CompareMem (NewBmmData->DriverOptionOrder, OldBmmData->DriverOptionOrder, sizeof (NewBmmData->DriverOptionOrder)) != 0) {  
>      Status = Var_UpdateDriverOrder (Private);
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, DriverOptionOrder);
> +      goto Exit;
> +    }
>    }
>  
>    if (CompareMem (&NewBmmData->ConsoleOutMode, &OldBmmData->ConsoleOutMode, sizeof (NewBmmData->ConsoleOutMode)) != 0){
> -    Var_UpdateConMode(Private);
> +    Status = Var_UpdateConMode(Private);
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, ConsoleOutMode);
> +      goto Exit;
> +    }
>    }
>  
>    TerminalAttChange = FALSE;
>    for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) {
>  
> @@ -733,95 +937,81 @@ BootMaintRouteConfig (
>           CompareMem (&NewBmmData->COMTerminalType[Index], &OldBmmData->COMTerminalType[Index], sizeof (NewBmmData->COMTerminalType[Index])) == 0 &&
>           CompareMem (&NewBmmData->COMFlowControl[Index], &OldBmmData->COMFlowControl[Index], sizeof (NewBmmData->COMFlowControl[Index])) == 0) {
>        continue;
>      }
>  
> -    NewMenuEntry = BOpt_GetMenuEntry (&TerminalMenu, Index);
> -    ASSERT (NewMenuEntry != NULL);
> -    NewTerminalContext = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext;
> -    NewTerminalContext->BaudRateIndex = NewBmmData->COMBaudRate[Index];
> -    ASSERT (NewBmmData->COMBaudRate[Index] < (sizeof (BaudRateList) / sizeof (BaudRateList[0])));
> -    NewTerminalContext->BaudRate      = BaudRateList[NewBmmData->COMBaudRate[Index]].Value;
> -    NewTerminalContext->DataBitsIndex = NewBmmData->COMDataRate[Index];
> -    ASSERT (NewBmmData->COMDataRate[Index] < (sizeof (DataBitsList) / sizeof (DataBitsList[0])));
> -    NewTerminalContext->DataBits      = (UINT8) DataBitsList[NewBmmData->COMDataRate[Index]].Value;
> -    NewTerminalContext->StopBitsIndex = NewBmmData->COMStopBits[Index];
> -    ASSERT (NewBmmData->COMStopBits[Index] < (sizeof (StopBitsList) / sizeof (StopBitsList[0])));
> -    NewTerminalContext->StopBits      = (UINT8) StopBitsList[NewBmmData->COMStopBits[Index]].Value;
> -    NewTerminalContext->ParityIndex   = NewBmmData->COMParity[Index];
> -    ASSERT (NewBmmData->COMParity[Index] < (sizeof (ParityList) / sizeof (ParityList[0])));
> -    NewTerminalContext->Parity        = (UINT8) ParityList[NewBmmData->COMParity[Index]].Value;
> -    NewTerminalContext->TerminalType  = NewBmmData->COMTerminalType[Index];
> -    NewTerminalContext->FlowControl   = NewBmmData->COMFlowControl[Index];
> -    ChangeTerminalDevicePath (
> -      NewTerminalContext->DevicePath,
> -      FALSE
> -      );
>      TerminalAttChange = TRUE;
>    }
>    if (TerminalAttChange) {
> -    Var_UpdateConsoleInpOption ();
> -    Var_UpdateConsoleOutOption ();
> -    Var_UpdateErrorOutOption ();
> +    if (CompareMem (&NewBmmData->COMBaudRate[Index], &OldBmmData->COMBaudRate[Index], sizeof (NewBmmData->COMBaudRate[Index])) != 0) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, COMBaudRate);
> +    } else if (CompareMem (&NewBmmData->COMDataRate[Index], &OldBmmData->COMDataRate[Index], sizeof (NewBmmData->COMDataRate[Index])) != 0) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, COMDataRate);
> +    } else if (CompareMem (&NewBmmData->COMStopBits[Index], &OldBmmData->COMStopBits[Index], sizeof (NewBmmData->COMStopBits[Index])) != 0) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, COMStopBits);
> +    } else if (CompareMem (&NewBmmData->COMParity[Index], &OldBmmData->COMParity[Index], sizeof (NewBmmData->COMParity[Index])) != 0) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, COMParity);
> +    } else if (CompareMem (&NewBmmData->COMTerminalType[Index], &OldBmmData->COMTerminalType[Index], sizeof (NewBmmData->COMTerminalType[Index])) != 0) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, COMTerminalType);
> +    } else if (CompareMem (&NewBmmData->COMFlowControl[Index], &OldBmmData->COMFlowControl[Index], sizeof (NewBmmData->COMFlowControl[Index])) != 0) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, COMFlowControl);
> +    }
> +    UpdateTerminalContent (NewBmmData);
> +    Status = Var_UpdateConsoleInpOption ();
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }
> +    Status = Var_UpdateConsoleOutOption ();
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }
> +    Status = Var_UpdateErrorOutOption ();
> +    if (EFI_ERROR (Status)) {
> +      goto Exit;
> +    }
>    }
>    //
>    // Check data which located in Console Options Menu and save the settings if need
>    //
>    if (CompareMem (NewBmmData->ConsoleInCheck, OldBmmData->ConsoleInCheck, sizeof (NewBmmData->ConsoleInCheck)) != 0){
> -    for (Index = 0; Index < ConsoleInpMenu.MenuNumber; Index++){
> -      NewMenuEntry                = BOpt_GetMenuEntry(&ConsoleInpMenu, Index);
> -      NewConsoleContext           = (BM_CONSOLE_CONTEXT *)NewMenuEntry->VariableContext;
> -      ASSERT (Index < MAX_MENU_NUMBER);
> -      NewConsoleContext->IsActive = NewBmmData->ConsoleInCheck[Index];
> -    }
> -    for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) {
> -      NewMenuEntry                = BOpt_GetMenuEntry (&TerminalMenu, Index);
> -      NewTerminalContext          = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext;
> -      ASSERT (Index + ConsoleInpMenu.MenuNumber < MAX_MENU_NUMBER);
> -      NewTerminalContext->IsConIn = NewBmmData->ConsoleInCheck[Index + ConsoleInpMenu.MenuNumber];
> +    UpdateConsoleContent (L"ConIn", NewBmmData);
> +    Status = Var_UpdateConsoleInpOption();
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, ConsoleInCheck);
> +      goto Exit;
>      }
> -    Var_UpdateConsoleInpOption();
>    }
>  
>    if (CompareMem (NewBmmData->ConsoleOutCheck, OldBmmData->ConsoleOutCheck, sizeof (NewBmmData->ConsoleOutCheck)) != 0){
> -    for (Index = 0; Index < ConsoleOutMenu.MenuNumber; Index++){
> -      NewMenuEntry                = BOpt_GetMenuEntry(&ConsoleOutMenu, Index);
> -      NewConsoleContext           = (BM_CONSOLE_CONTEXT *)NewMenuEntry->VariableContext;
> -      ASSERT (Index < MAX_MENU_NUMBER);
> -      NewConsoleContext->IsActive = NewBmmData->ConsoleOutCheck[Index];
> -    }
> -    for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) {
> -      NewMenuEntry                = BOpt_GetMenuEntry (&TerminalMenu, Index);
> -      NewTerminalContext          = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext;
> -      ASSERT (Index + ConsoleOutMenu.MenuNumber < MAX_MENU_NUMBER);
> -      NewTerminalContext->IsConOut = NewBmmData->ConsoleOutCheck[Index + ConsoleOutMenu.MenuNumber];
> +    UpdateConsoleContent (L"ConOut", NewBmmData);
> +    Status = Var_UpdateConsoleOutOption();
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, ConsoleOutCheck);
> +      goto Exit;
>      }
> -    Var_UpdateConsoleOutOption();
>    }
>  
>    if (CompareMem (NewBmmData->ConsoleErrCheck, OldBmmData->ConsoleErrCheck, sizeof (NewBmmData->ConsoleErrCheck)) != 0){
> -    for (Index = 0; Index < ConsoleErrMenu.MenuNumber; Index++){
> -      NewMenuEntry                = BOpt_GetMenuEntry(&ConsoleErrMenu, Index);
> -      NewConsoleContext           = (BM_CONSOLE_CONTEXT *)NewMenuEntry->VariableContext;
> -      ASSERT (Index < MAX_MENU_NUMBER);
> -      NewConsoleContext->IsActive = NewBmmData->ConsoleErrCheck[Index];
> -    }
> -    for (Index = 0; Index < TerminalMenu.MenuNumber; Index++) {
> -      NewMenuEntry                = BOpt_GetMenuEntry (&TerminalMenu, Index);
> -      NewTerminalContext          = (BM_TERMINAL_CONTEXT *) NewMenuEntry->VariableContext;
> -      ASSERT (Index + ConsoleErrMenu.MenuNumber < MAX_MENU_NUMBER);
> -      NewTerminalContext->IsStdErr = NewBmmData->ConsoleErrCheck[Index + ConsoleErrMenu.MenuNumber];
> +    UpdateConsoleContent (L"ErrOut", NewBmmData);
> +    Status = Var_UpdateErrorOutOption();
> +    if (EFI_ERROR (Status)) {
> +      Offset = OFFSET_OF (BMM_FAKE_NV_DATA, ConsoleErrCheck);
> +      goto Exit;
>      }
> -    Var_UpdateErrorOutOption();
>    }
>  
>    if (CompareMem (NewBmmData->BootDescriptionData, OldBmmData->BootDescriptionData, sizeof (NewBmmData->BootDescriptionData)) != 0 ||
>         CompareMem (NewBmmData->BootOptionalData, OldBmmData->BootOptionalData, sizeof (NewBmmData->BootOptionalData)) != 0) {
>      Status = Var_UpdateBootOption (Private);
>      NewBmmData->BootOptionChanged = FALSE;
>      if (EFI_ERROR (Status)) {
> -      return Status;
> +      if (CompareMem (NewBmmData->BootDescriptionData, OldBmmData->BootDescriptionData, sizeof (NewBmmData->BootDescriptionData)) != 0) {
> +        Offset = OFFSET_OF (BMM_FAKE_NV_DATA, BootDescriptionData);
> +      } else {
> +        Offset = OFFSET_OF (BMM_FAKE_NV_DATA, BootOptionalData);
> +      }
> +      goto Exit;
>      }
>      BOpt_GetBootOptions (Private);
>    }
>  
>    if (CompareMem (NewBmmData->DriverDescriptionData, OldBmmData->DriverDescriptionData, sizeof (NewBmmData->DriverDescriptionData)) != 0 ||
> @@ -834,11 +1024,16 @@ BootMaintRouteConfig (
>                NewBmmData->ForceReconnect
>                );
>      NewBmmData->DriverOptionChanged = FALSE;
>      NewBmmData->ForceReconnect      = TRUE;
>      if (EFI_ERROR (Status)) {
> -      return Status;
> +      if (CompareMem (NewBmmData->DriverDescriptionData, OldBmmData->DriverDescriptionData, sizeof (NewBmmData->DriverDescriptionData)) != 0) {
> +        Offset = OFFSET_OF (BMM_FAKE_NV_DATA, DriverDescriptionData);
> +      } else {
> +        Offset = OFFSET_OF (BMM_FAKE_NV_DATA, DriverOptionalData);
> +      }
> +      goto Exit;
>      }
>  
>      BOpt_GetDriverOptions (Private);
>    }
>  
> @@ -846,10 +1041,21 @@ BootMaintRouteConfig (
>    // After user do the save action, need to update OldBmmData.
>    //
>    CopyMem (OldBmmData, NewBmmData, sizeof (BMM_FAKE_NV_DATA));
>  
>    return EFI_SUCCESS;
> +
> +Exit:
> +  //
> +  // Fail to save the data, update the progress string.
> +  //
> +  *Progress = UpdateProgress (Offset, Configuration);
> +  if (Status == EFI_OUT_OF_RESOURCES) {
> +    return Status;
> +  } else {
> +    return EFI_NOT_FOUND;
> +  }
>  }
>  
>  /**
>    This function processes the results of changes in configuration.
>  
> @@ -880,10 +1086,11 @@ BootMaintCallback (
>    )
>  {
>    BMM_CALLBACK_DATA *Private;
>    BM_MENU_ENTRY     *NewMenuEntry;
>    BMM_FAKE_NV_DATA  *CurrentFakeNVMap;
> +  BMM_FAKE_NV_DATA  *OldFakeNVMap;
>    UINTN             Index;
>    EFI_DEVICE_PATH_PROTOCOL * File;
>  
>    if (Action != EFI_BROWSER_ACTION_CHANGING && Action != EFI_BROWSER_ACTION_CHANGED && Action != EFI_BROWSER_ACTION_FORM_OPEN) {
>      //
> @@ -914,10 +1121,11 @@ BootMaintCallback (
>    }
>    //
>    // Retrive uncommitted data from Form Browser
>    //
>    CurrentFakeNVMap = &Private->BmmFakeNvData;
> +  OldFakeNVMap     = &Private->BmmOldFakeNVData;
>    HiiGetBrowserData (&mBootMaintGuid, mBootMaintStorageName, sizeof (BMM_FAKE_NV_DATA), (UINT8 *) CurrentFakeNVMap);
>  
>    if (Action == EFI_BROWSER_ACTION_CHANGING) {
>      if (Value == NULL) {
>        return EFI_INVALID_PARAMETER;
> @@ -1016,21 +1224,25 @@ BootMaintCallback (
>        *ActionRequest = EFI_BROWSER_ACTION_REQUEST_FORM_SUBMIT_EXIT;
>      } else if (QuestionId == KEY_VALUE_NO_SAVE_AND_EXIT_DRIVER) {
>        //
>        // Discard changes and exit formset
>        //
> -      CurrentFakeNVMap->DriverOptionalData[0]     = 0x0000;
> -      CurrentFakeNVMap->DriverDescriptionData[0]  = 0x0000;
> +      ZeroMem (CurrentFakeNVMap->DriverOptionalData, sizeof (CurrentFakeNVMap->DriverOptionalData));
> +      ZeroMem (CurrentFakeNVMap->BootDescriptionData, sizeof (CurrentFakeNVMap->BootDescriptionData));
> +      ZeroMem (OldFakeNVMap->DriverOptionalData, sizeof (OldFakeNVMap->DriverOptionalData));
> +      ZeroMem (OldFakeNVMap->DriverDescriptionData, sizeof (OldFakeNVMap->DriverDescriptionData));
>        CurrentFakeNVMap->DriverOptionChanged = FALSE;
>        CurrentFakeNVMap->ForceReconnect      = TRUE;
>        *ActionRequest = EFI_BROWSER_ACTION_REQUEST_FORM_DISCARD_EXIT;
>      } else if (QuestionId == KEY_VALUE_NO_SAVE_AND_EXIT_BOOT) {
>        //
>        // Discard changes and exit formset
>        //
> -      CurrentFakeNVMap->BootOptionalData[0]     = 0x0000;
> -      CurrentFakeNVMap->BootDescriptionData[0]  = 0x0000;
> +      ZeroMem (CurrentFakeNVMap->BootOptionalData, sizeof (CurrentFakeNVMap->BootOptionalData));
> +      ZeroMem (CurrentFakeNVMap->BootDescriptionData, sizeof (CurrentFakeNVMap->BootDescriptionData));
> +      ZeroMem (OldFakeNVMap->BootOptionalData, sizeof (OldFakeNVMap->BootOptionalData));
> +      ZeroMem (OldFakeNVMap->BootDescriptionData, sizeof (OldFakeNVMap->BootDescriptionData));
>        CurrentFakeNVMap->BootOptionChanged = FALSE;
>        *ActionRequest = EFI_BROWSER_ACTION_REQUEST_FORM_DISCARD_EXIT;
>      } else if (QuestionId == KEY_VALUE_BOOT_DESCRIPTION || QuestionId == KEY_VALUE_BOOT_OPTION) {
>        CurrentFakeNVMap->BootOptionChanged = TRUE;
>      } else if (QuestionId == KEY_VALUE_DRIVER_DESCRIPTION || QuestionId == KEY_VALUE_DRIVER_OPTION) {
> @@ -1074,10 +1286,26 @@ BootMaintCallback (
>  
>        default:
>          break;
>        }
>      }
> +    //
> +    // Update the content in Terminal menu and Console menu here.
> +    //
> +    if ((QuestionId >= CON_IN_DEVICE_QUESTION_ID) && (QuestionId < CON_IN_DEVICE_QUESTION_ID + MAX_MENU_NUMBER)) {
> +      UpdateConsoleContent (L"ConIn",CurrentFakeNVMap);
> +      UpdateTerminalContent(CurrentFakeNVMap);
> +    } else if ((QuestionId >= CON_OUT_DEVICE_QUESTION_ID) && (QuestionId < CON_OUT_DEVICE_QUESTION_ID + MAX_MENU_NUMBER)) {
> +      UpdateConsoleContent (L"ConOut", CurrentFakeNVMap);
> +      UpdateTerminalContent(CurrentFakeNVMap);
> +    } else if ((QuestionId >= CON_ERR_DEVICE_QUESTION_ID) && (QuestionId < CON_ERR_DEVICE_QUESTION_ID + MAX_MENU_NUMBER)) {
> +      UpdateConsoleContent (L"ConErr", CurrentFakeNVMap);
> +      UpdateTerminalContent(CurrentFakeNVMap);
> +    }
>    }
>  
>    //
>    // Pass changed uncommitted data back to Form Browser
>    //
> diff --git a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
> index 9e79826..398c346 100644
> --- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
> +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/UpdatePage.c
> @@ -260,10 +260,11 @@ UpdateBootDelPage (
>        // CallbackData->BmmFakeNvData.BootOptionDelMark[Index] = FALSE means BDS knows the selected boot option has
>        // deleted, browser maintains old useless info. So clear this info here, and later update this info to browser
>        // through HiiSetBrowserData function.
>        //
>        CallbackData->BmmFakeNvData.BootOptionDel[Index] = FALSE;
> +      CallbackData->BmmOldFakeNVData.BootOptionDel[Index] = FALSE;
>      }
>      
>      HiiCreateCheckBoxOpCode (
>        mStartOpCodeHandle,
>        (EFI_QUESTION_ID) (BOOT_OPTION_DEL_QUESTION_ID + Index),
> @@ -346,10 +347,11 @@ UpdateDrvDelPage (
>        // CallbackData->BmmFakeNvData.BootOptionDelMark[Index] = FALSE means BDS knows the selected boot option has
>        // deleted, browser maintains old useless info. So clear this info here, and later update this info to browser
>        // through HiiSetBrowserData function.
>        //
>        CallbackData->BmmFakeNvData.DriverOptionDel[Index] = FALSE;
> +      CallbackData->BmmOldFakeNVData.DriverOptionDel[Index] = FALSE;
>      }
>      HiiCreateCheckBoxOpCode (
>        mStartOpCodeHandle,
>        (EFI_QUESTION_ID) (DRIVER_OPTION_DEL_QUESTION_ID + Index),
>        VARSTORE_ID_BOOT_MAINT,
> @@ -457,46 +459,36 @@ UpdateConsolePage (
>    BM_TERMINAL_CONTEXT *NewTerminalContext;
>    UINT16              Index;
>    UINT16              Index2;
>    UINT8               CheckFlags;
>    UINT8               *ConsoleCheck;
> -  UINT8               *OldConsoleCheck;
> -  UINTN               ConsoleCheckSize;
>    EFI_QUESTION_ID     QuestionIdBase;
>    UINT16              VariableOffsetBase;
>  
>    CallbackData->BmmAskSaveOrNot = TRUE;
>  
>    UpdatePageStart (CallbackData);
>  
>    ConsoleCheck       = NULL;
> -  OldConsoleCheck    = NULL;
>    QuestionIdBase     = 0;
>    VariableOffsetBase = 0;
> -  ConsoleCheckSize   = 0;
>  
>    switch (UpdatePageId) {
>    case FORM_CON_IN_ID:
>      ConsoleCheck       = &CallbackData->BmmFakeNvData.ConsoleInCheck[0];
> -    OldConsoleCheck    = &CallbackData->BmmOldFakeNVData.ConsoleInCheck[0];
> -    ConsoleCheckSize   = sizeof (CallbackData->BmmFakeNvData.ConsoleInCheck);
>      QuestionIdBase     = CON_IN_DEVICE_QUESTION_ID;
>      VariableOffsetBase = CON_IN_DEVICE_VAR_OFFSET;
>      break;
>  
>    case FORM_CON_OUT_ID:
>      ConsoleCheck       = &CallbackData->BmmFakeNvData.ConsoleOutCheck[0];
> -    OldConsoleCheck    = &CallbackData->BmmOldFakeNVData.ConsoleOutCheck[0];
> -    ConsoleCheckSize   = sizeof (CallbackData->BmmFakeNvData.ConsoleOutCheck);
>      QuestionIdBase     = CON_OUT_DEVICE_QUESTION_ID;
>      VariableOffsetBase = CON_OUT_DEVICE_VAR_OFFSET;
>      break;
>  
>    case FORM_CON_ERR_ID:
>      ConsoleCheck       = &CallbackData->BmmFakeNvData.ConsoleErrCheck[0];
> -    OldConsoleCheck    = &CallbackData->BmmOldFakeNVData.ConsoleErrCheck[0];
> -    ConsoleCheckSize   = sizeof (CallbackData->BmmFakeNvData.ConsoleErrCheck);
>      QuestionIdBase     = CON_ERR_DEVICE_QUESTION_ID;
>      VariableOffsetBase = CON_ERR_DEVICE_VAR_OFFSET;
>      break;
>    }
>    ASSERT (ConsoleCheck != NULL);
> @@ -517,11 +509,11 @@ UpdateConsolePage (
>        (EFI_QUESTION_ID) (QuestionIdBase + Index),
>        VARSTORE_ID_BOOT_MAINT,
>        (UINT16) (VariableOffsetBase + Index),
>        NewMenuEntry->DisplayStringToken,
>        NewMenuEntry->HelpStringToken,
> -      0,
> +      EFI_IFR_FLAG_CALLBACK,
>        CheckFlags,
>        NULL
>        );
>    }
>  
> @@ -546,20 +538,18 @@ UpdateConsolePage (
>        (EFI_QUESTION_ID) (QuestionIdBase + Index),
>        VARSTORE_ID_BOOT_MAINT,
>        (UINT16) (VariableOffsetBase + Index),
>        NewMenuEntry->DisplayStringToken,
>        NewMenuEntry->HelpStringToken,
> -      0,
> +      EFI_IFR_FLAG_CALLBACK,
>        CheckFlags,
>        NULL
>        );
>  
>      Index++;
>    }
>  
> -  CopyMem (OldConsoleCheck, ConsoleCheck, ConsoleCheckSize);
> -
>    UpdatePageEnd (CallbackData);
>  }
>  
>  /**
>    Update the page's NV Map if user has changed the order
> @@ -593,18 +583,34 @@ UpdateOrderPage (
>    QuestionId = 0;
>    VarOffset = 0;
>    switch (UpdatePageId) { 
>    
>    case FORM_BOOT_CHG_ID:
> -    GetBootOrder (CallbackData);
> +    //
> +    // If the BootOptionOrder in the BmmFakeNvData are same with the date in the BmmOldFakeNVData,
> +    // means all Boot Options has been save in BootOptionMenu, we can get the date from the menu.
> +    // else means browser maintains some uncommitted date which are not saved in BootOptionMenu,
> +    // so we should not get the data from BootOptionMenu to show it.
> +    //
> +    if (CompareMem (CallbackData->BmmFakeNvData.BootOptionOrder, CallbackData->BmmOldFakeNVData.BootOptionOrder, sizeof (CallbackData->BmmFakeNvData.BootOptionOrder)) == 0) {
> +      GetBootOrder (CallbackData);
> +    }
>      OptionOrder = CallbackData->BmmFakeNvData.BootOptionOrder;
>      QuestionId = BOOT_OPTION_ORDER_QUESTION_ID;
>      VarOffset = BOOT_OPTION_ORDER_VAR_OFFSET;
>      break;
>      
>    case FORM_DRV_CHG_ID:
> -    GetDriverOrder (CallbackData);
> +    //
> +    // If the DriverOptionOrder in the BmmFakeNvData are same with the date in the BmmOldFakeNVData,
> +    // means all Driver Options has been save in DriverOptionMenu, we can get the DriverOptionOrder from the menu.
> +    // else means browser maintains some uncommitted date which are not saved in DriverOptionMenu,
> +    // so we should not get the data from DriverOptionMenu to show it.
> +    //
> +    if (CompareMem (CallbackData->BmmFakeNvData.DriverOptionOrder, CallbackData->BmmOldFakeNVData.DriverOptionOrder, sizeof (CallbackData->BmmFakeNvData.DriverOptionOrder)) == 0) {
> +      GetDriverOrder (CallbackData);
> +    }
>      OptionOrder = CallbackData->BmmFakeNvData.DriverOptionOrder;
>      QuestionId = DRIVER_OPTION_ORDER_QUESTION_ID;
>      VarOffset = DRIVER_OPTION_ORDER_VAR_OFFSET;
>      break;
>    }  
> @@ -1035,15 +1041,19 @@ UpdateOptionPage(
>  
>    if(FormId == FORM_BOOT_ADD_ID){
>      if (!CallbackData->BmmFakeNvData.BootOptionChanged) {
>        ZeroMem (CallbackData->BmmFakeNvData.BootOptionalData, sizeof (CallbackData->BmmFakeNvData.BootOptionalData));
>        ZeroMem (CallbackData->BmmFakeNvData.BootDescriptionData, sizeof (CallbackData->BmmFakeNvData.BootDescriptionData));
> +      ZeroMem (CallbackData->BmmOldFakeNVData.BootOptionalData, sizeof (CallbackData->BmmOldFakeNVData.BootOptionalData));
> +      ZeroMem (CallbackData->BmmOldFakeNVData.BootDescriptionData, sizeof (CallbackData->BmmOldFakeNVData.BootDescriptionData));
>      }
>    } else if (FormId == FORM_DRV_ADD_FILE_ID){
>      if (!CallbackData->BmmFakeNvData.DriverOptionChanged) {
>        ZeroMem (CallbackData->BmmFakeNvData.DriverOptionalData, sizeof (CallbackData->BmmFakeNvData.DriverOptionalData));
>        ZeroMem (CallbackData->BmmFakeNvData.DriverDescriptionData, sizeof (CallbackData->BmmFakeNvData.DriverDescriptionData));
> +      ZeroMem (CallbackData->BmmOldFakeNVData.DriverOptionalData, sizeof (CallbackData->BmmOldFakeNVData.DriverOptionalData));
> +      ZeroMem (CallbackData->BmmOldFakeNVData.DriverDescriptionData, sizeof (CallbackData->BmmOldFakeNVData.DriverDescriptionData));
>      }
>    }
>  
>    RefreshUpdateData();
>    mStartLabel->Number = FormId;
> diff --git a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c
> index b65d6a5..a2ae2a7 100644
> --- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c
> +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c
> @@ -463,11 +463,13 @@ Var_UpdateErrorOutOption (
>    @param HiiHandle       The HII handle associated with the BMM formset.
>    @param DescriptionData The description of this driver option.
>    @param OptionalData    The optional load option.
>    @param ForceReconnect  If to force reconnect.
>  
> -  @retval EFI_OUT_OF_RESOURCES If not enought memory to complete the operation.
> +  @retval other                Contain some errors when excuting this function.See function
> +                               EfiBootManagerInitializeLoadOption/EfiBootManagerAddLoadOptionVariabl
> +                               for detail return information.
>    @retval EFI_SUCCESS          If function completes successfully.
>  
>  **/
>  EFI_STATUS
>  Var_UpdateDriverOption (
> @@ -523,12 +525,18 @@ Var_UpdateDriverOption (
>               DescriptionData,
>               CallbackData->LoadContext->FilePathList,
>               OptionalDesData,
>               OptionalDataSize
>             );
> -  if (!EFI_ERROR (Status)){
> -    Status = EfiBootManagerAddLoadOptionVariable (&LoadOption,(UINTN) -1 );
> +  if (EFI_ERROR (Status)){
> +    return Status;
> +  }
> +
> +  Status = EfiBootManagerAddLoadOptionVariable (&LoadOption,(UINTN) -1 );
> +  if (EFI_ERROR (Status)) {
> +    EfiBootManagerFreeLoadOption(&LoadOption);
> +    return Status;
>    }
>  
>    NewLoadContext                  = (BM_LOAD_CONTEXT *) NewMenuEntry->VariableContext;
>    NewLoadContext->Deleted         = FALSE;
>    NewLoadContext->Attributes = LoadOption.Attributes;
> @@ -580,11 +588,13 @@ Var_UpdateDriverOption (
>    the "BootOrder" list. It also append this Boot Opotion to the end
>    of BootOptionMenu.
>  
>    @param CallbackData    The BMM context data.
>  
> -  @retval EFI_OUT_OF_RESOURCES If not enought memory to complete the operation.
> +  @retval other                Contain some errors when excuting this function. See function
> +                               EfiBootManagerInitializeLoadOption/EfiBootManagerAddLoadOptionVariabl
> +                               for detail return information.
>    @retval EFI_SUCCESS          If function completes successfully.
>  
>  **/
>  EFI_STATUS
>  Var_UpdateBootOption (
> @@ -633,12 +643,18 @@ Var_UpdateBootOption (
>               NvRamMap->BootDescriptionData,
>               CallbackData->LoadContext->FilePathList,
>               OptionalData,
>               OptionalDataSize
>             );
> -  if (!EFI_ERROR (Status)){
> -    Status = EfiBootManagerAddLoadOptionVariable (&LoadOption,(UINTN) -1 );
> +  if (EFI_ERROR (Status)){
> +    return Status;
> +  }
> +
> +  Status = EfiBootManagerAddLoadOptionVariable (&LoadOption,(UINTN) -1 );
> +  if (EFI_ERROR (Status)) {
> +    EfiBootManagerFreeLoadOption(&LoadOption);
> +    return Status;
>    }
>  
>    NewLoadContext                  = (BM_LOAD_CONTEXT *) NewMenuEntry->VariableContext;
>    NewLoadContext->Deleted         = FALSE;
>    NewLoadContext->Attributes = LoadOption.Attributes;
>