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
next prev parent 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