From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E008621B02822 for ; Thu, 9 Aug 2018 09:17:10 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 381B0402178A; Thu, 9 Aug 2018 16:17:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-52.rdu2.redhat.com [10.10.121.52]) by smtp.corp.redhat.com (Postfix) with ESMTP id BD3412166BA2; Thu, 9 Aug 2018 16:17:08 +0000 (UTC) To: Jim.Dailey@dell.com, shenglei.zhang@intel.com Cc: jaben.carsey@intel.com, ruiyu.ni@intel.com, edk2-devel@lists.01.org References: <20180809054155.30244-1-shenglei.zhang@intel.com> <809ce67145704edeb41da02b7e5bdce5@ausx13mps335.AMER.DELL.COM> From: Laszlo Ersek Message-ID: Date: Thu, 9 Aug 2018 18:17:08 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <809ce67145704edeb41da02b7e5bdce5@ausx13mps335.AMER.DELL.COM> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 09 Aug 2018 16:17:10 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 09 Aug 2018 16:17:10 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] ShellPkg Shell: Remove redundant functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Aug 2018 16:17:11 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/09/18 17:41, Jim.Dailey@dell.com wrote: > The InvalidChars[] array is only used in function IsValidCommandName(). > The array should be deleted also, I think. I've pointed out a similar continuation for another patch: http://mid.mail-archive.com/141a93ac-e2e0-0987-82ee-3fe4533e84a3@redhat.com However, as far as I understand, these BZs mainly focus on the uncalled functions that were revealed / flagged by a "UEFI-aware compiler". I think it's fine to clean up those functions first. If some global variables become practically unused afterwards, I think it's OK to split such further cleanup to different BZs, or at least to different patches. IOW, I think pointing out such loose ends is very valuable, but they shouldn't block the current patch set(s). Thanks! Laszlo > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of shenglei > Sent: Thursday, August 9, 2018 12:42 AM > To: edk2-devel@lists.01.org > Cc: Jaben Carsey; Ruiyu Ni > Subject: [edk2] [PATCH] ShellPkg Shell: Remove redundant functions > > The redundant functions which are never called have been > removed. They are InternalShellProtocolDebugPrintMessage, > UpdateFileName,RemoveFileTag and IsValidCommandName. > https://bugzilla.tianocore.org/show_bug.cgi?id=1066 > > Cc: Jaben Carsey > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: shenglei > --- > ShellPkg/Application/Shell/Shell.c | 29 ------- > ShellPkg/Application/Shell/Shell.h | 13 --- > .../Shell/ShellParametersProtocol.c | 24 ------ > ShellPkg/Application/Shell/ShellProtocol.c | 81 +------------------ > 4 files changed, 1 insertion(+), 146 deletions(-) > > diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c > index 47ae3c373c..397cfd1994 100644 > --- a/ShellPkg/Application/Shell/Shell.c > +++ b/ShellPkg/Application/Shell/Shell.c > @@ -2752,35 +2752,6 @@ RunCommand( > > > STATIC CONST UINT16 InvalidChars[] = {L'*', L'?', L'<', L'>', L'\\', L'/', L'\"', 0x0001, 0x0002}; > -/** > - Function determines if the CommandName COULD be a valid command. It does not determine whether > - this is a valid command. It only checks for invalid characters. > - > - @param[in] CommandName The name to check > - > - @retval TRUE CommandName could be a command name > - @retval FALSE CommandName could not be a valid command name > -**/ > -BOOLEAN > -IsValidCommandName( > - IN CONST CHAR16 *CommandName > - ) > -{ > - UINTN Count; > - if (CommandName == NULL) { > - ASSERT(FALSE); > - return (FALSE); > - } > - for ( Count = 0 > - ; Count < sizeof(InvalidChars) / sizeof(InvalidChars[0]) > - ; Count++ > - ){ > - if (ScanMem16(CommandName, StrSize(CommandName), InvalidChars[Count]) != NULL) { > - return (FALSE); > - } > - } > - return (TRUE); > -} > > /** > Function to process a NSH script file via SHELL_FILE_HANDLE. > diff --git a/ShellPkg/Application/Shell/Shell.h b/ShellPkg/Application/Shell/Shell.h > index 69b19c6a2d..bad8f08d47 100644 > --- a/ShellPkg/Application/Shell/Shell.h > +++ b/ShellPkg/Application/Shell/Shell.h > @@ -309,19 +309,6 @@ RunShellCommand( > OUT EFI_STATUS *CommandStatus > ); > > -/** > - Function determines if the CommandName COULD be a valid command. It does not determine whether > - this is a valid command. It only checks for invalid characters. > - > - @param[in] CommandName The name to check > - > - @retval TRUE CommandName could be a command name > - @retval FALSE CommandName could not be a valid command name > -**/ > -BOOLEAN > -IsValidCommandName( > - IN CONST CHAR16 *CommandName > - ); > > /** > Function to process a NSH script file via SHELL_FILE_HANDLE. > diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c b/ShellPkg/Application/Shell/ShellParametersProtocol.c > index 90889a3725..a21c690518 100644 > --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c > +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c > @@ -626,30 +626,6 @@ FixVarName ( > return (FixFileName(Copy)); > } > > -/** > - Remove the unicode file tag from the begining of the file buffer since that will not be > - used by StdIn. > - > - @param[in] Handle Pointer to the handle of the file to be processed. > - > - @retval EFI_SUCCESS The unicode file tag has been moved successfully. > -**/ > -EFI_STATUS > -RemoveFileTag( > - IN SHELL_FILE_HANDLE *Handle > - ) > -{ > - UINTN CharSize; > - CHAR16 CharBuffer; > - > - CharSize = sizeof(CHAR16); > - CharBuffer = 0; > - gEfiShellProtocol->ReadFile(*Handle, &CharSize, &CharBuffer); > - if (CharBuffer != gUnicodeFileTag) { > - gEfiShellProtocol->SetFilePosition(*Handle, 0); > - } > - return (EFI_SUCCESS); > -} > > /** > Write the unicode file tag to the specified file. > diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c > index f2ca2029e3..8cf924b384 100644 > --- a/ShellPkg/Application/Shell/ShellProtocol.c > +++ b/ShellPkg/Application/Shell/ShellProtocol.c > @@ -98,40 +98,6 @@ InternalShellProtocolIsSimpleFileSystemPresent( > return (FALSE); > } > > -/** > - Internal worker debug helper function to print out maps as they are added. > - > - @param[in] Mapping string mapping that has been added > - @param[in] DevicePath pointer to device path that has been mapped. > - > - @retval EFI_SUCCESS the operation was successful. > - @return other an error ocurred > - > - @sa LocateHandle > - @sa OpenProtocol > -**/ > -EFI_STATUS > -InternalShellProtocolDebugPrintMessage ( > - IN CONST CHAR16 *Mapping, > - IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath > - ) > -{ > - EFI_STATUS Status; > - CHAR16 *Temp; > - > - Status = EFI_SUCCESS; > - DEBUG_CODE_BEGIN(); > - > - if (Mapping != NULL) { > - DEBUG((EFI_D_INFO, "Added new map item:\"%S\"\r\n", Mapping)); > - } > - Temp = ConvertDevicePathToText(DevicePath, TRUE, TRUE); > - DEBUG((EFI_D_INFO, "DevicePath: %S\r\n", Temp)); > - FreePool(Temp); > - > - DEBUG_CODE_END(); > - return (Status); > -} > > /** > This function creates a mapping for a device path. > @@ -1333,7 +1299,7 @@ EfiShellOpenFileByName( > // We are opening a regular file. > // > DevicePath = EfiShellGetDevicePathFromFilePath(FileName); > -// DEBUG_CODE(InternalShellProtocolDebugPrintMessage (NULL, DevicePath);); > + > if (DevicePath == NULL) { > return (EFI_NOT_FOUND); > } > @@ -2261,52 +2227,7 @@ EfiShellGetGuidName( > return (EFI_SUCCESS); > } > > -/** > - Updates a file name to be preceeded by the mapped drive name > - > - @param[in] BasePath the Mapped drive name to prepend > - @param[in, out] Path pointer to pointer to the file name to update. > - > - @retval EFI_SUCCESS > - @retval EFI_OUT_OF_RESOURCES > -**/ > -EFI_STATUS > -UpdateFileName( > - IN CONST CHAR16 *BasePath, > - IN OUT CHAR16 **Path > - ) > -{ > - CHAR16 *Path2; > - UINTN Path2Size; > - > - Path2Size = 0; > - Path2 = NULL; > - > - ASSERT(Path != NULL); > - ASSERT(*Path != NULL); > - ASSERT(BasePath != NULL); > - > - // > - // convert a local path to an absolute path > - // > - if (StrStr(*Path, L":") == NULL) { > - ASSERT((Path2 == NULL && Path2Size == 0) || (Path2 != NULL)); > - StrnCatGrow(&Path2, &Path2Size, BasePath, 0); > - if (Path2 == NULL) { > - return (EFI_OUT_OF_RESOURCES); > - } > - ASSERT((Path2 == NULL && Path2Size == 0) || (Path2 != NULL)); > - StrnCatGrow(&Path2, &Path2Size, (*Path)[0] == L'\\'?(*Path) + 1 :*Path, 0); > - if (Path2 == NULL) { > - return (EFI_OUT_OF_RESOURCES); > - } > - } > - > - FreePool(*Path); > - (*Path) = Path2; > > - return (EFI_SUCCESS); > -} > > /** > If FileHandle is a directory then the function reads from FileHandle and reads in >