Re: [edk2-devel] [PATCH V3 1/2] MdePkg/UefiDevicePathLib: Separate the device path lib

2019-12-18 Thread Vitaly Cheptsov via Groups.Io
Reviewed-by: Vitaly Cheptsov 

On Wed, Dec 18, 2019 at 05:10, Zhichao Gao  wrote:

> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2298
>
> UefiDevicePathLibOptionalDevicePathProtocol's implementation isn't
> fit its description. It should be implement as blow:
> Try to find the DevicePathProtocol, if found then use it to implement
> the interface. Else, use the local interface. It should not have the
> depex and ASSERT of gEfiDevicePathUtilitiesProtocolGuid when not find
> the DevicePathProtocol.
>
> Add a mandatory one to force using the DevicePathUtilities,
> DevicePathToText and DevicePathFromText protocol.
>
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Vitaly Cheptsov 
> Tested-by: Zhichao Gao 
> Signed-off-by: Zhichao Gao 
> ---
>
> V2:
> Optional one should always return EFI_SUCCESS in its constructor.
> Change the description of optional one's uni file.
> Fix the copyright date of mandatory one's uni file.
>
> V3:
> Remove the Status variable in
> UefiDevicePathLibOptionalDevicePathProtocolConstructor.
> The Status would cause GCC build fail because the variable is
> initialized but not used. Since it is useless for the constructor,
> directly remove it.
>
> ...DevicePathLibMandatoryDevicePathProtocol.c | 469 ++
> ...vicePathLibMandatoryDevicePathProtocol.inf | 86 
> ...vicePathLibMandatoryDevicePathProtocol.uni | 18 +
> ...iDevicePathLibOptionalDevicePathProtocol.c | 21 +-
> ...evicePathLibOptionalDevicePathProtocol.inf | 5 +-
> ...evicePathLibOptionalDevicePathProtocol.uni | 6 +-
> 6 files changed, 585 insertions(+), 20 deletions(-)
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.inf
> create mode 100644 
> MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.uni
>
> diff --git 
> a/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
>  
> b/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
> new file mode 100644
> index 00..fa27110fd4
> --- /dev/null
> +++ 
> b/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
> @@ -0,0 +1,469 @@
> +/** @file
> + Device Path services. The thing to remember is device paths are built out of
> + nodes. The device path is terminated by an end node that is length
> + sizeof(EFI_DEVICE_PATH_PROTOCOL). That would be why there is 
> sizeof(EFI_DEVICE_PATH_PROTOCOL)
> + all over this file.
> +
> + The only place where multi-instance device paths are supported is in
> + environment varibles. Multi-instance device paths should never be placed
> + on a Handle.
> +
> + Copyright (c) 2019, Intel Corporation. All rights reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +
> +#include "UefiDevicePathLib.h"
> +
> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_UTILITIES_PROTOCOL 
> *mDevicePathLibDevicePathUtilities = NULL;
> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_TO_TEXT_PROTOCOL 
> *mDevicePathLibDevicePathToText = NULL;
> +GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL 
> *mDevicePathLibDevicePathFromText = NULL;
> +
> +/**
> + The constructor function caches the pointer to DevicePathUtilites protocol,
> + DevicePathToText protocol and DevicePathFromText protocol.
> +
> + The constructor function locates these three protocols from protocol 
> database.
> + It will caches the pointer to local protocol instance if that operation 
> fails
> + and it will always return EFI_SUCCESS.
> +
> + @param ImageHandle The firmware allocated handle for the EFI image.
> + @param SystemTable A pointer to the EFI System Table.
> +
> + @retval EFI_SUCCESS The constructor always returns EFI_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +UefiDevicePathLibMandatoryDevicePathProtocolConstructor (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + EFI_STATUS Status;
> +
> + Status = gBS->LocateProtocol (
> + ,
> + NULL,
> + (VOID**) 
> + );
> + ASSERT_EFI_ERROR (Status);
> + ASSERT (mDevicePathLibDevicePathUtilities != NULL);
> +
> + Status = gBS->LocateProtocol (
> + ,
> + NULL,
> + (VOID**) 
> + );
> + ASSERT_EFI_ERROR (Status);
> + ASSERT (mDevicePathLibDevicePathToText != NULL);
> +
> + Status = gBS->LocateProtocol (
> + ,
> + NULL,
> + (VOID**) 
> + );
> + ASSERT_EFI_ERROR (Status);
> + ASSERT (mDevicePathLibDevicePathFromText != NULL);
> +
> + return Status;
> +}
> +
> +/**
> + Returns the size of a device path in bytes.
> +
> + This function returns the size, in bytes, of the device path data structure
> + specified by DevicePath including the end of device path node.
> + If DevicePath is NULL or invalid, then 0 is returned.
> +
> + @param DevicePath A pointer to a device path data structure.
> +
> + @retval 0 If DevicePath is NULL or invalid.
> + @retval Others The size of a device path in bytes.
> +
> +**/

