Hi Ray,
All of your V2 patches (01-12) look good to me. Thanks for addressing
all my comments.
Reviewed by: Sunny Wang <sunnyw...@hpe.com>
Regards,
Sunny Wang
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Friday, November 13, 2015 7:45 PM
To: edk2-devel@lists.01.org
Cc: Ruiyu Ni
Subject: [edk2] [Patch V2 00/12] Add PlatformRecovery support to BDS core
The V2 patch serials adopted below comments:
7/12
[Sunny] I would like to make sure that changing to different behavior for the
unsupported OptionType is intended. The original code would directly return
EFI_NOT_FOUND and do nothing for the unsupported OptionType.
However, in this patch, the unsupported OptionType will cause ASSERT and would
possibly still run this code block to SetVariable. Could you check whether this
is an expected behavior? Is it change to "} else if (OptionType ==
LoadOptionTypePlatformRecovery) {"
instead of "} else {" and "ASSERT (....."
[Sunny] Just a suggestion. Is it a good idea to rename it to
BM_OPTION_NAME_MAX_LEN or add comment here to explain why we use
PlatformRecovery#### to
get BM_OPTION_NAME_LEN (because it is the option variable with longest name)?
9/12
[Sunny] Are these two lines above having the wrong indent?
[Ray] I checked the change and the indent should be correct.
11/12
[Sunny] It would be good to update the comment as well because it doesn't match
your modification. You can cut part of UEFI 2.5 Section 3.1.1 Boot Manager
Programming, the 5th paragraph below to update the comment.
If the boot via Boot#### returns with a status of EFI_SUCCESS, platform
firmware supports boot manager menu, and if firmware is configured to boot in
an interactive mode, the boot manager will stop processing the BootOrder
variable and present a boot manager menu to the user.
[Sunny] Do we need to initialize BootSuccess to FALSE here? It looks like the
uninitialized BootSuccess variable would cause build failure.
[Sunny] May I know why we need to set Status to EFI_SUCCESS here? If we need to
do this, could you add comment here? :)
Ruiyu Ni (12):
MdePkg: Add Platform Recovery definitions.
MdeModulePkg: Add Bm prefix for internal functions
MdeModulePkg: Use BmCharToUint in BmIsKeyOptionVariable
MdeModulePkg: Use BM_OPTION_NAME_LEN instead of sizeof L"Boot####"
MdeModulePkg: Use BmForEachVariable to collect all key options
MdeModulePkg: Support to expand File device path
MdeModulePkg: Add Platform recovery support
MdeModulePkg: Add missing PrintLib to BdsDxe.inf
MdeModulePkg: Use UefiSpec.h defined macro to replace L"xxx" string
MdeModulePkg: Add PlatformRecovery#### pointing to default file path
MdeModulePkg: Enable PlatformRecovery in BdsDxe driver
MdeModulePkg/VarCheck: Add VarCheck handler for PlatformRecovery####
MdeModulePkg/Include/Library/UefiBootManagerLib.h | 1 +
MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c | 76 ++++
MdeModulePkg/Library/UefiBootManagerLib/BmHotkey.c | 181 +++++----
.../Library/UefiBootManagerLib/BmLoadOption.c | 154 +++++--
MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 26 ++
.../Library/UefiBootManagerLib/InternalBm.h | 33 +-
.../UefiBootManagerLib/UefiBootManagerLib.inf | 1 +
MdeModulePkg/Library/VarCheckLib/VarCheckLib.c | 7 +
.../VarCheckUefiLib/VarCheckUefiLibNullClass.c | 11 +
MdeModulePkg/Universal/BdsDxe/Bds.h | 3 -
MdeModulePkg/Universal/BdsDxe/BdsDxe.inf | 1 +
MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 448 ++++++---------------
MdePkg/Include/Uefi/UefiSpec.h | 1 +
13 files changed, 494 insertions(+), 449 deletions(-)
--
1.9.5.msysgit.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
