public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Carsey, Jaben" <jaben.carsey@intel.com>
To: "Ni, Ruiyu" <ruiyu.ni@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Chen, Chen A" <chen.a.chen@intel.com>,
	"Carsey, Jaben" <jaben.carsey@intel.com>
Subject: Re: [PATCH 4/5] ShellPkg/Dh: Fix coding style issues
Date: Tue, 10 Jan 2017 16:20:55 +0000	[thread overview]
Message-ID: <CB6E33457884FA40993F35157061515C54B3F963@FMSMSX103.amr.corp.intel.com> (raw)
In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5B8799B9@SHSMSX104.ccr.corp.intel.com>

Oh.

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

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, January 09, 2017 6:25 PM
> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Cc: Chen, Chen A <chen.a.chen@intel.com>
> Subject: RE: [PATCH 4/5] ShellPkg/Dh: Fix coding style issues
> Importance: High
> 
> DisplayDriverModelHandle() always return SUCCESS so we change the return
> type to VOID.
> 
> Thanks/Ray
> 
> > -----Original Message-----
> > From: Carsey, Jaben
> > Sent: Monday, January 9, 2017 11:37 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: Chen, Chen A <chen.a.chen@intel.com>; Carsey, Jaben
> > <jaben.carsey@intel.com>
> > Subject: RE: [PATCH 4/5] ShellPkg/Dh: Fix coding style issues
> >
> > One question inline.
> >
> > > -----Original Message-----
> > > From: Ni, Ruiyu
> > > Sent: Monday, January 9, 2017 1:31 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Chen, Chen A <chen.a.chen@intel.com>; Carsey, Jaben
> > > <jaben.carsey@intel.com>
> > > Subject: [PATCH 4/5] ShellPkg/Dh: Fix coding style issues
> > > Importance: High
> > >
> > > From: Chen A Chen <chen.a.chen@intel.com>
> > >
> > > The change doesn't impact the functionality.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Cc: Jaben Carsey <jaben.carsey@intel.com>
> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> > > Signed-off-by: Chen A Chen <chen.a.chen@intel.com>
> > > ---
> > >  ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c | 119
> > > ++++++++++----------
> > > --
> > >  1 file changed, 54 insertions(+), 65 deletions(-)
> > >
> > > diff --git a/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c
> > > b/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c
> > > index 0ac49e1..2773842 100644
> > > --- a/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c
> > > +++ b/ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c
> > > @@ -642,11 +642,8 @@ DisplayDriverModelHandle (
> > >    @param[in] DriverInfo       TRUE to show all info about the handle.
> > >    @param[in] Multiple         TRUE indicates more than  will be output,
> > >                                FALSE for a single one.
> > > -
> > > -  @retval SHELL_SUCCESS           The operation was successful.
> > > -  @retval SHELL_INVALID_PARAMETER ProtocolName was NULL or
> invalid.
> > >  **/
> > > -SHELL_STATUS
> > > +VOID
> >
> > Is there a reason to change the function to VOID and not add a check for
> the
> > return from the DisplayDriverModelHandle function?
> >
> > >  DoDhByHandle(
> > >    IN CONST EFI_HANDLE TheHandle,
> > >    IN CONST BOOLEAN    Verbose,
> > > @@ -656,10 +653,8 @@ DoDhByHandle(
> > >    IN CONST BOOLEAN    Multiple
> > >    )
> > >  {
> > > -  CHAR16              *ProtocolInfoString;
> > > -  SHELL_STATUS        ShellStatus;
> > > +  CHAR16 *ProtocolInfoString;
> > >
> > > -  ShellStatus         = SHELL_SUCCESS;
> > >    ProtocolInfoString  = NULL;
> > >
> > >    if (!Sfo) {
> > > @@ -672,7 +667,8 @@ DoDhByHandle(
> > >          STRING_TOKEN (STR_DH_OUTPUT),
> > >          gShellDriver1HiiHandle,
> > >          ConvertHandleToHandleIndex(TheHandle),
> > > -        ProtocolInfoString==NULL?L"":ProtocolInfoString);
> > > +        ProtocolInfoString==NULL?L"":ProtocolInfoString
> > > +      );
> > >      } else {
> > >        ProtocolInfoString = GetProtocolInfoString(TheHandle, Language,
> > > L"\r\n", Verbose, TRUE);
> > >        ShellPrintHiiEx(
> > > @@ -683,7 +679,8 @@ DoDhByHandle(
> > >          gShellDriver1HiiHandle,
> > >          ConvertHandleToHandleIndex(TheHandle),
> > >          TheHandle,
> > > -        ProtocolInfoString==NULL?L"":ProtocolInfoString);
> > > +        ProtocolInfoString==NULL?L"":ProtocolInfoString
> > > +      );
> > >      }
> > >
> > >      if (DriverInfo) {
> > > @@ -702,16 +699,13 @@ DoDhByHandle(
> > >          L"ControllerName",
> > >          ConvertHandleToHandleIndex(TheHandle),
> > >          L"DevPath",
> > > -        ProtocolInfoString==NULL?L"":ProtocolInfoString);
> > > -
> > > -
> > > +        ProtocolInfoString==NULL?L"":ProtocolInfoString
> > > +      );
> > >    }
> > >
> > > -
> > >    if (ProtocolInfoString != NULL) {
> > >      FreePool(ProtocolInfoString);
> > >    }
> > > -  return (ShellStatus);
> > >  }
> > >
> > >  /**
> > > @@ -723,8 +717,8 @@ DoDhByHandle(
> > >    @param[in] Language         Language string per UEFI specification.
> > >    @param[in] DriverInfo       TRUE to show all info about the handle.
> > >
> > > -  @retval SHELL_SUCCESS           The operation was successful.
> > > -  @retval SHELL_INVALID_PARAMETER ProtocolName was NULL or
> invalid.
> > > +  @retval SHELL_SUCCESS       The operation was successful.
> > > +  @retval SHELL_ABORTED       The operation was aborted.
> > >  **/
> > >  SHELL_STATUS
> > >  DoDhForHandleList(
> > > @@ -740,15 +734,8 @@ DoDhForHandleList(
> > >
> > >    ShellStatus       = SHELL_SUCCESS;
> > >
> > > -  for (HandleWalker = HandleList ; HandleWalker != NULL &&
> > > *HandleWalker != NULL && ShellStatus == SHELL_SUCCESS;
> > HandleWalker++) {
> > > -    ShellStatus = DoDhByHandle(
> > > -          *HandleWalker,
> > > -          Verbose,
> > > -          Sfo,
> > > -          Language,
> > > -          DriverInfo,
> > > -          TRUE
> > > -         );
> > > +  for ( HandleWalker = HandleList; HandleWalker != NULL &&
> > > + *HandleWalker !=
> > > NULL; HandleWalker++ ) {
> > > +    DoDhByHandle (*HandleWalker, Verbose, Sfo, Language, DriverInfo,
> > > + TRUE);
> > >      if (ShellGetExecutionBreakFlag ()) {
> > >        ShellStatus = SHELL_ABORTED;
> > >        break;
> > > @@ -862,10 +849,10 @@ ShellCommandRunDh (
> > >    SHELL_STATUS        ShellStatus;
> > >    CHAR8               *Language;
> > >    CONST CHAR16        *Lang;
> > > -  CONST CHAR16        *Temp2;
> > > -  BOOLEAN             SfoMode;
> > > -  BOOLEAN             FlagD;
> > > -  BOOLEAN             Verbose;
> > > +  CONST CHAR16        *RawValue;
> > > +  BOOLEAN             SfoFlag;
> > > +  BOOLEAN             DriverFlag;
> > > +  BOOLEAN             VerboseFlag;
> > >    UINT64              Intermediate;
> > >
> > >    ShellStatus         = SHELL_SUCCESS;
> > > @@ -900,30 +887,32 @@ ShellCommandRunDh (
> > >        return (SHELL_INVALID_PARAMETER);
> > >      }
> > >
> > > -    Lang = ShellCommandLineGetValue(Package, L"-l");
> > > -    if (Lang != NULL) {
> > > -      Language = AllocateZeroPool(StrSize(Lang));
> > > -      AsciiSPrint(Language, StrSize(Lang), "%S", Lang);
> > > -    } else if (!ShellCommandLineGetFlag(Package, L"-l")){
> > > +    if (ShellCommandLineGetFlag(Package, L"-l")) {
> > > +      Lang = ShellCommandLineGetValue(Package, L"-l");
> > > +      if (Lang != NULL) {
> > > +        Language = AllocateZeroPool(StrSize(Lang));
> > > +        AsciiSPrint(Language, StrSize(Lang), "%S", Lang);
> > > +      } else {
> > > +        ASSERT(Language == NULL);
> > > +        ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN(STR_GEN_NO_VALUE),
> > > gShellDriver1HiiHandle, L"dh", L"-l");
> > > +        ShellCommandLineFreeVarList(Package);
> > > +        return (SHELL_INVALID_PARAMETER);
> > > +      }
> > > +    } else {
> > >        Language = AllocateZeroPool(10);
> > >        AsciiSPrint(Language, 10, "en-us");
> > > -    } else {
> > > -      ASSERT(Language == NULL);
> > > -      ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_VALUE),
> > > gShellDriver1HiiHandle, L"dh",  L"-l");
> > > -      ShellCommandLineFreeVarList (Package);
> > > -      return (SHELL_INVALID_PARAMETER);
> > >      }
> > >
> > > -    SfoMode = ShellCommandLineGetFlag(Package, L"-sfo");
> > > -    FlagD   = ShellCommandLineGetFlag(Package, L"-d");
> > > -    Verbose = (BOOLEAN)(ShellCommandLineGetFlag(Package, L"-v") ||
> > > ShellCommandLineGetFlag(Package, L"-verbose"));
> > > +    SfoFlag     = ShellCommandLineGetFlag (Package, L"-sfo");
> > > +    DriverFlag  = ShellCommandLineGetFlag (Package, L"-d");
> > > +    VerboseFlag = (BOOLEAN)(ShellCommandLineGetFlag (Package, L"-
> v")
> > > + ||
> > > ShellCommandLineGetFlag (Package, L"-verbose"));
> > >
> > > -    if (ShellCommandLineGetFlag(Package, L"-p")) {
> > > -      if (ShellCommandLineGetCount(Package) > 1) {
> > > -        ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
> (STR_GEN_TOO_MANY),
> > > gShellDriver1HiiHandle, L"dh");
> > > +    if (ShellCommandLineGetFlag (Package, L"-p")) {
> > > +      if (ShellCommandLineGetCount (Package) > 1) {
> > > +        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
> > > + (STR_GEN_TOO_MANY),
> > > gShellDriver1HiiHandle, L"dh");
> > >          ShellStatus = SHELL_INVALID_PARAMETER;
> > >        } else if (ShellCommandLineGetValue(Package, L"-p") == NULL) {
> > > -        ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_VALUE),
> > > gShellDriver1HiiHandle, L"dh",  L"-p");
> > > +        ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
> > > + (STR_GEN_NO_VALUE),
> > > gShellDriver1HiiHandle, L"dh",  L"-p");
> > >          ShellStatus = SHELL_INVALID_PARAMETER;
> > >        } else {
> > >          //
> > > @@ -931,41 +920,41 @@ ShellCommandRunDh (
> > >          //
> > >          ShellStatus = DoDhByProtocol(
> > >            ShellCommandLineGetValue(Package, L"-p"),
> > > -          Verbose,
> > > -          SfoMode,
> > > -          Lang==NULL?NULL:Language,
> > > -          FlagD
> > > -         );
> > > +          VerboseFlag,
> > > +          SfoFlag,
> > > +          Language,
> > > +          DriverFlag
> > > +        );
> > >        }
> > >      } else {
> > > -      Temp2 = ShellCommandLineGetRawValue(Package, 1);
> > > -      if (Temp2 == NULL) {
> > > +      RawValue = ShellCommandLineGetRawValue(Package, 1);
> > > +      if (RawValue == NULL) {
> > >          //
> > >          // Print everything
> > >          //
> > >          ShellStatus = DoDhForAll(
> > > -          SfoMode,
> > > -          Verbose,
> > > -          Lang==NULL?NULL:Language,
> > > -          FlagD
> > > +          SfoFlag,
> > > +          VerboseFlag,
> > > +          Language,
> > > +          DriverFlag
> > >           );
> > >        } else {
> > > -        Status = ShellConvertStringToUint64(Temp2, &Intermediate, TRUE,
> > FALSE);
> > > +        Status = ShellConvertStringToUint64(RawValue, &Intermediate,
> > > + TRUE,
> > > FALSE);
> > >          if (EFI_ERROR(Status) ||
> > > ConvertHandleIndexToHandle((UINTN)Intermediate) == NULL) {
> > > -          ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
> (STR_GEN_INV_HANDLE),
> > > gShellDriver1HiiHandle, L"dh", Temp2);
> > > +          ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN
> > > + (STR_GEN_INV_HANDLE),
> > > gShellDriver1HiiHandle, L"dh", RawValue);
> > >            ShellStatus = SHELL_INVALID_PARAMETER;
> > >          } else {
> > >            //
> > >            // print 1 handle
> > >            //
> > > -          ShellStatus = DoDhByHandle(
> > > +          DoDhByHandle(
> > >              ConvertHandleIndexToHandle((UINTN)Intermediate),
> > > -            Verbose,
> > > -            SfoMode,
> > > -            Lang==NULL?NULL:Language,
> > > -            FlagD,
> > > +            VerboseFlag,
> > > +            SfoFlag,
> > > +            Language,
> > > +            DriverFlag,
> > >              FALSE
> > > -           );
> > > +          );
> > >          }
> > >        }
> > >      }
> > > --
> > > 2.9.0.windows.1



  reply	other threads:[~2017-01-10 16:20 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-09  9:30 [PATCH 0/5] Change "dh" to support dump from GUID and "decode" parameter Ruiyu Ni
2017-01-09  9:30 ` [PATCH 1/5] ShellPkg/HandleParsingLib: Rename global variables Ruiyu Ni
2017-01-09  9:30 ` [PATCH 2/5] ShellPkg/HandleParsingLib: Return NULL name for unknown GUID Ruiyu Ni
2017-01-09  9:30 ` [PATCH 3/5] ShellPkg/HandleParsingLib: Add new API GetAllMappingGuids Ruiyu Ni
2017-01-09  9:30 ` [PATCH 4/5] ShellPkg/Dh: Fix coding style issues Ruiyu Ni
2017-01-09 15:36   ` Carsey, Jaben
2017-01-10  2:24     ` Ni, Ruiyu
2017-01-10 16:20       ` Carsey, Jaben [this message]
2017-01-09  9:30 ` [PATCH 5/5] ShellPkg/dh: Support dump from GUID and "decode" parameter Ruiyu Ni
2017-01-09 15:40 ` [PATCH 0/5] Change "dh" to support " Carsey, Jaben
2017-01-10  2:23   ` Ni, Ruiyu
2017-01-10 21:43     ` Carsey, Jaben

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CB6E33457884FA40993F35157061515C54B3F963@FMSMSX103.amr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox