Hi Siyuan,
I just found a possible memory leak issue with the following code block. Others
look good to me.
+ Status = EfiBootManagerAddLoadOptionVariable (&NewOption, (UINTN) -1);
+ if (EFI_ERROR (Status)) {
+ EfiBootManagerFreeLoadOption (&NewOption);
+ }
I think we always need to call EfiBootManagerFreeLoadOption no matter
EfiBootManagerAddLoadOptionVariable returns error or not because the memory
allocated for NewOption (description and device path) is no longer needed. We
already set NewOption to variable, so the data in memory will not be used
anymore. Therefore, we can modify the code block above to the following, what
do you think?
+ EfiBootManagerAddLoadOptionVariable (&NewOption, (UINTN) -1);
+ EfiBootManagerFreeLoadOption (&NewOption);
By the way, thanks for addressing my comment in the other review.
Regards,
Sunny Wang
-----Original Message-----
From: edk2-devel [mailto:[email protected]] On Behalf Of Fu Siyuan
Sent: Wednesday, May 04, 2016 1:58 PM
To: [email protected]
Cc: Ye Ting <[email protected]>; Ni Ruiyu <[email protected]>; Wu Jiaxin
<[email protected]>
Subject: [edk2] [PATCH v2] NetworkPkg: Use UefiBootManagerLib API to create
load option.
V2:
Remove unnecessary ZeroMem and free load option.
This patch updates the HTTP boot driver to use the API in UefiBootManagerLib to
create new load option, to avoid duplicate code.
Cc: Ye Ting <[email protected]>
Cc: Wu Jiaxin <[email protected]>
Cc: Ni Ruiyu <[email protected]>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Fu Siyuan <[email protected]>
---
NetworkPkg/HttpBootDxe/HttpBootConfig.c | 136 ++++++--------------------------
NetworkPkg/HttpBootDxe/HttpBootDxe.inf | 1 +
NetworkPkg/NetworkPkg.dsc | 8 ++
3 files changed, 33 insertions(+), 112 deletions(-)
diff --git a/NetworkPkg/HttpBootDxe/HttpBootConfig.c
b/NetworkPkg/HttpBootDxe/HttpBootConfig.c
index 00e4782..2ca38b5 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootConfig.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootConfig.c
@@ -13,6 +13,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
**/
#include "HttpBootDxe.h"
+#include <Library/UefiBootManagerLib.h>
CHAR16 mHttpBootConfigStorageName[] = L"HTTP_BOOT_CONFIG_IFR_NVDATA";
@@ -36,31 +37,18 @@ HttpBootAddBootOption (
IN CHAR16 *Uri
)
{
- EFI_DEV_PATH *Node;
- EFI_DEVICE_PATH_PROTOCOL *TmpDevicePath;
- EFI_DEVICE_PATH_PROTOCOL *NewDevicePath;
- UINTN Length;
- CHAR8 AsciiUri[URI_STR_MAX_SIZE];
- CHAR16 *CurrentOrder;
- EFI_STATUS Status;
- UINTN OrderCount;
- UINTN TargetLocation;
- BOOLEAN Found;
- UINT8 *TempByteBuffer;
- UINT8 *TempByteStart;
- UINTN DescSize;
- UINTN FilePathSize;
- CHAR16 OptionStr[10];
- UINT16 *NewOrder;
- UINTN Index;
-
- NewOrder = NULL;
- TempByteStart = NULL;
+ EFI_DEV_PATH *Node;
+ EFI_DEVICE_PATH_PROTOCOL *TmpDevicePath;
+ EFI_DEVICE_PATH_PROTOCOL *NewDevicePath;
+ UINTN Length;
+ CHAR8 AsciiUri[URI_STR_MAX_SIZE];
+ EFI_STATUS Status;
+ UINTN Index;
+ EFI_BOOT_MANAGER_LOAD_OPTION NewOption;
+
NewDevicePath = NULL;
- NewOrder = NULL;
Node = NULL;
TmpDevicePath = NULL;
- CurrentOrder = NULL;
if (StrLen (Description) == 0) {
return EFI_INVALID_PARAMETER;
@@ -137,105 +125,29 @@ HttpBootAddBootOption (
}
//
- // Get current "BootOrder" variable and find a free target.
- //
- Length = 0;
- Status = GetVariable2 (
- L"BootOrder",
- &gEfiGlobalVariableGuid,
- (VOID **)&CurrentOrder,
- &Length
- );
- if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND) {
- goto ON_EXIT;
- }
- OrderCount = Length / sizeof (UINT16);
- Found = FALSE;
- for (TargetLocation=0; TargetLocation < 0xFFFF; TargetLocation++) {
- Found = TRUE;
- for (Index = 0; Index < OrderCount; Index++) {
- if (CurrentOrder[Index] == TargetLocation) {
- Found = FALSE;
- break;
- }
- }
- if (Found) {
- break;
- }
- }
-
- if (TargetLocation == 0xFFFF) {
- DEBUG ((EFI_D_ERROR, "Could not find unused target index.\n"));
- Status = EFI_OUT_OF_RESOURCES;
- goto ON_EXIT;
- } else {
- DEBUG ((EFI_D_INFO, "TargetIndex = %04x.\n", TargetLocation));
- }
-
- //
- // Construct and set the "Boot####" variable
+ // Add a new load option.
//
- DescSize = StrSize(Description);
- FilePathSize = GetDevicePathSize (NewDevicePath);
- TempByteBuffer = AllocateZeroPool(sizeof(EFI_LOAD_OPTION) + DescSize +
FilePathSize);
- if (TempByteBuffer == NULL) {
- Status = EFI_OUT_OF_RESOURCES;
- goto ON_EXIT;
- }
-
- TempByteStart = TempByteBuffer;
- *((UINT32 *) TempByteBuffer) = LOAD_OPTION_ACTIVE; // Attributes
- TempByteBuffer += sizeof (UINT32);
-
- *((UINT16 *) TempByteBuffer) = (UINT16)FilePathSize; // FilePathListLength
- TempByteBuffer += sizeof (UINT16);
-
- CopyMem (TempByteBuffer, Description, DescSize);
- TempByteBuffer += DescSize;
- CopyMem (TempByteBuffer, NewDevicePath, FilePathSize);
-
- UnicodeSPrint (OptionStr, sizeof(OptionStr), L"%s%04x", L"Boot",
TargetLocation);
- Status = gRT->SetVariable (
- OptionStr,
- &gEfiGlobalVariableGuid,
- EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS
| EFI_VARIABLE_RUNTIME_ACCESS,
- sizeof(UINT32) + sizeof(UINT16) + DescSize + FilePathSize,
- TempByteStart
- );
+ Status = EfiBootManagerInitializeLoadOption (
+ &NewOption,
+ LoadOptionNumberUnassigned,
+ LoadOptionTypeBoot,
+ LOAD_OPTION_ACTIVE,
+ Description,
+ NewDevicePath,
+ NULL,
+ 0
+ );
if (EFI_ERROR (Status)) {
goto ON_EXIT;
}
- //
- // Insert into the order list and set "BootOrder" variable
- //
- NewOrder = AllocateZeroPool ((OrderCount + 1) * sizeof (UINT16));
- if (NewOrder == NULL) {
- Status = EFI_OUT_OF_RESOURCES;
- goto ON_EXIT;
+ Status = EfiBootManagerAddLoadOptionVariable (&NewOption, (UINTN)
+ -1); if (EFI_ERROR (Status)) {
+ EfiBootManagerFreeLoadOption (&NewOption);
}
- CopyMem(NewOrder, CurrentOrder, OrderCount * sizeof(UINT16));
- NewOrder[OrderCount] = (UINT16) TargetLocation;
- Status = gRT->SetVariable (
- L"BootOrder",
- &gEfiGlobalVariableGuid,
- EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS
| EFI_VARIABLE_RUNTIME_ACCESS,
- ((OrderCount + 1) * sizeof (UINT16)),
- NewOrder
- );
-
ON_EXIT:
- if (CurrentOrder != NULL) {
- FreePool (CurrentOrder);
- }
- if (NewOrder != NULL) {
- FreePool (NewOrder);
- }
- if (TempByteStart != NULL) {
- FreePool (TempByteStart);
- }
if (NewDevicePath != NULL) {
FreePool (NewDevicePath);
}
diff --git a/NetworkPkg/HttpBootDxe/HttpBootDxe.inf
b/NetworkPkg/HttpBootDxe/HttpBootDxe.inf
index d3df5f7..e6ce864 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootDxe.inf
+++ b/NetworkPkg/HttpBootDxe/HttpBootDxe.inf
@@ -61,6 +61,7 @@
HiiLib
PrintLib
UefiHiiServicesLib
+ UefiBootManagerLib
[Protocols]
## TO_START
diff --git a/NetworkPkg/NetworkPkg.dsc b/NetworkPkg/NetworkPkg.dsc index
0695dc1..3c809d5 100644
--- a/NetworkPkg/NetworkPkg.dsc
+++ b/NetworkPkg/NetworkPkg.dsc
@@ -39,6 +39,12 @@
UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
UefiHiiServicesLib|MdeModulePkg/Library/UefiHiiServicesLib/UefiHiiServicesLib.inf
+
+ UefiBootManagerLib|MdeModulePkg/Library/UefiBootManagerLib/UefiBootMan
+ agerLib.inf
+ TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTempl
+ ate.inf
+ PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLi
+ bNull.inf
+ PeCoffGetEntryPointLib|MdePkg/Library/BasePeCoffGetEntryPointLib/BaseP
+ eCoffGetEntryPointLib.inf
+ DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
+
+ DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTabl
+ eLib.inf
DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf
NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
@@ -54,6 +60,8 @@
SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
[LibraryClasses.common.UEFI_DRIVER]
+ HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
+
+ ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRep
+ ortStatusCodeLib.inf
DebugLib|MdePkg/Library/UefiDebugLibConOut/UefiDebugLibConOut.inf
[LibraryClasses.common.UEFI_APPLICATION]
--
2.7.4.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
