[PATCH 0/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol info

[PATCH 0/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol info

[Jeff Westfahl]

In the old shell, 'dh -v' showed some useful information about the file path
associated with a LoadedImageProtocol that is no longer shown in the modern
shell.

For example, with the old shell:

    Handle D3 (3A552218)
    Image (3A54C918)   File:MicrocodeUpdate
        ParentHandle..: 3A666398
        SystemTable...: 3D2A8F18
        DeviceHandle..: 3B1C8098
        FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
        ImageBase.....: 3D650000 - 3D655540
        ImageSize.....: 5540
        CodeType......: RT_code   
        DataType......: RT_data   

compared to the new shell:

    D3: 3A552218
    LoadedImage
        Revision......: 0x00001000
        ParentHandle..: 3A666398
        SystemTable...: 3D2A8F18
        DeviceHandle..: 3B1C8098
        FilePath......: 3A552298
        OptionsSize...: 0
        LoadOptions...: 0
        ImageBase.....: 3D650000
        ImageSize.....: 5540
        CodeType......: EfiRuntimeServicesCode
        DataType......: EfiRuntimeServicesData
        Unload........: 0

Here is the output for the same handle with this series applied:

    D3: 3A552218
    LoadedImage
        Name..........: MicrocodeUpdate
        Revision......: 0x00001000
        ParentHandle..: 3A666398
        SystemTable...: 3D2A8F18
        DeviceHandle..: 3B1C8098
        FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
        OptionsSize...: 0
        LoadOptions...: 0
        ImageBase.....: 3D650000
        ImageSize.....: 5540
        CodeType......: EfiRuntimeServicesCode
        DataType......: EfiRuntimeServicesData
        Unload........: 0

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>

Jeff Westfahl (3):
  ShellPkg/HandleParsingLib: Show LoadedImageProtocol file path as text
  ShellPkg/HandleParsingLib: Open LoadedImageProtocol first
  ShellPkg/HandleParsingLib: Show LoadedImageProtocol file name

 .../UefiHandleParsingLib/UefiHandleParsingLib.c    | 111 +++++++++++++++++++--
 .../UefiHandleParsingLib/UefiHandleParsingLib.uni  |   4 +-
 2 files changed, 104 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol file path as text

[Jeff Westfahl]

This patch adds support for displaying a text representation of the file
path associated with a LoadedImageProtocol. This is a behavior that was
present in the old shell but has been lost in the new shell.

For example, using 'dh -v' in the old shell:

    FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
    FilePath......: \EFI\BOOT\BOOTX64.EFI

vs. the new shell:

    FilePath......: 3A539018
    FilePath......: 3A728718

This seems like useful information for the shell to display.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c   | 6 +++++-
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
index da1d92f..2db8a3a 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -169,6 +169,7 @@ LoadedImageProtocolDumpInformation(
   EFI_STATUS                        Status;
   CHAR16                            *RetVal;
   CHAR16                            *Temp;
+  CHAR16                            *FilePath;
   CHAR16                            *CodeType;
   CHAR16                            *DataType;
 
@@ -197,6 +198,8 @@ LoadedImageProtocolDumpInformation(
     return NULL;
   }
 
+  FilePath = ConvertDevicePathToText(LoadedImage->FilePath, TRUE, TRUE);
+
   DataType = ConvertMemoryType(LoadedImage->ImageDataType);
   CodeType = ConvertMemoryType(LoadedImage->ImageCodeType);
 
@@ -207,7 +210,7 @@ LoadedImageProtocolDumpInformation(
              LoadedImage->ParentHandle,
              LoadedImage->SystemTable,
              LoadedImage->DeviceHandle,
-             LoadedImage->FilePath,
+             FilePath,
              LoadedImage->LoadOptionsSize,
              LoadedImage->LoadOptions,
              LoadedImage->ImageBase,
@@ -219,6 +222,7 @@ LoadedImageProtocolDumpInformation(
 
 
   SHELL_FREE_NON_NULL(Temp);
+  SHELL_FREE_NON_NULL(FilePath);
   SHELL_FREE_NON_NULL(CodeType);
   SHELL_FREE_NON_NULL(DataType);
 
diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
index 273a420..7b3711d 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
@@ -352,7 +352,7 @@
                                                   "     ParentHandle..: %%H%x%%N\r\n"
                                                   "     SystemTable...: %%H%x%%N\r\n"
                                                   "     DeviceHandle..: %%H%x%%N\r\n"
-                                                  "     FilePath......: %%H%x%%N\r\n"
+                                                  "     FilePath......: %%H%s%%N\r\n"
                                                   "     OptionsSize...: %%H%x%%N\r\n"
                                                   "     LoadOptions...: %%H%x%%N\r\n"
                                                   "     ImageBase.....: %%H%x%%N\r\n"
-- 
2.7.4

[PATCH 2/3] ShellPkg/HandleParsingLib: Open LoadedImageProtocol first

[Jeff Westfahl]

This patch changes the order of operations to make sure we can open the
LoadedImageProtocol before getting the format string. This should not
affect functionality, and makes the next patch easier to review.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 .../Library/UefiHandleParsingLib/UefiHandleParsingLib.c   | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
index 2db8a3a..c96f6dd 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -177,13 +177,6 @@ LoadedImageProtocolDumpInformation(
     return (CatSPrint(NULL, L"LoadedImage"));
   }
 
-  HandleParsingHiiInit();
-
-  Temp = HiiGetString(mHandleParsingHiiHandle, STRING_TOKEN(STR_LI_DUMP_MAIN), NULL);
-  if (Temp == NULL) {
-    return NULL;
-  }
-
   Status = gBS->OpenProtocol (
                 TheHandle,
                 &gEfiLoadedImageProtocolGuid,
@@ -194,7 +187,13 @@ LoadedImageProtocolDumpInformation(
                );
 
   if (EFI_ERROR (Status)) {
-    SHELL_FREE_NON_NULL (Temp);
+    return NULL;
+  }
+
+  HandleParsingHiiInit();
+
+  Temp = HiiGetString(mHandleParsingHiiHandle, STRING_TOKEN(STR_LI_DUMP_MAIN), NULL);
+  if (Temp == NULL) {
     return NULL;
   }
 
-- 
2.7.4

[PATCH 3/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol file name

[Jeff Westfahl]

This patch adds support for showing the file name associated with a
LoadedImageProtocol file path. This is a behavior that was present in
the old shell but has been lost in the new shell.

For example, using 'dh -v' in the old shell:

    Handle D3 (3A552218)
    Image (3A54C918)   File:MicrocodeUpdate
        ParentHandle..: 3A666398

vs. the new shell:

    D3: 3A552218
    LoadedImage
        Revision......: 0x00001000
        ParentHandle..: 3A666398

Here's what the output of 'dh -v' looks like after this patch:

    D3: 3A552218
    LoadedImage
        Name..........: MicrocodeUpdate
        Revision......: 0x00001000
        ParentHandle..: 3A666398

This seems like useful information for the shell to display.

Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 .../UefiHandleParsingLib/UefiHandleParsingLib.c    | 90 +++++++++++++++++++++-
 .../UefiHandleParsingLib/UefiHandleParsingLib.uni  |  2 +
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
index c96f6dd..74934f8 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -16,12 +16,85 @@
 
 #include "UefiHandleParsingLib.h"
 #include "IndustryStandard/Acpi10.h"
+#include "Pi/PiFirmwareFile.h"
+#include "Pi/PiFirmwareVolume.h"
+#include "Protocol/FirmwareVolume2.h"
 
 EFI_HANDLE        mHandleParsingHiiHandle = NULL;
 HANDLE_INDEX_LIST mHandleList = {{{NULL,NULL},0,0},0};
 GUID_INFO_BLOCK   *mGuidList;
 UINTN             mGuidListCount;
 /**
+  Function to find the file name associated with a LoadedImageProtocol.
+
+  @param[in] LoadedImage     An instance of LoadedImageProtocol.
+
+  @retval                    A string representation of the file name associated
+                             with LoadedImage, or NULL if no name can be found.
+**/
+CHAR16*
+FindLoadedImageFileName (
+  IN EFI_LOADED_IMAGE_PROTOCOL *LoadedImage
+  )
+{
+  EFI_GUID                       *NameGuid;
+  EFI_STATUS                     Status;
+  EFI_FIRMWARE_VOLUME2_PROTOCOL  *Fv;
+  VOID                           *Buffer;
+  UINTN                          BufferSize;
+  UINT32                         AuthenticationStatus;
+
+  //
+  // Only subtype MEDIA_PIWG_FW_FILE_DP of type MEDIA_DEVICE_PATH is supported.
+  //
+  if (   (LoadedImage == NULL)
+      || (LoadedImage->FilePath == NULL)
+      || (LoadedImage->FilePath->Type != MEDIA_DEVICE_PATH)
+      || (LoadedImage->FilePath->SubType != MEDIA_PIWG_FW_FILE_DP)
+   ) {
+    return (NULL);
+  }
+
+  NameGuid = EfiGetNameGuidFromFwVolDevicePathNode((MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)LoadedImage->FilePath);
+
+  if (NameGuid == NULL) {
+    return (NULL);
+  }
+
+  //
+  // Get the FirmwareVolume2Protocol of the device handle that this image was
+  // loaded from.
+  //
+  Status = gBS->HandleProtocol(
+    LoadedImage->DeviceHandle,
+    &gEfiFirmwareVolume2ProtocolGuid,
+    (VOID**)&Fv);
+
+  //
+  // FirmwareVolume2Protocol is PI, and is not required to be available.
+  //
+  if (EFI_ERROR(Status)) {
+    return (NULL);
+  }
+
+  //
+  // Read the user interface section of the image.
+  //
+  Buffer = NULL;
+  Status = Fv->ReadSection(Fv, NameGuid, EFI_SECTION_USER_INTERFACE, 0, &Buffer, &BufferSize, &AuthenticationStatus);
+
+  if (EFI_ERROR(Status)) {
+    return (NULL);
+  }
+
+  //
+  // ReadSection returns just the section data, without any section header. For
+  // a user interface section, the only data is the file name.
+  //
+  return (Buffer);
+}
+
+/**
   Function to translate the EFI_MEMORY_TYPE into a string.
 
   @param[in] Memory     The memory type.
@@ -169,6 +242,7 @@ LoadedImageProtocolDumpInformation(
   EFI_STATUS                        Status;
   CHAR16                            *RetVal;
   CHAR16                            *Temp;
+  CHAR16                            *FileName;
   CHAR16                            *FilePath;
   CHAR16                            *CodeType;
   CHAR16                            *DataType;
@@ -192,6 +266,20 @@ LoadedImageProtocolDumpInformation(
 
   HandleParsingHiiInit();
 
+  FileName = FindLoadedImageFileName(LoadedImage);
+
+  RetVal = NULL;
+  if (FileName) {
+    Temp = HiiGetString(mHandleParsingHiiHandle, STRING_TOKEN(STR_LI_DUMP_NAME), NULL);
+
+    if (Temp != NULL) {
+      RetVal = CatSPrint(NULL, Temp, FileName);
+    }
+
+    SHELL_FREE_NON_NULL(Temp);
+    SHELL_FREE_NON_NULL(FileName);
+  }
+
   Temp = HiiGetString(mHandleParsingHiiHandle, STRING_TOKEN(STR_LI_DUMP_MAIN), NULL);
   if (Temp == NULL) {
     return NULL;
@@ -203,7 +291,7 @@ LoadedImageProtocolDumpInformation(
   CodeType = ConvertMemoryType(LoadedImage->ImageCodeType);
 
   RetVal = CatSPrint(
-             NULL,
+             RetVal,
              Temp,
              LoadedImage->Revision,
              LoadedImage->ParentHandle,
diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
index 7b3711d..e07c9a1 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
@@ -348,6 +348,8 @@
 
 
 
+#string STR_LI_DUMP_NAME          #language en-US "     Name..........: %%H%s%%N\r\n"
+
 #string STR_LI_DUMP_MAIN          #language en-US "     Revision......: %%H0x%08x%%N\r\n"
                                                   "     ParentHandle..: %%H%x%%N\r\n"
                                                   "     SystemTable...: %%H%x%%N\r\n"
-- 
2.7.4

Re: [PATCH 0/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol info

[Ni, Ruiyu]

Jeff,
Firstly great thanks for filling the gap between EDK shell and UEFI shell.
There are many behavior differences between the two,  and EDK Shell does carry much more information that
are very helpful for developer debugging.

Before I check in your patch, could you please kindly review whether the additional change is ok to you.
One change is to fix the ECC failure: "python BaseTools\Source\Python\Ecc\Ecc.py -t ShellPkg\Library\UefiHandleParsingLib -s"

ShellPkg\Library\UefiHandleParsingLib\UefiHandleParsingLib.c(272): [3002]Non-Boolean comparisons should use a compare operator (==, !=, >, < >=, <=) Predicate Expression: FileName
ShellPkg\Library\UefiHandleParsingLib\UefiHandleParsingLib.c(272): [3003]A comparison of any pointer to zero must be done via the NULL type Predicate Expression: FileName

====patch content===
.../UefiHandleParsingLib/UefiHandleParsingLib.c    | 39 ++++++++--------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
index 74934f8617..d3ee068eba 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -16,14 +16,14 @@
 
 #include "UefiHandleParsingLib.h"
 #include "IndustryStandard/Acpi10.h"
-#include "Pi/PiFirmwareFile.h"
-#include "Pi/PiFirmwareVolume.h"
-#include "Protocol/FirmwareVolume2.h"
+#include <PiDxe.h>
+#include <Protocol/FirmwareVolume2.h>
 
 EFI_HANDLE        mHandleParsingHiiHandle = NULL;
 HANDLE_INDEX_LIST mHandleList = {{{NULL,NULL},0,0},0};
 GUID_INFO_BLOCK   *mGuidList;
 UINTN             mGuidListCount;
+
 /**
   Function to find the file name associated with a LoadedImageProtocol.
 
@@ -44,37 +44,26 @@ FindLoadedImageFileName (
   UINTN                          BufferSize;
   UINT32                         AuthenticationStatus;
 
-  //
-  // Only subtype MEDIA_PIWG_FW_FILE_DP of type MEDIA_DEVICE_PATH is supported.
-  //
-  if (   (LoadedImage == NULL)
-      || (LoadedImage->FilePath == NULL)
-      || (LoadedImage->FilePath->Type != MEDIA_DEVICE_PATH)
-      || (LoadedImage->FilePath->SubType != MEDIA_PIWG_FW_FILE_DP)
-   ) {
-    return (NULL);
+  if ((LoadedImage == NULL) || (LoadedImage->FilePath == NULL)) {
+    return NULL;
   }
 
   NameGuid = EfiGetNameGuidFromFwVolDevicePathNode((MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)LoadedImage->FilePath);
 
   if (NameGuid == NULL) {
-    return (NULL);
+    return NULL;
   }
 
   //
-  // Get the FirmwareVolume2Protocol of the device handle that this image was
-  // loaded from.
+  // Get the FirmwareVolume2Protocol of the device handle that this image was loaded from.
   //
-  Status = gBS->HandleProtocol(
-    LoadedImage->DeviceHandle,
-    &gEfiFirmwareVolume2ProtocolGuid,
-    (VOID**)&Fv);
+  Status = gBS->HandleProtocol (LoadedImage->DeviceHandle, &gEfiFirmwareVolume2ProtocolGuid, (VOID**) &Fv);
 
   //
   // FirmwareVolume2Protocol is PI, and is not required to be available.
   //
-  if (EFI_ERROR(Status)) {
-    return (NULL);
+  if (EFI_ERROR (Status)) {
+    return NULL;
   }
 
   //
@@ -83,15 +72,15 @@ FindLoadedImageFileName (
   Buffer = NULL;
   Status = Fv->ReadSection(Fv, NameGuid, EFI_SECTION_USER_INTERFACE, 0, &Buffer, &BufferSize, &AuthenticationStatus);
 
-  if (EFI_ERROR(Status)) {
-    return (NULL);
+  if (EFI_ERROR (Status)) {
+    return NULL;
   }
 
   //
   // ReadSection returns just the section data, without any section header. For
   // a user interface section, the only data is the file name.
   //
-  return (Buffer);
+  return Buffer;
 }
 
 /**
@@ -269,7 +258,7 @@ LoadedImageProtocolDumpInformation(
   FileName = FindLoadedImageFileName(LoadedImage);
 
   RetVal = NULL;
-  if (FileName) {
+  if (FileName != NULL) {
     Temp = HiiGetString(mHandleParsingHiiHandle, STRING_TOKEN(STR_LI_DUMP_NAME), NULL);
 
     if (Temp != NULL) {

Thanks/Ray

> -----Original Message-----
> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> Sent: Friday, May 5, 2017 5:53 AM
> To: edk2-devel@lists.01.org
> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Carsey, Jaben <jaben.carsey@intel.com>
> Subject: [PATCH 0/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol
> info
> 
> In the old shell, 'dh -v' showed some useful information about the file path
> associated with a LoadedImageProtocol that is no longer shown in the modern
> shell.
> 
> For example, with the old shell:
> 
>     Handle D3 (3A552218)
>     Image (3A54C918)   File:MicrocodeUpdate
>         ParentHandle..: 3A666398
>         SystemTable...: 3D2A8F18
>         DeviceHandle..: 3B1C8098
>         FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
>         ImageBase.....: 3D650000 - 3D655540
>         ImageSize.....: 5540
>         CodeType......: RT_code
>         DataType......: RT_data
> 
> compared to the new shell:
> 
>     D3: 3A552218
>     LoadedImage
>         Revision......: 0x00001000
>         ParentHandle..: 3A666398
>         SystemTable...: 3D2A8F18
>         DeviceHandle..: 3B1C8098
>         FilePath......: 3A552298
>         OptionsSize...: 0
>         LoadOptions...: 0
>         ImageBase.....: 3D650000
>         ImageSize.....: 5540
>         CodeType......: EfiRuntimeServicesCode
>         DataType......: EfiRuntimeServicesData
>         Unload........: 0
> 
> Here is the output for the same handle with this series applied:
> 
>     D3: 3A552218
>     LoadedImage
>         Name..........: MicrocodeUpdate
>         Revision......: 0x00001000
>         ParentHandle..: 3A666398
>         SystemTable...: 3D2A8F18
>         DeviceHandle..: 3B1C8098
>         FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
>         OptionsSize...: 0
>         LoadOptions...: 0
>         ImageBase.....: 3D650000
>         ImageSize.....: 5540
>         CodeType......: EfiRuntimeServicesCode
>         DataType......: EfiRuntimeServicesData
>         Unload........: 0
> 
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> 
> Jeff Westfahl (3):
>   ShellPkg/HandleParsingLib: Show LoadedImageProtocol file path as text
>   ShellPkg/HandleParsingLib: Open LoadedImageProtocol first
>   ShellPkg/HandleParsingLib: Show LoadedImageProtocol file name
> 
>  .../UefiHandleParsingLib/UefiHandleParsingLib.c    | 111
> +++++++++++++++++++--
>  .../UefiHandleParsingLib/UefiHandleParsingLib.uni  |   4 +-
>  2 files changed, 104 insertions(+), 11 deletions(-)
> 
> --
> 2.7.4

Re: [PATCH 0/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol info

[Jeff Westfahl]

Hi Ray,

Thank you for pointing out the ECC tool. These changes look good to me.

Reviewed-by: Jeff Westfahl <jeff.westfahl@ni.com>

On Fri, 5 May 2017, Ni, Ruiyu wrote:

> Jeff,
> Firstly great thanks for filling the gap between EDK shell and UEFI shell.
> There are many behavior differences between the two,  and EDK Shell does carry much more information that
> are very helpful for developer debugging.
>
> Before I check in your patch, could you please kindly review whether the additional change is ok to you.
> One change is to fix the ECC failure: "python BaseTools\Source\Python\Ecc\Ecc.py -t ShellPkg\Library\UefiHandleParsingLib -s"
>
> ShellPkg\Library\UefiHandleParsingLib\UefiHandleParsingLib.c(272): [3002]Non-Boolean comparisons should use a compare operator (==, !=, >, < >=, <=) Predicate Expression: FileName
> ShellPkg\Library\UefiHandleParsingLib\UefiHandleParsingLib.c(272): [3003]A comparison of any pointer to zero must be done via the NULL type Predicate Expression: FileName
>
> ====patch content===
> .../UefiHandleParsingLib/UefiHandleParsingLib.c    | 39 ++++++++--------------
> 1 file changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
> index 74934f8617..d3ee068eba 100644
> --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
> +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
> @@ -16,14 +16,14 @@
>
> #include "UefiHandleParsingLib.h"
> #include "IndustryStandard/Acpi10.h"
> -#include "Pi/PiFirmwareFile.h"
> -#include "Pi/PiFirmwareVolume.h"
> -#include "Protocol/FirmwareVolume2.h"
> +#include <PiDxe.h>
> +#include <Protocol/FirmwareVolume2.h>
>
> EFI_HANDLE        mHandleParsingHiiHandle = NULL;
> HANDLE_INDEX_LIST mHandleList = {{{NULL,NULL},0,0},0};
> GUID_INFO_BLOCK   *mGuidList;
> UINTN             mGuidListCount;
> +
> /**
>   Function to find the file name associated with a LoadedImageProtocol.
>
> @@ -44,37 +44,26 @@ FindLoadedImageFileName (
>   UINTN                          BufferSize;
>   UINT32                         AuthenticationStatus;
>
> -  //
> -  // Only subtype MEDIA_PIWG_FW_FILE_DP of type MEDIA_DEVICE_PATH is supported.
> -  //
> -  if (   (LoadedImage == NULL)
> -      || (LoadedImage->FilePath == NULL)
> -      || (LoadedImage->FilePath->Type != MEDIA_DEVICE_PATH)
> -      || (LoadedImage->FilePath->SubType != MEDIA_PIWG_FW_FILE_DP)
> -   ) {
> -    return (NULL);
> +  if ((LoadedImage == NULL) || (LoadedImage->FilePath == NULL)) {
> +    return NULL;
>   }
>
>   NameGuid = EfiGetNameGuidFromFwVolDevicePathNode((MEDIA_FW_VOL_FILEPATH_DEVICE_PATH *)LoadedImage->FilePath);
>
>   if (NameGuid == NULL) {
> -    return (NULL);
> +    return NULL;
>   }
>
>   //
> -  // Get the FirmwareVolume2Protocol of the device handle that this image was
> -  // loaded from.
> +  // Get the FirmwareVolume2Protocol of the device handle that this image was loaded from.
>   //
> -  Status = gBS->HandleProtocol(
> -    LoadedImage->DeviceHandle,
> -    &gEfiFirmwareVolume2ProtocolGuid,
> -    (VOID**)&Fv);
> +  Status = gBS->HandleProtocol (LoadedImage->DeviceHandle, &gEfiFirmwareVolume2ProtocolGuid, (VOID**) &Fv);
>
>   //
>   // FirmwareVolume2Protocol is PI, and is not required to be available.
>   //
> -  if (EFI_ERROR(Status)) {
> -    return (NULL);
> +  if (EFI_ERROR (Status)) {
> +    return NULL;
>   }
>
>   //
> @@ -83,15 +72,15 @@ FindLoadedImageFileName (
>   Buffer = NULL;
>   Status = Fv->ReadSection(Fv, NameGuid, EFI_SECTION_USER_INTERFACE, 0, &Buffer, &BufferSize, &AuthenticationStatus);
>
> -  if (EFI_ERROR(Status)) {
> -    return (NULL);
> +  if (EFI_ERROR (Status)) {
> +    return NULL;
>   }
>
>   //
>   // ReadSection returns just the section data, without any section header. For
>   // a user interface section, the only data is the file name.
>   //
> -  return (Buffer);
> +  return Buffer;
> }
>
> /**
> @@ -269,7 +258,7 @@ LoadedImageProtocolDumpInformation(
>   FileName = FindLoadedImageFileName(LoadedImage);
>
>   RetVal = NULL;
> -  if (FileName) {
> +  if (FileName != NULL) {
>     Temp = HiiGetString(mHandleParsingHiiHandle, STRING_TOKEN(STR_LI_DUMP_NAME), NULL);
>
>     if (Temp != NULL) {
>
> Thanks/Ray
>
>> -----Original Message-----
>> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
>> Sent: Friday, May 5, 2017 5:53 AM
>> To: edk2-devel@lists.01.org
>> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
>> Carsey, Jaben <jaben.carsey@intel.com>
>> Subject: [PATCH 0/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol
>> info
>>
>> In the old shell, 'dh -v' showed some useful information about the file path
>> associated with a LoadedImageProtocol that is no longer shown in the modern
>> shell.
>>
>> For example, with the old shell:
>>
>>     Handle D3 (3A552218)
>>     Image (3A54C918)   File:MicrocodeUpdate
>>         ParentHandle..: 3A666398
>>         SystemTable...: 3D2A8F18
>>         DeviceHandle..: 3B1C8098
>>         FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
>>         ImageBase.....: 3D650000 - 3D655540
>>         ImageSize.....: 5540
>>         CodeType......: RT_code
>>         DataType......: RT_data
>>
>> compared to the new shell:
>>
>>     D3: 3A552218
>>     LoadedImage
>>         Revision......: 0x00001000
>>         ParentHandle..: 3A666398
>>         SystemTable...: 3D2A8F18
>>         DeviceHandle..: 3B1C8098
>>         FilePath......: 3A552298
>>         OptionsSize...: 0
>>         LoadOptions...: 0
>>         ImageBase.....: 3D650000
>>         ImageSize.....: 5540
>>         CodeType......: EfiRuntimeServicesCode
>>         DataType......: EfiRuntimeServicesData
>>         Unload........: 0
>>
>> Here is the output for the same handle with this series applied:
>>
>>     D3: 3A552218
>>     LoadedImage
>>         Name..........: MicrocodeUpdate
>>         Revision......: 0x00001000
>>         ParentHandle..: 3A666398
>>         SystemTable...: 3D2A8F18
>>         DeviceHandle..: 3B1C8098
>>         FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
>>         OptionsSize...: 0
>>         LoadOptions...: 0
>>         ImageBase.....: 3D650000
>>         ImageSize.....: 5540
>>         CodeType......: EfiRuntimeServicesCode
>>         DataType......: EfiRuntimeServicesData
>>         Unload........: 0
>>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Jaben Carsey <jaben.carsey@intel.com>
>>
>> Jeff Westfahl (3):
>>   ShellPkg/HandleParsingLib: Show LoadedImageProtocol file path as text
>>   ShellPkg/HandleParsingLib: Open LoadedImageProtocol first
>>   ShellPkg/HandleParsingLib: Show LoadedImageProtocol file name
>>
>>  .../UefiHandleParsingLib/UefiHandleParsingLib.c    | 111
>> +++++++++++++++++++--
>>  .../UefiHandleParsingLib/UefiHandleParsingLib.uni  |   4 +-
>>  2 files changed, 104 insertions(+), 11 deletions(-)
>>
>> --
>> 2.7.4
>
>

Re: [PATCH 0/3] ShellPkg/HandleParsingLib: Show LoadedImageProtocol info

[Carsey, Jaben]

I agree with Ray's commentary.  Good positive changes to UEFI Shell! 

For the series.

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> -----Original Message-----
> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> Sent: Friday, May 05, 2017 5:25 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; edk2-devel@lists.01.org; Carsey,
> Jaben <jaben.carsey@intel.com>
> Subject: RE: [PATCH 0/3] ShellPkg/HandleParsingLib: Show
> LoadedImageProtocol info
> Importance: High
> 
> Hi Ray,
> 
> Thank you for pointing out the ECC tool. These changes look good to me.
> 
> Reviewed-by: Jeff Westfahl <jeff.westfahl@ni.com>
> 
> On Fri, 5 May 2017, Ni, Ruiyu wrote:
> 
> > Jeff,
> > Firstly great thanks for filling the gap between EDK shell and UEFI shell.
> > There are many behavior differences between the two,  and EDK Shell
> does carry much more information that
> > are very helpful for developer debugging.
> >
> > Before I check in your patch, could you please kindly review whether the
> additional change is ok to you.
> > One change is to fix the ECC failure: "python
> BaseTools\Source\Python\Ecc\Ecc.py -t
> ShellPkg\Library\UefiHandleParsingLib -s"
> >
> > ShellPkg\Library\UefiHandleParsingLib\UefiHandleParsingLib.c(272):
> [3002]Non-Boolean comparisons should use a compare operator (==, !=, >, <
> >=, <=) Predicate Expression: FileName
> > ShellPkg\Library\UefiHandleParsingLib\UefiHandleParsingLib.c(272):
> [3003]A comparison of any pointer to zero must be done via the NULL type
> Predicate Expression: FileName
> >
> > ====patch content===
> > .../UefiHandleParsingLib/UefiHandleParsingLib.c    | 39 ++++++++------------
> --
> > 1 file changed, 14 insertions(+), 25 deletions(-)
> >
> > diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
> b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
> > index 74934f8617..d3ee068eba 100644
> > --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
> > +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
> > @@ -16,14 +16,14 @@
> >
> > #include "UefiHandleParsingLib.h"
> > #include "IndustryStandard/Acpi10.h"
> > -#include "Pi/PiFirmwareFile.h"
> > -#include "Pi/PiFirmwareVolume.h"
> > -#include "Protocol/FirmwareVolume2.h"
> > +#include <PiDxe.h>
> > +#include <Protocol/FirmwareVolume2.h>
> >
> > EFI_HANDLE        mHandleParsingHiiHandle = NULL;
> > HANDLE_INDEX_LIST mHandleList = {{{NULL,NULL},0,0},0};
> > GUID_INFO_BLOCK   *mGuidList;
> > UINTN             mGuidListCount;
> > +
> > /**
> >   Function to find the file name associated with a LoadedImageProtocol.
> >
> > @@ -44,37 +44,26 @@ FindLoadedImageFileName (
> >   UINTN                          BufferSize;
> >   UINT32                         AuthenticationStatus;
> >
> > -  //
> > -  // Only subtype MEDIA_PIWG_FW_FILE_DP of type
> MEDIA_DEVICE_PATH is supported.
> > -  //
> > -  if (   (LoadedImage == NULL)
> > -      || (LoadedImage->FilePath == NULL)
> > -      || (LoadedImage->FilePath->Type != MEDIA_DEVICE_PATH)
> > -      || (LoadedImage->FilePath->SubType != MEDIA_PIWG_FW_FILE_DP)
> > -   ) {
> > -    return (NULL);
> > +  if ((LoadedImage == NULL) || (LoadedImage->FilePath == NULL)) {
> > +    return NULL;
> >   }
> >
> >   NameGuid =
> EfiGetNameGuidFromFwVolDevicePathNode((MEDIA_FW_VOL_FILEPATH_
> DEVICE_PATH *)LoadedImage->FilePath);
> >
> >   if (NameGuid == NULL) {
> > -    return (NULL);
> > +    return NULL;
> >   }
> >
> >   //
> > -  // Get the FirmwareVolume2Protocol of the device handle that this
> image was
> > -  // loaded from.
> > +  // Get the FirmwareVolume2Protocol of the device handle that this
> image was loaded from.
> >   //
> > -  Status = gBS->HandleProtocol(
> > -    LoadedImage->DeviceHandle,
> > -    &gEfiFirmwareVolume2ProtocolGuid,
> > -    (VOID**)&Fv);
> > +  Status = gBS->HandleProtocol (LoadedImage->DeviceHandle,
> &gEfiFirmwareVolume2ProtocolGuid, (VOID**) &Fv);
> >
> >   //
> >   // FirmwareVolume2Protocol is PI, and is not required to be available.
> >   //
> > -  if (EFI_ERROR(Status)) {
> > -    return (NULL);
> > +  if (EFI_ERROR (Status)) {
> > +    return NULL;
> >   }
> >
> >   //
> > @@ -83,15 +72,15 @@ FindLoadedImageFileName (
> >   Buffer = NULL;
> >   Status = Fv->ReadSection(Fv, NameGuid, EFI_SECTION_USER_INTERFACE,
> 0, &Buffer, &BufferSize, &AuthenticationStatus);
> >
> > -  if (EFI_ERROR(Status)) {
> > -    return (NULL);
> > +  if (EFI_ERROR (Status)) {
> > +    return NULL;
> >   }
> >
> >   //
> >   // ReadSection returns just the section data, without any section header.
> For
> >   // a user interface section, the only data is the file name.
> >   //
> > -  return (Buffer);
> > +  return Buffer;
> > }
> >
> > /**
> > @@ -269,7 +258,7 @@ LoadedImageProtocolDumpInformation(
> >   FileName = FindLoadedImageFileName(LoadedImage);
> >
> >   RetVal = NULL;
> > -  if (FileName) {
> > +  if (FileName != NULL) {
> >     Temp = HiiGetString(mHandleParsingHiiHandle,
> STRING_TOKEN(STR_LI_DUMP_NAME), NULL);
> >
> >     if (Temp != NULL) {
> >
> > Thanks/Ray
> >
> >> -----Original Message-----
> >> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> >> Sent: Friday, May 5, 2017 5:53 AM
> >> To: edk2-devel@lists.01.org
> >> Cc: Jeff Westfahl <jeff.westfahl@ni.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>;
> >> Carsey, Jaben <jaben.carsey@intel.com>
> >> Subject: [PATCH 0/3] ShellPkg/HandleParsingLib: Show
> LoadedImageProtocol
> >> info
> >>
> >> In the old shell, 'dh -v' showed some useful information about the file
> path
> >> associated with a LoadedImageProtocol that is no longer shown in the
> modern
> >> shell.
> >>
> >> For example, with the old shell:
> >>
> >>     Handle D3 (3A552218)
> >>     Image (3A54C918)   File:MicrocodeUpdate
> >>         ParentHandle..: 3A666398
> >>         SystemTable...: 3D2A8F18
> >>         DeviceHandle..: 3B1C8098
> >>         FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
> >>         ImageBase.....: 3D650000 - 3D655540
> >>         ImageSize.....: 5540
> >>         CodeType......: RT_code
> >>         DataType......: RT_data
> >>
> >> compared to the new shell:
> >>
> >>     D3: 3A552218
> >>     LoadedImage
> >>         Revision......: 0x00001000
> >>         ParentHandle..: 3A666398
> >>         SystemTable...: 3D2A8F18
> >>         DeviceHandle..: 3B1C8098
> >>         FilePath......: 3A552298
> >>         OptionsSize...: 0
> >>         LoadOptions...: 0
> >>         ImageBase.....: 3D650000
> >>         ImageSize.....: 5540
> >>         CodeType......: EfiRuntimeServicesCode
> >>         DataType......: EfiRuntimeServicesData
> >>         Unload........: 0
> >>
> >> Here is the output for the same handle with this series applied:
> >>
> >>     D3: 3A552218
> >>     LoadedImage
> >>         Name..........: MicrocodeUpdate
> >>         Revision......: 0x00001000
> >>         ParentHandle..: 3A666398
> >>         SystemTable...: 3D2A8F18
> >>         DeviceHandle..: 3B1C8098
> >>         FilePath......: FvFile(F3331DE6-4A55-44E4-B767-7453F7A1A021)
> >>         OptionsSize...: 0
> >>         LoadOptions...: 0
> >>         ImageBase.....: 3D650000
> >>         ImageSize.....: 5540
> >>         CodeType......: EfiRuntimeServicesCode
> >>         DataType......: EfiRuntimeServicesData
> >>         Unload........: 0
> >>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Jaben Carsey <jaben.carsey@intel.com>
> >>
> >> Jeff Westfahl (3):
> >>   ShellPkg/HandleParsingLib: Show LoadedImageProtocol file path as text
> >>   ShellPkg/HandleParsingLib: Open LoadedImageProtocol first
> >>   ShellPkg/HandleParsingLib: Show LoadedImageProtocol file name
> >>
> >>  .../UefiHandleParsingLib/UefiHandleParsingLib.c    | 111
> >> +++++++++++++++++++--
> >>  .../UefiHandleParsingLib/UefiHandleParsingLib.uni  |   4 +-
> >>  2 files changed, 104 insertions(+), 11 deletions(-)
> >>
> >> --
> >> 2.7.4
> >
> >