Re: [Patch 2/2] ShellPkg/tftp: Add one option for tftp command to specify windowsize

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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 <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Carsey,
> Jaben <jaben.carsey@intel.com>
> Subject: RE: [Patch 2/2] ShellPkg/tftp: Add one option for tftp command to
> specify windowsize
> 
> 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?
> 
> -Jaben
> 
> > -----Original Message-----
> > From: Wu, Jiaxin
> > Sent: Thursday, March 16, 2017 1:36 AM
> > To: edk2-devel@lists.01.org
> > Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>;
> Carsey,
> > Jaben <jaben.carsey@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> > 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 <ting.ye@intel.com>
> > Cc: Fu Siyuan <siyuan.fu@intel.com>
> > Cc: Carsey Jaben <jaben.carsey@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> > ---
> >  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.<BR>
> > -  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<BR>
> >
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of the
> BSD
> > License
> >    which accompanies this distribution.  The full text of the license may 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[] =
> {
> >    {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[] =
> {
> >  ///
> >  /// 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         = SHELL_INVALID_PARAMETER;
> >    ProblemParam        = NULL;
> >    NicFound            = FALSE;
> >    AsciiRemoteFilePath = NULL;
> >    Handles             = NULL;
> >    FileSize            = 0;
> >    BlockSize           = MTFTP_DEFAULT_BLKSIZE;
> > +  WindowSize          = 1;
> 
>  [Jaben] If the #define is the default, then why do we not initialize the
> variable with it?
> 
> >
> >    //
> >    // Initialize the Shell library (we must be in non-auto-init...)
> >    //
> >    Status = ShellInitialize ();
> > @@ -432,10 +441,24 @@ ShellCommandRunTftp (
> >        );
> >        goto Error;
> >      }
> >    }
> >
> > +  ValueStr = ShellCommandLineGetValue (CheckPackage, L"-w");
> > +  if (ValueStr != NULL) {
> > +    if (!StringToUint16 (ValueStr, &WindowSize)) {
> > +      goto Error;
> > +    }
> > +    if (WindowSize < MTFTP_DEFAULT_WINDOWSIZE || WindowSize >
> > MTFTP_MAX_WINDOWSIZE) {
> 
>  [Jaben] If the value must not be less that it, it should be labeled minimum
> and not default.
> 
> > +      ShellPrintHiiEx (
> > +        -1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
> > +        gShellTftpHiiHandle, L"tftp", ValueStr
> > +      );
> > +      goto Error;
> > +    }
> > +  }
> > +
> >    //
> >    // Locate all MTFTP4 Service Binding protocols
> >    //
> >    ShellStatus = SHELL_NOT_FOUND;
> >    Status = gBS->LocateHandleBuffer (
> > @@ -506,11 +529,11 @@ ShellCommandRunTftp (
> >          gShellTftpHiiHandle, RemoteFilePath, NicName, Status
> >        );
> >        goto NextHandle;
> >      }
> >
> > -    Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath,
> > FileSize, BlockSize, &Data);
> > +    Status = DownloadFile (Mtftp4, RemoteFilePath, AsciiRemoteFilePath,
> > 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    = (UINT8*)AsciiFilePath;
> >    Mtftp4Token.BufferSize  = FileSize;
> >    Mtftp4Token.Buffer      = Buffer;
> >    Mtftp4Token.CheckPacket = CheckPacket;
> >    Mtftp4Token.Context     = (VOID*)TftpContext;
> > +  Mtftp4Token.OptionCount = 0;
> > +  Mtftp4Token.OptionList  = AllocatePool (sizeof (EFI_MTFTP4_OPTION) *
> > 2);
> > +  if (Mtftp4Token.OptionList == NULL) {
> > +    Status = EFI_OUT_OF_RESOURCES;
> > +    goto Error;
> > +  }
> > +
> >    if (BlockSize != MTFTP_DEFAULT_BLKSIZE) {
> > -    ReqOpt.OptionStr = (UINT8 *) "blksize";
> > -    AsciiSPrint ((CHAR8 *)OptBuf, sizeof (OptBuf), "%d", BlockSize);
> > -    ReqOpt.ValueStr  = OptBuf;
> > +    Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr =
> (UINT8
> > *) "blksize";
> > +    AsciiSPrint ((CHAR8 *) BlksizeBuf, sizeof (BlksizeBuf), "%d", BlockSize);
> > +    Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr  =
> > BlksizeBuf;
> > +    Mtftp4Token.OptionCount ++;
> > +  }
> >
> > -    Mtftp4Token.OptionCount = 1;
> > -    Mtftp4Token.OptionList  = &ReqOpt;
> > +  if (WindowSize != MTFTP_DEFAULT_WINDOWSIZE) {
> > +    Mtftp4Token.OptionList[Mtftp4Token.OptionCount].OptionStr =
> (UINT8
> > *) "windowsize";
> > +    AsciiSPrint ((CHAR8 *) WindowsizeBuf, sizeof (WindowsizeBuf), "%d",
> > WindowSize);
> > +    Mtftp4Token.OptionList[Mtftp4Token.OptionCount].ValueStr  =
> > WindowsizeBuf;
> > +    Mtftp4Token.OptionCount ++;
> >    }
> >
> >    ShellPrintHiiEx (
> >      -1, -1, NULL, STRING_TOKEN (STR_TFTP_DOWNLOADING),
> >      gShellTftpHiiHandle, FilePath
> > @@ -954,14 +990,18 @@ DownloadFile (
> >      gShellTftpHiiHandle
> >      );
> >
> >  Error :
> >
> > -  if (TftpContext == NULL) {
> > +  if (TftpContext != NULL) {
> >      FreePool (TftpContext);
> >    }
> >
> > +  if (Mtftp4Token.OptionList != 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<BR>
> > -// Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved. <BR>
> > +// Copyright (c) 2010 - 2017, Intel Corporation. All rights reserved. <BR>
> >  // This program and the accompanying materials
> >  // are licensed and made available under the terms and conditions of the
> > BSD License
> >  // which accompanies this distribution. The full text of the license may 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 <port>] [-r <port>] [-c <retry count>] [-t
> > <timeout>]\r\n"
> > -"     [-s <block size>] host remotefilepath [localfilepath]\r\n"
> > +"     [-s <block size>] [-w <window size>] 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 is 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 <timeout>     - The number of seconds to wait for a response
> after\r\n"
> >  "                     sending a request packet. Default value is 4s.\r\n"
> >  "  -s <block size>  - Specifies the TFTP blksize option as defined in RFC
> > 2348.\r\n"
> >  "                     Valid range is between 8 and 65464, default value is 512.\r\n"
> > +"  -w <window size> - Specifies the TFTP windowsize option as defined in
> > RFC 7440.\r\n"
> > +"                     Valid range is between 1 and 64, default value is 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