Re: [patch] MdeModulePkg/BMMUiLib: Replace same logic with API in UefiBootManagerLib

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

Thanks/Ray

> -----Original Message-----
> From: Bi, Dandan
> Sent: Monday, February 27, 2017 3:15 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: [patch] MdeModulePkg/BMMUiLib: Replace same logic with API in
> UefiBootManagerLib
> 
> Use the API　EfiBootManagerDeleteLoadOptionVariable in
> UefiBootManagerLib　to replace the same logic in function
> Var_DelBootOption/Var_DelDriverOption.
> This can make code clean and prevent potential bugs.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=391
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Dandan Bi <dandan.bi@intel.com>
> ---
>  .../BootMaintenanceManager.h                       |  37 +---
>  .../Library/BootMaintenanceManagerUiLib/Variable.c | 202 +-------------------
> -
>  2 files changed, 11 insertions(+), 228 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManager.h
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManager.h
> index 532b75b..a8d7a0f 100644
> ---
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eManager.h
> +++
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootMaintenanc
> eMa
> +++ nager.h
> @@ -631,12 +631,10 @@ Var_UpdateBootOption (
>    IN  BMM_CALLBACK_DATA                   *CallbackData
>    );
> 
>  /**
>    Delete Boot Option that represent a Deleted state in BootOptionMenu.
> -  After deleting this boot option, call Var_ChangeBootOrder to
> -  make sure BootOrder is in valid state.
> 
>    @retval EFI_SUCCESS   If all boot load option EFI Variables corresponding to
>                          BM_LOAD_CONTEXT marked for deletion is deleted
>    @return Others        If failed to update the "BootOrder" variable after
> deletion.
> 
> @@ -645,25 +643,10 @@ EFI_STATUS
>  Var_DelBootOption (
>    VOID
>    );
> 
>  /**
> -  After any operation on Boot####, there will be a discrepancy in BootOrder.
> -  Since some are missing but in BootOrder, while some are present but are
> -  not reflected by BootOrder. Then a function rebuild BootOrder from
> -  scratch by content from BootOptionMenu is needed.
> -
> -  @retval  EFI_SUCCESS  The boot order is updated successfully.
> -  @return  other than EFI_SUCCESS if failed to change the "BootOrder" EFI
> Variable.
> -
> -**/
> -EFI_STATUS
> -Var_ChangeBootOrder (
> -  VOID
> -  );
> -
> -/**
>    This function create a currently loaded Drive Option from
>    the BMM. It then appends this Driver Option to the end of
>    the "DriverOrder" list. It append this Driver Opotion to the end
>    of DriverOptionMenu.
> 
> @@ -685,13 +668,11 @@ Var_UpdateDriverOption (
>    IN  UINT16                    *OptionalData,
>    IN  UINT8                     ForceReconnect
>    );
> 
>  /**
> -  Delete Load Option that represent a Deleted state in BootOptionMenu.
> -  After deleting this Driver option, call Var_ChangeDriverOrder to
> -  make sure DriverOrder is in valid state.
> +  Delete Load Option that represent a Deleted state in DriverOptionMenu.
> 
>    @retval EFI_SUCCESS Load Option is successfully updated.
>    @return Other value than EFI_SUCCESS if failed to update "Driver Order"
> EFI
>            Variable.
> 
> @@ -700,26 +681,10 @@ EFI_STATUS
>  Var_DelDriverOption (
>    VOID
>    );
> 
>  /**
> -  After any operation on Driver####, there will be a discrepancy in
> -  DriverOrder. Since some are missing but in DriverOrder, while some
> -  are present but are not reflected by DriverOrder. Then a function
> -  rebuild DriverOrder from scratch by content from DriverOptionMenu is
> -  needed.
> -
> -  @retval  EFI_SUCCESS  The driver order is updated successfully.
> -  @return  other than EFI_SUCCESS if failed to set the "DriverOrder" EFI
> Variable.
> -
> -**/
> -EFI_STATUS
> -Var_ChangeDriverOrder (
> -  VOID
> -  );
> -
> -/**
>    This function delete and build multi-instance device path ConIn
>    console device.
> 
>    @retval EFI_SUCCESS    The function complete successfully.
>    @return The EFI variable can not be saved. See gRT->SetVariable for detail
> return information.
> diff --git
> a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c
> b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c
> index 746b233..c1c55b0 100644
> --- a/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c
> +++ b/MdeModulePkg/Library/BootMaintenanceManagerUiLib/Variable.c
> @@ -1,9 +1,9 @@
>  /** @file
>  Variable operation that will be used by bootmaint
> 
> -Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials  are licensed and made
> available under the terms and conditions of the BSD License  which
> accompanies this distribution.  The full text of the license may be found at
> http://opensource.org/licenses/bsd-license.php
> 
> @@ -14,12 +14,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
> 
>  #include "BootMaintenanceManager.h"
> 
>  /**
>    Delete Boot Option that represent a Deleted state in BootOptionMenu.
> -  After deleting this boot option, call Var_ChangeBootOrder to
> -  make sure BootOrder is in valid state.
> 
>    @retval EFI_SUCCESS   If all boot load option EFI Variables corresponding to
>                          BM_LOAD_CONTEXT marked for deletion is deleted.
>    @retval EFI_NOT_FOUND If can not find the boot option want to be
> deleted.
>    @return Others        If failed to update the "BootOrder" variable after
> deletion.
> @@ -30,16 +28,14 @@ Var_DelBootOption (
>    VOID
>    )
>  {
>    BM_MENU_ENTRY   *NewMenuEntry;
>    BM_LOAD_CONTEXT *NewLoadContext;
> -  UINT16          BootString[10];
>    EFI_STATUS      Status;
>    UINTN           Index;
>    UINTN           Index2;
> 
> -  Status  = EFI_SUCCESS;
>    Index2  = 0;
>    for (Index = 0; Index < BootOptionMenu.MenuNumber; Index++) {
>      NewMenuEntry = BOpt_GetMenuEntry (&BootOptionMenu, (Index -
> Index2));
>      if (NULL == NewMenuEntry) {
>        return EFI_NOT_FOUND;
> @@ -48,18 +44,14 @@ Var_DelBootOption (
>      NewLoadContext = (BM_LOAD_CONTEXT *) NewMenuEntry-
> >VariableContext;
>      if (!NewLoadContext->Deleted) {
>        continue;
>      }
> 
> -    UnicodeSPrint (
> -      BootString,
> -      sizeof (BootString),
> -      L"Boot%04x",
> -      NewMenuEntry->OptionNumber
> -      );
> -
> -    EfiLibDeleteVariable (BootString, &gEfiGlobalVariableGuid);
> +    Status = EfiBootManagerDeleteLoadOptionVariable (NewMenuEntry-
> >OptionNumber,LoadOptionTypeBoot);
> +    if (EFI_ERROR (Status)) {
> +     return Status;
> +    }
>      Index2++;
>      //
>      // If current Load Option is the same as BootNext,
>      // must delete BootNext in order to make sure
>      // there will be no panic on next boot @@ -73,106 +65,15 @@
> Var_DelBootOption (
>      NewMenuEntry = NULL;
>    }
> 
>    BootOptionMenu.MenuNumber -= Index2;
> 
> -  Status = Var_ChangeBootOrder ();
> -  return Status;
> -}
> -
> -/**
> -  After any operation on Boot####, there will be a discrepancy in BootOrder.
> -  Since some are missing but in BootOrder, while some are present but are
> -  not reflected by BootOrder. Then a function rebuild BootOrder from
> -  scratch by content from BootOptionMenu is needed.
> -
> -
> -
> -
> -  @retval  EFI_SUCCESS  The boot order is updated successfully.
> -  @return               EFI_STATUS other than EFI_SUCCESS if failed to
> -                        Set the "BootOrder" EFI Variable.
> -
> -**/
> -EFI_STATUS
> -Var_ChangeBootOrder (
> -  VOID
> -  )
> -{
> -
> -  EFI_STATUS    Status;
> -  BM_MENU_ENTRY *NewMenuEntry;
> -  UINT16        *BootOrderList;
> -  UINT16        *BootOrderListPtr;
> -  UINTN         BootOrderListSize;
> -  UINTN         Index;
> -
> -  BootOrderList     = NULL;
> -  BootOrderListSize = 0;
> -  //
> -  // First check whether BootOrder is present in current configuration
> -  //
> -  GetEfiGlobalVariable2 (L"BootOrder", (VOID **) &BootOrderList,
> &BootOrderListSize);
> -
> -  //
> -  // If exists, delete it to hold new BootOrder
> -  //
> -  if (BootOrderList != NULL) {
> -    EfiLibDeleteVariable (L"BootOrder", &gEfiGlobalVariableGuid);
> -    FreePool (BootOrderList);
> -    BootOrderList = NULL;
> -  }
> -  //
> -  // Maybe here should be some check method to ensure that
> -  // no new added boot options will be added
> -  // but the setup engine now will give only one callback
> -  // that is to say, user are granted only one chance to
> -  // decide whether the boot option will be added or not
> -  // there should be no indictor to show whether this
> -  // is a "new" boot option
> -  //
> -  BootOrderListSize = BootOptionMenu.MenuNumber;
> -
> -  if (BootOrderListSize > 0) {
> -    BootOrderList = AllocateZeroPool (BootOrderListSize * sizeof (UINT16));
> -    ASSERT (BootOrderList != NULL);
> -    BootOrderListPtr = BootOrderList;
> -
> -    //
> -    // Get all current used Boot#### from BootOptionMenu.
> -    // OptionNumber in each BM_LOAD_OPTION is really its
> -    // #### value.
> -    //
> -    for (Index = 0; Index < BootOrderListSize; Index++) {
> -      NewMenuEntry    = BOpt_GetMenuEntry (&BootOptionMenu, Index);
> -      *BootOrderList  = (UINT16) NewMenuEntry->OptionNumber;
> -      BootOrderList++;
> -    }
> -
> -    BootOrderList = BootOrderListPtr;
> -
> -    //
> -    // After building the BootOrderList, write it back
> -    //
> -    Status = gRT->SetVariable (
> -                    L"BootOrder",
> -                    &gEfiGlobalVariableGuid,
> -                    VAR_FLAG,
> -                    BootOrderListSize * sizeof (UINT16),
> -                    BootOrderList
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      return Status;
> -    }
> -  }
>    return EFI_SUCCESS;
>  }
> 
>  /**
> -  Delete Load Option that represent a Deleted state in BootOptionMenu.
> -  After deleting this Driver option, call Var_ChangeDriverOrder to
> -  make sure DriverOrder is in valid state.
> +  Delete Load Option that represent a Deleted state in DriverOptionMenu.
> 
>    @retval EFI_SUCCESS       Load Option is successfully updated.
>    @retval EFI_NOT_FOUND     Fail to find the driver option want to be
> deleted.
>    @return Other value than EFI_SUCCESS if failed to update "Driver Order"
> EFI
>            Variable.
> @@ -183,16 +84,14 @@ Var_DelDriverOption (
>    VOID
>    )
>  {
>    BM_MENU_ENTRY   *NewMenuEntry;
>    BM_LOAD_CONTEXT *NewLoadContext;
> -  UINT16          DriverString[12];
>    EFI_STATUS      Status;
>    UINTN           Index;
>    UINTN           Index2;
> 
> -  Status  = EFI_SUCCESS;
>    Index2  = 0;
>    for (Index = 0; Index < DriverOptionMenu.MenuNumber; Index++) {
>      NewMenuEntry = BOpt_GetMenuEntry (&DriverOptionMenu, (Index -
> Index2));
>      if (NULL == NewMenuEntry) {
>        return EFI_NOT_FOUND;
> @@ -200,105 +99,24 @@ Var_DelDriverOption (
> 
>      NewLoadContext = (BM_LOAD_CONTEXT *) NewMenuEntry-
> >VariableContext;
>      if (!NewLoadContext->Deleted) {
>        continue;
>      }
> +    Status = EfiBootManagerDeleteLoadOptionVariable (NewMenuEntry-
> >OptionNumber,LoadOptionTypeDriver);
> +    if (EFI_ERROR (Status)) {
> +      return Status;
> +    }
> 
> -    UnicodeSPrint (
> -      DriverString,
> -      sizeof (DriverString),
> -      L"Driver%04x",
> -      NewMenuEntry->OptionNumber
> -      );
> -
> -    EfiLibDeleteVariable (DriverString, &gEfiGlobalVariableGuid);
>      Index2++;
> 
>      RemoveEntryList (&NewMenuEntry->Link);
>      BOpt_DestroyMenuEntry (NewMenuEntry);
>      NewMenuEntry = NULL;
>    }
> 
>    DriverOptionMenu.MenuNumber -= Index2;
> 
> -  Status = Var_ChangeDriverOrder ();
> -  return Status;
> -}
> -
> -/**
> -  After any operation on Driver####, there will be a discrepancy in
> -  DriverOrder. Since some are missing but in DriverOrder, while some
> -  are present but are not reflected by DriverOrder. Then a function
> -  rebuild DriverOrder from scratch by content from DriverOptionMenu is
> -  needed.
> -
> -  @retval  EFI_SUCCESS  The driver order is updated successfully.
> -  @return  Other status than EFI_SUCCESS if failed to set the "DriverOrder"
> EFI Variable.
> -
> -**/
> -EFI_STATUS
> -Var_ChangeDriverOrder (
> -  VOID
> -  )
> -{
> -  EFI_STATUS    Status;
> -  BM_MENU_ENTRY *NewMenuEntry;
> -  UINT16        *DriverOrderList;
> -  UINT16        *DriverOrderListPtr;
> -  UINTN         DriverOrderListSize;
> -  UINTN         Index;
> -
> -  DriverOrderList     = NULL;
> -  DriverOrderListSize = 0;
> -
> -  //
> -  // First check whether DriverOrder is present in current configuration
> -  //
> -  GetEfiGlobalVariable2 (L"DriverOrder", (VOID **) &DriverOrderList,
> &DriverOrderListSize);
> -  //
> -  // If exists, delete it to hold new DriverOrder
> -  //
> -  if (DriverOrderList != NULL) {
> -    EfiLibDeleteVariable (L"DriverOrder", &gEfiGlobalVariableGuid);
> -    FreePool (DriverOrderList);
> -    DriverOrderList = NULL;
> -  }
> -
> -  DriverOrderListSize = DriverOptionMenu.MenuNumber;
> -
> -  if (DriverOrderListSize > 0) {
> -    DriverOrderList = AllocateZeroPool (DriverOrderListSize * sizeof (UINT16));
> -    ASSERT (DriverOrderList != NULL);
> -    DriverOrderListPtr = DriverOrderList;
> -
> -    //
> -    // Get all current used Driver#### from DriverOptionMenu.
> -    // OptionNumber in each BM_LOAD_OPTION is really its
> -    // #### value.
> -    //
> -    for (Index = 0; Index < DriverOrderListSize; Index++) {
> -      NewMenuEntry      = BOpt_GetMenuEntry (&DriverOptionMenu, Index);
> -      *DriverOrderList  = (UINT16) NewMenuEntry->OptionNumber;
> -      DriverOrderList++;
> -    }
> -
> -    DriverOrderList = DriverOrderListPtr;
> -
> -    //
> -    // After building the DriverOrderList, write it back
> -    //
> -    Status = gRT->SetVariable (
> -                    L"DriverOrder",
> -                    &gEfiGlobalVariableGuid,
> -                    VAR_FLAG,
> -                    DriverOrderListSize * sizeof (UINT16),
> -                    DriverOrderList
> -                    );
> -    if (EFI_ERROR (Status)) {
> -      return Status;
> -    }
> -  }
>    return EFI_SUCCESS;
>  }
> 
>  /**
>    This function delete and build multi-instance device path for
> --
> 1.9.5.msysgit.1