[edk2-devel] [PATCH V3 1/2] MdePkg/UefiDevicePathLib: Separate the device path lib

2019-12-17 Thread Gao, Zhichao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2298

UefiDevicePathLibOptionalDevicePathProtocol's implementation isn't
fit its description. It should be implement as blow:
Try to find the DevicePathProtocol, if found then use it to implement
the interface. Else, use the local interface. It should not have the
depex and ASSERT of gEfiDevicePathUtilitiesProtocolGuid when not find
the DevicePathProtocol.

Add a mandatory one to force using the DevicePathUtilities,
DevicePathToText and DevicePathFromText protocol.

Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Vitaly Cheptsov 
Tested-by: Zhichao Gao 
Signed-off-by: Zhichao Gao 
---

V2:
Optional one should always return EFI_SUCCESS in its constructor.
Change the description of optional one's uni file.
Fix the copyright date of mandatory one's uni file.

V3:
Remove the Status variable in
UefiDevicePathLibOptionalDevicePathProtocolConstructor.
The Status would cause GCC build fail because the variable is
initialized but not used. Since it is useless for the constructor,
directly remove it.

 ...DevicePathLibMandatoryDevicePathProtocol.c | 469 ++
 ...vicePathLibMandatoryDevicePathProtocol.inf |  86 
 ...vicePathLibMandatoryDevicePathProtocol.uni |  18 +
 ...iDevicePathLibOptionalDevicePathProtocol.c |  21 +-
 ...evicePathLibOptionalDevicePathProtocol.inf |   5 +-
 ...evicePathLibOptionalDevicePathProtocol.uni |   6 +-
 6 files changed, 585 insertions(+), 20 deletions(-)
 create mode 100644 
MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
 create mode 100644 
MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.inf
 create mode 100644 
MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.uni

diff --git 
a/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
 
b/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
new file mode 100644
index 00..fa27110fd4
--- /dev/null
+++ 
b/MdePkg/Library/UefiDevicePathLib/UefiDevicePathLibMandatoryDevicePathProtocol.c
@@ -0,0 +1,469 @@
+/** @file
+  Device Path services. The thing to remember is device paths are built out of
+  nodes. The device path is terminated by an end node that is length
+  sizeof(EFI_DEVICE_PATH_PROTOCOL). That would be why there is 
sizeof(EFI_DEVICE_PATH_PROTOCOL)
+  all over this file.
+
+  The only place where multi-instance device paths are supported is in
+  environment varibles. Multi-instance device paths should never be placed
+  on a Handle.
+
+  Copyright (c) 2019, Intel Corporation. All rights reserved.
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+#include "UefiDevicePathLib.h"
+
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_UTILITIES_PROTOCOL 
*mDevicePathLibDevicePathUtilities = NULL;
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_TO_TEXT_PROTOCOL   
*mDevicePathLibDevicePathToText= NULL;
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL 
*mDevicePathLibDevicePathFromText  = NULL;
+
+/**
+  The constructor function caches the pointer to DevicePathUtilites protocol,
+  DevicePathToText protocol and DevicePathFromText protocol.
+
+  The constructor function locates these three protocols from protocol 
database.
+  It will caches the pointer to local protocol instance if that operation fails
+  and it will always return EFI_SUCCESS.
+
+  @param  ImageHandle   The firmware allocated handle for the EFI image.
+  @param  SystemTable   A pointer to the EFI System Table.
+
+  @retval EFI_SUCCESS   The constructor always returns EFI_SUCCESS.
+
+**/
+EFI_STATUS
+EFIAPI
+UefiDevicePathLibMandatoryDevicePathProtocolConstructor (
+  IN  EFI_HANDLEImageHandle,
+  IN  EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUSStatus;
+
+  Status = gBS->LocateProtocol (
+  ,
+  NULL,
+  (VOID**) 
+  );
+  ASSERT_EFI_ERROR (Status);
+  ASSERT (mDevicePathLibDevicePathUtilities != NULL);
+
+  Status = gBS->LocateProtocol (
+  ,
+  NULL,
+  (VOID**) 
+  );
+  ASSERT_EFI_ERROR (Status);
+  ASSERT (mDevicePathLibDevicePathToText != NULL);
+
+  Status = gBS->LocateProtocol (
+  ,
+  NULL,
+  (VOID**) 
+  );
+  ASSERT_EFI_ERROR (Status);
+  ASSERT (mDevicePathLibDevicePathFromText != NULL);
+
+  return Status;
+}
+
+/**
+  Returns the size of a device path in bytes.
+
+  This function returns the size, in bytes, of the device path data structure
+  specified by DevicePath including the end of device path node.
+  If DevicePath is NULL or invalid, then 0 is returned.
+
+  @param  DevicePath  A pointer to a device path data structure.
+
+  @retval 0   If DevicePath is NULL or invalid.
+  @retval Others  The size of a device path in bytes.
+
+**/
+UINTN
+EFIAPI