From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 3E61C8042C for ; Thu, 16 Mar 2017 18:23:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1489713826; x=1521249826; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=a0+YnLxrrRJtyzgDG7HkMW/vq4qMaR7NNZPo93Lht1k=; b=WrUz/BOb8I1uABhBDM+ooEOQhu1ah707XrIaA7tCL/NTJv6w7yLfiEzQ IRc8TvqAMBFn/AbJIytESfBZqtbxQg==; Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Mar 2017 18:23:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,174,1486454400"; d="scan'208";a="1123820461" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 16 Mar 2017 18:23:45 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 16 Mar 2017 18:23:45 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 16 Mar 2017 18:23:45 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.20]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.132]) with mapi id 14.03.0248.002; Fri, 17 Mar 2017 09:23:42 +0800 From: "Wu, Jiaxin" To: "Carsey, Jaben" , "edk2-devel@lists.01.org" CC: "Ye, Ting" , "Fu, Siyuan" Thread-Topic: [Patch 2/2] ShellPkg/tftp: Add one option for tftp command to specify windowsize Thread-Index: AQHSnjBd819OcCYlNke4vLLqW1uQbqGXHaYAgAEf20A= Date: Fri, 17 Mar 2017 01:23:42 +0000 Message-ID: <895558F6EA4E3B41AC93A00D163B7274162A3E40@SHSMSX103.ccr.corp.intel.com> References: <1489653350-62316-1-git-send-email-jiaxin.wu@intel.com> <1489653350-62316-3-git-send-email-jiaxin.wu@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDk0N2EwYjAtOTdhOC00OGM4LWFkZmEtMWU4ZmFiMDlkY2FiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IjNRaUhEWlRmSVpIWElhd3MxMm9Wd2EwdUJub3FaOFwvUjJlQjBud1g1Z1o0PSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch 2/2] ShellPkg/tftp: Add one option for tftp command to specify windowsize X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Mar 2017 01:23:46 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thanks Jaben, according your comments, I refined the constants definitions. Best Regards! Jiaxin > -----Original Message----- > From: Carsey, Jaben > Sent: Friday, March 17, 2017 12:12 AM > To: Wu, Jiaxin ; edk2-devel@lists.01.org > Cc: Ye, Ting ; Fu, Siyuan ; Carse= y, > Jaben > Subject: RE: [Patch 2/2] ShellPkg/tftp: Add one option for tftp command t= o > specify windowsize >=20 > I'm a little confused by the constants you defined. 2 questions inline... > Maybe we need another #define so we have: the max, the min, and the > default? >=20 > -Jaben >=20 > > -----Original Message----- > > From: Wu, Jiaxin > > Sent: Thursday, March 16, 2017 1:36 AM > > To: edk2-devel@lists.01.org > > Cc: Ye, Ting ; Fu, Siyuan ; > Carsey, > > Jaben ; Wu, Jiaxin > > Subject: [Patch 2/2] ShellPkg/tftp: Add one option for tftp command to > > specify windowsize > > Importance: High > > > > Define -w option for tftp shell command to specify the TFTP windowsize > > option. That will benefit the big file download for tftp server. > > > > Cc: Ye Ting > > Cc: Fu Siyuan > > Cc: Carsey Jaben > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Wu Jiaxin > > --- > > ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c | 60 > > ++++++++++++++++++---- > > .../UefiShellTftpCommandLib.uni | 6 ++- > > 2 files changed, 54 insertions(+), 12 deletions(-) > > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > index 5c50797..26f73ea 100755 > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > @@ -1,10 +1,10 @@ > > /** @file > > The implementation for the 'tftp' Shell command. > > > > Copyright (c) 2015, ARM Ltd. All rights reserved.
> > - Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved. <= BR> > > + Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved. <= BR> > > (C) Copyright 2015 Hewlett Packard Enterprise Development LP
> > > > This program and the accompanying materials > > are licensed and made available under the terms and conditions of th= e > BSD > > License > > which accompanies this distribution. The full text of the license m= ay be > > found at > > @@ -180,10 +180,11 @@ DownloadFile ( > > IN EFI_MTFTP4_PROTOCOL *Mtftp4, > > IN CONST CHAR16 *FilePath, > > IN CONST CHAR8 *AsciiFilePath, > > IN UINTN FileSize, > > IN UINT16 BlockSize, > > + IN UINT16 WindowSize, > > OUT VOID **Data > > ); > > > > /** > > Update the progress of a file download > > @@ -224,10 +225,11 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] =3D > { > > {L"-l", TypeValue}, > > {L"-r", TypeValue}, > > {L"-c", TypeValue}, > > {L"-t", TypeValue}, > > {L"-s", TypeValue}, > > + {L"-w", TypeValue}, > > {NULL , TypeMax} > > }; > > > > /// > > /// The default block size (512) of tftp is defined in the RFC1350. > > @@ -236,11 +238,16 @@ STATIC CONST SHELL_PARAM_ITEM ParamList[] =3D > { > > /// > > /// The valid range of block size option is defined in the RFC2348. > > /// > > #define MTFTP_MIN_BLKSIZE 8 > > #define MTFTP_MAX_BLKSIZE 65464 > > +/// > > +/// The The default windowsize. > > +/// > > +#define MTFTP_DEFAULT_WINDOWSIZE 1 > > > > +#define MTFTP_MAX_WINDOWSIZE 64 > > > > /** > > Function for 'tftp' command. > > > > @param[in] ImageHandle Handle to the Image (NULL if Internal). > > @@ -285,18 +292,20 @@ ShellCommandRunTftp ( > > EFI_MTFTP4_PROTOCOL *Mtftp4; > > UINTN FileSize; > > VOID *Data; > > SHELL_FILE_HANDLE FileHandle; > > UINT16 BlockSize; > > + UINT16 WindowSize; > > > > ShellStatus =3D SHELL_INVALID_PARAMETER; > > ProblemParam =3D NULL; > > NicFound =3D FALSE; > > AsciiRemoteFilePath =3D NULL; > > Handles =3D NULL; > > FileSize =3D 0; > > BlockSize =3D MTFTP_DEFAULT_BLKSIZE; > > + WindowSize =3D 1; >=20 > [Jaben] If the #define is the default, then why do we not initialize the > variable with it? >=20 > > > > // > > // Initialize the Shell library (we must be in non-auto-init...) > > // > > Status =3D ShellInitialize (); > > @@ -432,10 +441,24 @@ ShellCommandRunTftp ( > > ); > > goto Error; > > } > > } > > > > + ValueStr =3D ShellCommandLineGetValue (CheckPackage, L"-w"); > > + if (ValueStr !=3D NULL) { > > + if (!StringToUint16 (ValueStr, &WindowSize)) { > > + goto Error; > > + } > > + if (WindowSize < MTFTP_DEFAULT_WINDOWSIZE || WindowSize > > > MTFTP_MAX_WINDOWSIZE) { >=20 > [Jaben] If the value must not be less that it, it should be labeled mini= mum > and not default. >=20 > > + ShellPrintHiiEx ( > > + -1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV), > > + gShellTftpHiiHandle, L"tftp", ValueStr > > + ); > > + goto Error; > > + } > > + } > > + > > // > > // Locate all MTFTP4 Service Binding protocols > > // > > ShellStatus =3D SHELL_NOT_FOUND; > > Status =3D gBS->LocateHandleBuffer ( > > @@ -506,11 +529,11 @@ ShellCommandRunTftp ( > > gShellTftpHiiHandle, RemoteFilePath, NicName, Status > > ); > > goto NextHandle; > > } > > > > - Status =3D DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePa= th, > > FileSize, BlockSize, &Data); > > + Status =3D DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePa= th, > > FileSize, BlockSize, WindowSize, &Data); > > if (EFI_ERROR (Status)) { > > ShellPrintHiiEx ( > > -1, -1, NULL, STRING_TOKEN (STR_TFTP_ERR_DOWNLOAD), > > gShellTftpHiiHandle, RemoteFilePath, NicName, Status > > ); > > @@ -890,20 +913,21 @@ DownloadFile ( > > IN EFI_MTFTP4_PROTOCOL *Mtftp4, > > IN CONST CHAR16 *FilePath, > > IN CONST CHAR8 *AsciiFilePath, > > IN UINTN FileSize, > > IN UINT16 BlockSize, > > + IN UINT16 WindowSize, > > OUT VOID **Data > > ) > > { > > EFI_STATUS Status; > > EFI_PHYSICAL_ADDRESS PagesAddress; > > VOID *Buffer; > > DOWNLOAD_CONTEXT *TftpContext; > > EFI_MTFTP4_TOKEN Mtftp4Token; > > - EFI_MTFTP4_OPTION ReqOpt; > > - UINT8 OptBuf[10]; > > + UINT8 BlksizeBuf[10]; > > + UINT8 WindowsizeBuf[10]; > > > > // 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 > > @@ -932,17 +956,29 @@ DownloadFile ( > > 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; > > + Mtftp4Token.OptionList =3D AllocatePool (sizeof (EFI_MTFTP4_OPTION)= * > > 2); > > + if (Mtftp4Token.OptionList =3D=3D NULL) { > > + Status =3D EFI_OUT_OF_RESOURCES; > > + goto Error; > > + } > > + > > if (BlockSize !=3D MTFTP_DEFAULT_BLKSIZE) { > > - ReqOpt.OptionStr =3D (UINT8 *) "blksize"; > > - AsciiSPrint ((CHAR8 *)OptBuf, sizeof (OptBuf), "%d", BlockSize); > > - ReqOpt.ValueStr =3D OptBuf; > > + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr =3D > (UINT8 > > *) "blksize"; > > + AsciiSPrint ((CHAR8 *) BlksizeBuf, sizeof (BlksizeBuf), "%d", Bloc= kSize); > > + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr =3D > > BlksizeBuf; > > + Mtftp4Token.OptionCount ++; > > + } > > > > - Mtftp4Token.OptionCount =3D 1; > > - Mtftp4Token.OptionList =3D &ReqOpt; > > + if (WindowSize !=3D MTFTP_DEFAULT_WINDOWSIZE) { > > + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr =3D > (UINT8 > > *) "windowsize"; > > + AsciiSPrint ((CHAR8 *) WindowsizeBuf, sizeof (WindowsizeBuf), "%d"= , > > WindowSize); > > + Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr =3D > > WindowsizeBuf; > > + Mtftp4Token.OptionCount ++; > > } > > > > ShellPrintHiiEx ( > > -1, -1, NULL, STRING_TOKEN (STR_TFTP_DOWNLOADING), > > gShellTftpHiiHandle, FilePath > > @@ -954,14 +990,18 @@ DownloadFile ( > > gShellTftpHiiHandle > > ); > > > > Error : > > > > - if (TftpContext =3D=3D NULL) { > > + if (TftpContext !=3D NULL) { > > FreePool (TftpContext); > > } > > > > + if (Mtftp4Token.OptionList !=3D NULL) { > > + FreePool (Mtftp4Token.OptionList); > > + } > > + > > if (EFI_ERROR (Status)) { > > gBS->FreePages (PagesAddress, EFI_SIZE_TO_PAGES (FileSize)); > > return Status; > > } > > > > diff --git > > > a/ShellPkg/Library/UefiShellTftpCommandLib/UefiShellTftpCommandLib.uni > > > b/ShellPkg/Library/UefiShellTftpCommandLib/UefiShellTftpCommandLib.uni > > index 4f4447d..ca6fa44 100644 > > --- > > > a/ShellPkg/Library/UefiShellTftpCommandLib/UefiShellTftpCommandLib.uni > > +++ > > > b/ShellPkg/Library/UefiShellTftpCommandLib/UefiShellTftpCommandLib.uni > > @@ -1,9 +1,9 @@ > > // /** > > // > > // (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP > > -// Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved. =
> > +// Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved. =
> > // This program and the accompanying materials > > // are licensed and made available under the terms and conditions of t= he > > BSD License > > // which accompanies this distribution. The full text of the license m= ay be > > found at > > // http://opensource.org/licenses/bsd-license.php > > // > > @@ -48,11 +48,11 @@ > > ".SH NAME\r\n" > > "Download a file from TFTP server.\r\n" > > ".SH SYNOPSIS\r\n" > > " \r\n" > > "TFTP [-i interface] [-l ] [-r ] [-c ] [-t > > ]\r\n" > > -" [-s ] host remotefilepath [localfilepath]\r\n" > > +" [-s ] [-w ] host remotefilepath > > [localfilepath]\r\n" > > ".SH OPTIONS\r\n" > > " \r\n" > > " -i interface - Specifies an adapter name, i.e., eth0.\r\n" > > " -l port - Specifies the local port number. Default value i= s 0\r\n" > > " and the port number is automatically assigned.\r= \n" > > @@ -61,10 +61,12 @@ > > " wait for a response. The default value is 6.\r\n= " > > " -t - The number of seconds to wait for a response > after\r\n" > > " sending a request packet. Default value is 4s.\r= \n" > > " -s - Specifies the TFTP blksize option as defined in = RFC > > 2348.\r\n" > > " Valid range is between 8 and 65464, default valu= e is 512.\r\n" > > +" -w - Specifies the TFTP windowsize option as defined = in > > RFC 7440.\r\n" > > +" Valid range is between 1 and 64, default value i= s 1.\r\n" > > " host - Specify TFTP Server IPv4 address.\r\n" > > " remotefilepath - TFTP server file path to download the file.\r\n" > > " localfilepath - Local destination file path.\r\n" > > ".SH DESCRIPTION\r\n" > > " \r\n" > > -- > > 1.9.5.msysgit.1