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

["Carsey, Jaben" <jaben.carsey@intel.com>]

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
> >
> >