From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=jaben.carsey@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 07A4B2119BAFD for ; Thu, 10 Jan 2019 07:19:16 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jan 2019 07:19:16 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,461,1539673200"; d="scan'208";a="105590990" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga007.jf.intel.com with ESMTP; 10 Jan 2019 07:19:16 -0800 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 10 Jan 2019 07:19:16 -0800 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.197]) by fmsmsx156.amr.corp.intel.com ([169.254.13.179]) with mapi id 14.03.0415.000; Thu, 10 Jan 2019 07:19:15 -0800 From: "Carsey, Jaben" To: "Li, Songpeng" , "edk2-devel@lists.01.org" CC: "Ni, Ray" , "Wu, Jiaxin" Thread-Topic: [PATCH v2] ShellPkg/TftpDynamicCommand: Change file writing method in tftp Thread-Index: AQHUqI/aZJrqcPgzvkm9pSkjnV6DWqWonovA Date: Thu, 10 Jan 2019 15:19:15 +0000 Message-ID: References: <20190110025415.41056-1-songpeng.li@intel.com> In-Reply-To: <20190110025415.41056-1-songpeng.li@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzRlMGY4NjAtMjBkMi00NDFiLWFhODgtNjQyMDY1NzFlOTYzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiN3Y3VjZcL1Bla0dFZlJMeTA1S3p3emRkUWVsNTdNRDE5MTVqdGJNQVVKMEFXb09Wb2VHNjlpYTZyZWpOSU5rRTAifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Subject: Re: [PATCH v2] 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 15:19:17 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jaben Carsey > -----Original Message----- > From: Li, Songpeng > Sent: Wednesday, January 09, 2019 6:54 PM > To: edk2-devel@lists.01.org > Cc: Carsey, Jaben ; Ni, Ray ; > Wu, Jiaxin > Subject: [PATCH v2] ShellPkg/TftpDynamicCommand: Change file writing > method in tftp > Importance: High >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1433 >=20 > v2: Remove an unused variable. >=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 | 154 ++++++++- > ----------- > 1 file changed, 64 insertions(+), 90 deletions(-) >=20 > diff --git a/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > b/ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c > index ed081b5bad7c..ba753a279b00 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,6 @@ 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 +307,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 +382,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 +391,7 @@ RunTftp ( > break; > } > } > - LocalFilePath =3D Walker + 1; > + mLocalFilePath =3D Walker + 1; > } >=20 > // > @@ -492,7 +489,6 @@ RunTftp ( > (NicNumber < HandleCount) && (ShellStatus !=3D SHELL_SUCCESS); > NicNumber++) { > ControllerHandle =3D Handles[NicNumber]; > - Data =3D NULL; >=20 > Status =3D GetNicName (ControllerHandle, NicNumber, NicName); > if (EFI_ERROR (Status)) { > @@ -543,7 +539,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 +548,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 +871,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 +886,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 +897,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 +907,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 +935,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 +978,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 +1009,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 +1017,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