From: Songpeng Li <songpeng.li@intel.com>
To: edk2-devel@lists.01.org
Cc: Jaben Carsey <jaben.carsey@intel.com>,
Ruiyu Ni <ruiyu.ni@intel.com>, Wu Jiaxin <jiaxin.wu@intel.com>
Subject: [PATCH v1] ShellPkg/TftpDynamicCommand: Change file writing method in tftp
Date: Wed, 9 Jan 2019 16:42:01 +0800 [thread overview]
Message-ID: <20190109084201.41188-1-songpeng.li@intel.com> (raw)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1433
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.
This patch update the code to open and close file inside
DownloadFile(), and save each packet to file within callback
function CheckPacket().
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.
Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Wu Jiaxin <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Songpeng Li <songpeng.li@intel.com>
---
ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c | 152 +++++++++-----------
1 file changed, 64 insertions(+), 88 deletions(-)
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[] = L"[
// (TFTP_PROGRESS_MESSAGE_SIZE-1) '\b'
STATIC CONST CHAR16 mTftpProgressDelete[] = 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";
+// 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
@@ -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.
@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
);
/**
@@ -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;
@@ -309,7 +308,6 @@ RunTftp (
AsciiRemoteFilePath = NULL;
Handles = NULL;
FileSize = 0;
- DataSize = 0;
BlockSize = MTFTP_DEFAULT_BLKSIZE;
WindowSize = MTFTP_DEFAULT_WINDOWSIZE;
@@ -385,7 +383,7 @@ RunTftp (
UnicodeStrToAsciiStrS (RemoteFilePath, AsciiRemoteFilePath, FilePathSize);
if (ParamCount == 4) {
- LocalFilePath = ShellCommandLineGetRawValue (CheckPackage, 3);
+ mLocalFilePath = ShellCommandLineGetRawValue (CheckPackage, 3);
} else {
Walker = RemoteFilePath + StrLen (RemoteFilePath);
while ((--Walker) >= RemoteFilePath) {
@@ -394,7 +392,7 @@ RunTftp (
break;
}
}
- LocalFilePath = Walker + 1;
+ mLocalFilePath = Walker + 1;
}
//
@@ -543,7 +541,7 @@ RunTftp (
goto NextHandle;
}
- Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath, FileSize, BlockSize, WindowSize, &Data);
+ Status = 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;
}
- DataSize = FileSize;
-
- if (!EFI_ERROR (ShellFileExists (LocalFilePath))) {
- ShellDeleteFileByName (LocalFilePath);
- }
-
- Status = 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 = ShellWriteFile (FileHandle, &FileSize, Data);
- if (!EFI_ERROR (Status)) {
- ShellStatus = SHELL_SUCCESS;
- } else {
- ShellPrintHiiEx (
- -1, -1, NULL, STRING_TOKEN (STR_TFTP_ERR_WRITE),
- mTftpHiiHandle, LocalFilePath, Status
- );
- }
- ShellCloseFile (&FileHandle);
-
NextHandle:
- if (Data != 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.
@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 (
ZeroMem (&Mtftp4Token, sizeof (EFI_MTFTP4_TOKEN));
- // 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 = gBS->AllocatePages (
- AllocateAnyPages,
- EfiBootServicesCode,
- EFI_SIZE_TO_PAGES (FileSize),
- &PagesAddress
- );
- if (EFI_ERROR (Status)) {
- return Status;
- }
-
- Buffer = (VOID*)(UINTN)PagesAddress;
TftpContext = AllocatePool (sizeof (DOWNLOAD_CONTEXT));
if (TftpContext == NULL) {
Status = EFI_OUT_OF_RESOURCES;
@@ -970,8 +909,6 @@ DownloadFile (
TftpContext->LastReportedNbOfBytes = 0;
Mtftp4Token.Filename = (UINT8*)AsciiFilePath;
- Mtftp4Token.BufferSize = FileSize;
- Mtftp4Token.Buffer = Buffer;
Mtftp4Token.CheckPacket = CheckPacket;
Mtftp4Token.Context = (VOID*)TftpContext;
Mtftp4Token.OptionCount = 0;
@@ -1000,14 +937,41 @@ DownloadFile (
mTftpHiiHandle, FilePath
);
+ //
+ // OPEN FILE
+ //
+ if (!EFI_ERROR (ShellFileExists (mLocalFilePath))) {
+ ShellDeleteFileByName (mLocalFilePath);
+ }
+
+ Status = 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 = Mtftp4->ReadFile (Mtftp4, &Mtftp4Token);
ShellPrintHiiEx (
-1, -1, NULL, STRING_TOKEN (STR_GEN_CRLF),
mTftpHiiHandle
);
-Error :
+ //
+ // CLOSE FILE
+ //
+ ShellCloseFile (&mFileHandle);
+Error :
if (TftpContext != NULL) {
FreePool (TftpContext);
}
@@ -1016,14 +980,7 @@ Error :
FreePool (Mtftp4Token.OptionList);
}
- if (EFI_ERROR (Status)) {
- gBS->FreePages (PagesAddress, EFI_SIZE_TO_PAGES (FileSize));
- return Status;
- }
-
- *Data = Buffer;
-
- return EFI_SUCCESS;
+ return Status;
}
/**
@@ -1054,6 +1011,7 @@ CheckPacket (
UINTN Index;
UINTN LastStep;
UINTN Step;
+ UINTN DownloadLen;
EFI_STATUS Status;
if ((NTOHS (Packet->OpCode)) != EFI_MTFTP4_OPCODE_DATA) {
@@ -1061,17 +1019,35 @@ CheckPacket (
}
Context = (DOWNLOAD_CONTEXT*)Token->Context;
- if (Context->DownloadedNbOfBytes == 0) {
- ShellPrintEx (-1, -1, L"%s 0 Kb", mTftpProgressFrame);
- }
//
// The data in the packet are prepended with two UINT16 :
// . OpCode = EFI_MTFTP4_OPCODE_DATA
// . Block = the number of this block of data
//
- Context->DownloadedNbOfBytes += PacketLen - sizeof (Packet->OpCode)
- - sizeof (Packet->Data.Block);
+ DownloadLen = (UINTN)PacketLen - sizeof (Packet->OpCode) - sizeof (Packet->Data.Block);
+
+ ShellSetFilePosition(mFileHandle, Context->DownloadedNbOfBytes);
+ Status = ShellWriteFile (mFileHandle, &DownloadLen, Packet->Data.Data);
+ 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 == 0) {
+ ShellPrintEx (-1, -1, L"%s 0 Kb", mTftpProgressFrame);
+ }
+
+ Context->DownloadedNbOfBytes += DownloadLen;
NbOfKb = Context->DownloadedNbOfBytes / 1024;
Progress[0] = L'\0';
--
2.18.0.windows.1
next reply other threads:[~2019-01-09 8:42 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-09 8:42 Songpeng Li [this message]
2019-01-10 1:27 ` [PATCH v1] ShellPkg/TftpDynamicCommand: Change file writing method in tftp Wu, Jiaxin
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=20190109084201.41188-1-songpeng.li@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