From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jiaxin.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 3790F211AEA76 for ; Wed, 9 Jan 2019 17:27:59 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jan 2019 17:27:59 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,459,1539673200"; d="scan'208";a="116914306" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga003.jf.intel.com with ESMTP; 09 Jan 2019 17:27:59 -0800 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 9 Jan 2019 17:27:58 -0800 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 9 Jan 2019 17:27:58 -0800 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.239]) by shsmsx102.ccr.corp.intel.com ([169.254.2.63]) with mapi id 14.03.0415.000; Thu, 10 Jan 2019 09:27:55 +0800 From: "Wu, Jiaxin" To: "Li, Songpeng" , "edk2-devel@lists.01.org" CC: "Carsey, Jaben" , "Ni, Ray" Thread-Topic: [PATCH v1] ShellPkg/TftpDynamicCommand: Change file writing method in tftp Thread-Index: AQHUp/dYuru9bCgiO0O8K1zrz9mXvaWnt06Q Date: Thu, 10 Jan 2019 01:27:55 +0000 Message-ID: <895558F6EA4E3B41AC93A00D163B727416EC3830@SHSMSX107.ccr.corp.intel.com> References: <20190109084201.41188-1-songpeng.li@intel.com> In-Reply-To: <20190109084201.41188-1-songpeng.li@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiY2ZhMDFlNTctZDE1Ni00MTAzLWJlZWMtOGEyNDAzYmRiMTUwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidjBkWHBLSFZGcHVsS09KNWFzU0lGZmtrdWJPUllReGNEaGVBekkwcEhwN0xZRTk3YVFjOUhCNHFWQ1d6TTZkZSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v1] ShellPkg/TftpDynamicCommand: Change file writing method in tftp X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Jan 2019 01:28:00 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Wu Jiaxin Thanks, Jiaxin > -----Original Message----- > From: Li, Songpeng > Sent: Wednesday, January 9, 2019 4:42 PM > To: edk2-devel@lists.01.org > Cc: Carsey, Jaben ; Ni, Ray ; > Wu, Jiaxin > Subject: [PATCH v1] ShellPkg/TftpDynamicCommand: Change file writing > method in tftp >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1433 >=20 > Current logic of shell tftp download was writing file after tftp > download finished, when the file is large, it looks like the shell > tftp command hanged after download was finished. To improve > end-user experience, the solution is using split file writing > instead. >=20 > This patch update the code to open and close file inside > DownloadFile(), and save each packet to file within callback > function CheckPacket(). >=20 > Since AllocatePage() is no-longer needed, This patch can also > remove the memory limitation. The download file can be larger > than system free memory now. >=20 > Cc: Jaben Carsey > Cc: Ruiyu Ni > Cc: Wu Jiaxin > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Songpeng Li > --- > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | 152 > +++++++++----------- > 1 file changed, 64 insertions(+), 88 deletions(-) >=20 > diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > index ed081b5bad7c..a53fe16f0683 100644 > --- a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > +++ b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > @@ -41,6 +41,12 @@ STATIC CONST CHAR16 mTftpProgressFrame[] =3D L"[ > // (TFTP_PROGRESS_MESSAGE_SIZE-1) '\b' > STATIC CONST CHAR16 mTftpProgressDelete[] =3D > L"\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\ > b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"; >=20 > +// Local File Handle > +SHELL_FILE_HANDLE mFileHandle; > + > +// Path of the local file, Unicode encoded > +CONST CHAR16 *mLocalFilePath; > + > /** > Check and convert the UINT16 option values of the 'tftp' command >=20 > @@ -166,9 +172,6 @@ GetFileSize ( > @param[in] FileSize Size of the file in number of bytes > @param[in] BlockSize Value of the TFTP blksize option > @param[in] WindowSize Value of the TFTP window size option > - @param[out] Data Address where to store the address of the = buffer > - where the data of the file were downloaded= in > - case of success. >=20 > @retval EFI_SUCCESS The file was downloaded. > @retval EFI_OUT_OF_RESOURCES A memory allocation failed. > @@ -184,8 +187,7 @@ DownloadFile ( > IN CONST CHAR8 *AsciiFilePath, > IN UINTN FileSize, > IN UINT16 BlockSize, > - IN UINT16 WindowSize, > - OUT VOID **Data > + IN UINT16 WindowSize > ); >=20 > /** > @@ -287,7 +289,6 @@ RunTftp ( > CHAR8 *AsciiRemoteFilePath; > UINTN FilePathSize; > CONST CHAR16 *Walker; > - CONST CHAR16 *LocalFilePath; > EFI_MTFTP4_CONFIG_DATA Mtftp4ConfigData; > EFI_HANDLE *Handles; > UINTN HandleCount; > @@ -297,9 +298,7 @@ RunTftp ( > EFI_HANDLE Mtftp4ChildHandle; > EFI_MTFTP4_PROTOCOL *Mtftp4; > UINTN FileSize; > - UINTN DataSize; > VOID *Data; > - SHELL_FILE_HANDLE FileHandle; > UINT16 BlockSize; > UINT16 WindowSize; >=20 > @@ -309,7 +308,6 @@ RunTftp ( > AsciiRemoteFilePath =3D NULL; > Handles =3D NULL; > FileSize =3D 0; > - DataSize =3D 0; > BlockSize =3D MTFTP_DEFAULT_BLKSIZE; > WindowSize =3D MTFTP_DEFAULT_WINDOWSIZE; >=20 > @@ -385,7 +383,7 @@ RunTftp ( > UnicodeStrToAsciiStrS (RemoteFilePath, AsciiRemoteFilePath, FilePathSi= ze); >=20 > if (ParamCount =3D=3D 4) { > - LocalFilePath =3D ShellCommandLineGetRawValue (CheckPackage, 3); > + mLocalFilePath =3D ShellCommandLineGetRawValue (CheckPackage, 3); > } else { > Walker =3D RemoteFilePath + StrLen (RemoteFilePath); > while ((--Walker) >=3D RemoteFilePath) { > @@ -394,7 +392,7 @@ RunTftp ( > break; > } > } > - LocalFilePath =3D Walker + 1; > + mLocalFilePath =3D Walker + 1; > } >=20 > // > @@ -543,7 +541,7 @@ RunTftp ( > goto NextHandle; > } >=20 > - Status =3D DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath= , > FileSize, BlockSize, WindowSize, &Data); > + Status =3D DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath= , > FileSize, BlockSize, WindowSize); > if (EFI_ERROR (Status)) { > ShellPrintHiiEx ( > -1, -1, NULL, STRING_TOKEN (STR_TFTP_ERR_DOWNLOAD), > @@ -552,45 +550,8 @@ RunTftp ( > goto NextHandle; > } >=20 > - DataSize =3D FileSize; > - > - if (!EFI_ERROR (ShellFileExists (LocalFilePath))) { > - ShellDeleteFileByName (LocalFilePath); > - } > - > - Status =3D ShellOpenFileByName ( > - LocalFilePath, > - &FileHandle, > - EFI_FILE_MODE_CREATE | > - EFI_FILE_MODE_WRITE | > - EFI_FILE_MODE_READ, > - 0 > - ); > - if (EFI_ERROR (Status)) { > - ShellPrintHiiEx ( > - -1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL), > - mTftpHiiHandle, L"tftp", LocalFilePath > - ); > - goto NextHandle; > - } > - > - Status =3D ShellWriteFile (FileHandle, &FileSize, Data); > - if (!EFI_ERROR (Status)) { > - ShellStatus =3D SHELL_SUCCESS; > - } else { > - ShellPrintHiiEx ( > - -1, -1, NULL, STRING_TOKEN (STR_TFTP_ERR_WRITE), > - mTftpHiiHandle, LocalFilePath, Status > - ); > - } > - ShellCloseFile (&FileHandle); > - > NextHandle: >=20 > - if (Data !=3D NULL) { > - gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)Data, > EFI_SIZE_TO_PAGES (DataSize)); > - } > - > CloseProtocolAndDestroyServiceChild ( > ControllerHandle, > &gEfiMtftp4ServiceBindingProtocolGuid, > @@ -912,9 +873,6 @@ Error : > @param[in] FileSize Size of the file in number of bytes > @param[in] BlockSize Value of the TFTP blksize option > @param[in] WindowSize Value of the TFTP window size option > - @param[out] Data Address where to store the address of the = buffer > - where the data of the file were downloaded= in > - case of success. >=20 > @retval EFI_SUCCESS The file was downloaded. > @retval EFI_OUT_OF_RESOURCES A memory allocation failed. > @@ -930,13 +888,10 @@ DownloadFile ( > IN CONST CHAR8 *AsciiFilePath, > IN UINTN FileSize, > IN UINT16 BlockSize, > - IN UINT16 WindowSize, > - OUT VOID **Data > + IN UINT16 WindowSize > ) > { > EFI_STATUS Status; > - EFI_PHYSICAL_ADDRESS PagesAddress; > - VOID *Buffer; > DOWNLOAD_CONTEXT *TftpContext; > EFI_MTFTP4_TOKEN Mtftp4Token; > UINT8 BlksizeBuf[10]; > @@ -944,22 +899,6 @@ DownloadFile ( >=20 > ZeroMem (&Mtftp4Token, sizeof (EFI_MTFTP4_TOKEN)); >=20 > - // Downloaded file can be large. BS.AllocatePages() is more faster > - // than AllocatePool() and avoid fragmentation. > - // The downloaded file could be an EFI application. Marking the > - // allocated page as EfiBootServicesCode would allow to execute a > - // potential downloaded EFI application. > - Status =3D gBS->AllocatePages ( > - AllocateAnyPages, > - EfiBootServicesCode, > - EFI_SIZE_TO_PAGES (FileSize), > - &PagesAddress > - ); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - > - Buffer =3D (VOID*)(UINTN)PagesAddress; > TftpContext =3D AllocatePool (sizeof (DOWNLOAD_CONTEXT)); > if (TftpContext =3D=3D NULL) { > Status =3D EFI_OUT_OF_RESOURCES; > @@ -970,8 +909,6 @@ DownloadFile ( > TftpContext->LastReportedNbOfBytes =3D 0; >=20 > Mtftp4Token.Filename =3D (UINT8*)AsciiFilePath; > - Mtftp4Token.BufferSize =3D FileSize; > - Mtftp4Token.Buffer =3D Buffer; > Mtftp4Token.CheckPacket =3D CheckPacket; > Mtftp4Token.Context =3D (VOID*)TftpContext; > Mtftp4Token.OptionCount =3D 0; > @@ -1000,14 +937,41 @@ DownloadFile ( > mTftpHiiHandle, FilePath > ); >=20 > + // > + // OPEN FILE > + // > + if (!EFI_ERROR (ShellFileExists (mLocalFilePath))) { > + ShellDeleteFileByName (mLocalFilePath); > + } > + > + Status =3D ShellOpenFileByName ( > + mLocalFilePath, > + &mFileHandle, > + EFI_FILE_MODE_CREATE | > + EFI_FILE_MODE_WRITE | > + EFI_FILE_MODE_READ, > + 0 > + ); > + if (EFI_ERROR (Status)) { > + ShellPrintHiiEx ( > + -1, -1, NULL, STRING_TOKEN (STR_GEN_FILE_OPEN_FAIL), > + mTftpHiiHandle, L"tftp", mLocalFilePath > + ); > + goto Error; > + } > + > Status =3D Mtftp4->ReadFile (Mtftp4, &Mtftp4Token); > ShellPrintHiiEx ( > -1, -1, NULL, STRING_TOKEN (STR_GEN_CRLF), > mTftpHiiHandle > ); >=20 > -Error : > + // > + // CLOSE FILE > + // > + ShellCloseFile (&mFileHandle); >=20 > +Error : > if (TftpContext !=3D NULL) { > FreePool (TftpContext); > } > @@ -1016,14 +980,7 @@ Error : > FreePool (Mtftp4Token.OptionList); > } >=20 > - if (EFI_ERROR (Status)) { > - gBS->FreePages (PagesAddress, EFI_SIZE_TO_PAGES (FileSize)); > - return Status; > - } > - > - *Data =3D Buffer; > - > - return EFI_SUCCESS; > + return Status; > } >=20 > /** > @@ -1054,6 +1011,7 @@ CheckPacket ( > UINTN Index; > UINTN LastStep; > UINTN Step; > + UINTN DownloadLen; > EFI_STATUS Status; >=20 > if ((NTOHS (Packet->OpCode)) !=3D EFI_MTFTP4_OPCODE_DATA) { > @@ -1061,17 +1019,35 @@ CheckPacket ( > } >=20 > Context =3D (DOWNLOAD_CONTEXT*)Token->Context; > - if (Context->DownloadedNbOfBytes =3D=3D 0) { > - ShellPrintEx (-1, -1, L"%s 0 Kb", mTftpProgressFrame); > - } >=20 > // > // The data in the packet are prepended with two UINT16 : > // . OpCode =3D EFI_MTFTP4_OPCODE_DATA > // . Block =3D the number of this block of data > // > - Context->DownloadedNbOfBytes +=3D PacketLen - sizeof (Packet->OpCode) > - - sizeof (Packet->Data.Block= ); > + DownloadLen =3D (UINTN)PacketLen - sizeof (Packet->OpCode) - sizeof > (Packet->Data.Block); > + > + ShellSetFilePosition(mFileHandle, Context->DownloadedNbOfBytes); > + Status =3D ShellWriteFile (mFileHandle, &DownloadLen, Packet->Data.Dat= a); > + if (EFI_ERROR (Status)) { > + if (Context->DownloadedNbOfBytes > 0) { > + ShellPrintHiiEx ( > + -1, -1, NULL, STRING_TOKEN (STR_GEN_CRLF), > + mTftpHiiHandle > + ); > + } > + ShellPrintHiiEx ( > + -1, -1, NULL, STRING_TOKEN (STR_TFTP_ERR_WRITE), > + mTftpHiiHandle, mLocalFilePath, Status > + ); > + return Status; > + } > + > + if (Context->DownloadedNbOfBytes =3D=3D 0) { > + ShellPrintEx (-1, -1, L"%s 0 Kb", mTftpProgressFrame); > + } > + > + Context->DownloadedNbOfBytes +=3D DownloadLen; > NbOfKb =3D Context->DownloadedNbOfBytes / 1024; >=20 > Progress[0] =3D L'\0'; > -- > 2.18.0.windows.1