Re: [PATCH] Tftp assert fix for openfile failure case

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

Hi Ray,

Please share your thoughts on this.


Thanks,
Meenakshi

> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Tuesday, February 13, 2018 9:07 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Leif Lindholm
> <leif.lindholm@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: Ye, Ting <ting.ye@intel.com>; edk2-devel@lists.01.org; Fu, Siyuan
> <siyuan.fu@intel.com>; ard.biesheuvel@linaro.org
> Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> 
> So I thought we are keeping the command, but I do agree with Leif that
> better error path logic would be good.  We can wait for Ray to confirm if he
> has different plans.
> 
> -Jaben
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Meenakshi Aggarwal
> > Sent: Tuesday, February 13, 2018 7:19 AM
> > To: Carsey, Jaben <jaben.carsey@intel.com>; Leif Lindholm
> > <leif.lindholm@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > Cc: Ye, Ting <ting.ye@intel.com>; edk2-devel@lists.01.org; Fu, Siyuan
> > <siyuan.fu@intel.com>; ard.biesheuvel@linaro.org
> > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case
> > Importance: High
> >
> > Jaben,
> >
> > The patch was not accepted last time because as per Leif
> > "
> > > > > > > > This shell command was introduced in the heyday of "let's
> > > > > > > > reimplement U-Boot in the EDK2 tree". Mainly, from my
> > impression,
> > > it
> > > > > > > > seems to be used in order that people don't need to learn how
> > boot
> > > > > > > > managers and device paths work.
> > > > > > >
> > > > > > > When you say about complete boot, then this may not be useful.
> > > > > > >"
> >
> >
> > So, if we are maintaining tftp command, then i will resend the patch with
> > inclusion of one comment of Leif
> > "
> > > > > > > > If the code is to be kept, I think (from a quick glance) that I
> > > > > > > > would also like to see
> > > > > > > >   *Data = NULL
> > > > > > > > in the error path of DownloadFile().
> > "
> >
> > Thanks,
> > Meenakshi
> >
> > > -----Original Message-----
> > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> > > Sent: Tuesday, February 13, 2018 8:44 PM
> > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Leif Lindholm
> > > <leif.lindholm@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > > Cc: ard.biesheuvel@linaro.org; Ye, Ting <ting.ye@intel.com>; edk2-
> > > devel@lists.01.org; Fu, Siyuan <siyuan.fu@intel.com>; Udit Kumar
> > > <udit.kumar@nxp.com>
> > > Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> > >
> > > Meenakshi,
> > >
> > > The TFTP command is outside the UEFI Shell specification, therefore it is
> > > included as a DynamicCommand, not a command built into the shell itself.
> > >
> > > I a little confused by your last sentence.  Do you want to send a new
> patch?
> > > or do you have a branch to pick changes from ?
> > >
> > > -Jaben
> > >
> > >
> > > > -----Original Message-----
> > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com]
> > > > Sent: Tuesday, February 13, 2018 1:43 AM
> > > > To: Leif Lindholm <leif.lindholm@linaro.org>; Ni, Ruiyu
> > > <ruiyu.ni@intel.com>
> > > > Cc: ard.biesheuvel@linaro.org; Ye, Ting <ting.ye@intel.com>; edk2-
> > > > devel@lists.01.org; Carsey, Jaben <jaben.carsey@intel.com>; Fu,
> Siyuan
> > > > <siyuan.fu@intel.com>; Udit Kumar <udit.kumar@nxp.com>
> > > > Subject: RE: [PATCH] Tftp assert fix for openfile failure case
> > > > Importance: High
> > > >
> > > > Hi,
> > > >
> > > > As per commit 0961002352e9115b72f544dded239ad226efe87b
> > > >
> > > > Tftp command will be maintained to extend internal commands and
> > > >
> > > > ShellPkg/DynamicCommand/TftpDynamicCommand/Tftp.c
> > > >
> > > > Looks like a copy of ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > >
> > > > So, below fix is needed in this case as well.
> > > >
> > > > Please suggest, so we can send the updated patch [incorporating Leif's
> > > > comments]
> > > >
> > > >
> > > > Thanks,
> > > > Meenakshi
> > > >
> > > > > -----Original Message-----
> > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On
> Behalf
> > Of
> > > > > Udit Kumar
> > > > > Sent: Thursday, November 09, 2017 10:13 AM
> > > > > To: Leif Lindholm <leif.lindholm@linaro.org>
> > > > > Cc: ruiyu.ni@intel.com; ard.biesheuvel@linaro.org;
> ting.ye@intel.com;
> > > > edk2-
> > > > > devel@lists.01.org; jaben.carsey@intel.com; siyuan.fu@intel.com
> > > > > Subject: Re: [edk2] [PATCH] Tftp assert fix for openfile failure case
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> > > > > > Sent: Wednesday, November 08, 2017 8:52 PM
> > > > > > To: Udit Kumar <udit.kumar@nxp.com>
> > > > > > Cc: Vabhav Sharma <vabhav.sharma@nxp.com>; edk2-
> > > > devel@lists.01.org;
> > > > > > ruiyu.ni@intel.com; jaben.carsey@intel.com;
> > > ard.biesheuvel@linaro.org;
> > > > > > siyuan.fu@intel.com; ting.ye@intel.com
> > > > > > Subject: Re: [PATCH] Tftp assert fix for openfile failure case
> > > > > >
> > > > > > On Wed, Nov 08, 2017 at 05:15:49AM +0000, Udit Kumar wrote:
> > > > > > > > > diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > > b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > > index fbde3bf..6425fc5 100755
> > > > > > > > > --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > > +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c
> > > > > > > > > @@ -509,6 +509,7 @@ ShellCommandRunTftp (
> > > > > > > > >        );
> > > > > > > > >        goto NextHandle;
> > > > > > > >
> > > > > > > > Wow, a goto in a foor loop in a 320-line function.
> > > > > > > > What could possibly go wrong?
> > > > > > >
> > > > > > > Instead of being on some volume, if you are on Shell.
> > > > > > > Then file open will fail.
> > > > > >
> > > > > > Sure. The above was a snarky comment on the state of the existing
> > > code.
> > > > > >
> > > > > > > > >      }
> > > > > > > > > +    DataSize = FileSize;
> > > > > > > > >
> > > > > > > > >      Status = DownloadFile (Mtftp4, RemoteFilePath,
> > > > > > > > > AsciiRemoteFilePath,
> > > > > > > > FileSize, BlockSize, &Data);
> > > > > > > > >      if (EFI_ERROR (Status)) {
> > > > > > > > > @@ -539,7 +540,6 @@ ShellCommandRunTftp (
> > > > > > > > >        goto NextHandle;
> > > > > > > > >      }
> > > > > > > > >
> > > > > > > > > -    DataSize = FileSize;
> > > > > > > > >      Status = ShellWriteFile (FileHandle, &FileSize, Data);
> > > > > > > > >      if (!EFI_ERROR (Status)) {
> > > > > > > > >        ShellStatus = SHELL_SUCCESS;
> > > > > > > > > --
> > > > > > > > > 1.9.1
> > > > > > > >
> > > > > > > > So, a wider question:
> > > > > > > > This shell command was introduced in the heyday of "let's
> > > > > > > > reimplement U-Boot in the EDK2 tree". Mainly, from my
> > impression,
> > > it
> > > > > > > > seems to be used in order that people don't need to learn how
> > boot
> > > > > > > > managers and device paths work.
> > > > > > >
> > > > > > > When you say about complete boot, then this may not be useful.
> > > > > > >
> > > > > > > > Am I being too harsh?
> > > > > > > > Are there practical uses for this?
> > > > > > >
> > > > > > > For doing some sort of unit testing of given interface. I found this
> > > > > > > useful. During development, this is useful to transfer generic file
> to
> > > > > > > development board.
> > > > > >
> > > > > > OK, I can see how it could be useful.
> > > > > > My opposition is based on three things:
> > > > > > 1) people _are_ trying to use it for boot
> > > > >
> > > > > I agree with this, please see my previous comments,
> > > > > ' When you say about complete boot, then this may not be useful.'
> > > > >
> > > > > > 2) not a command described by UEFI Shell spec, but I keep seeing
> > > > > >    platforms including it even in RELEASE builds (most likely because
> 1)
> > > > > > 3) code quality/maintainability
> > > > >
> > > > > > > > If the code is to be kept, I think (from a quick glance) that I
> > > > > > > > would also like to see
> > > > > > > >   *Data = NULL
> > > > > > > > in the error path of DownloadFile().
> > > > > >
> > > > > > OK, so we don't need to drop it right now, but what's your take on
> > this
> > > > > > comment?
> > > > >
> > > > > I am fine, if you prefer to remove this then we will develop some test
> > > > > application
> > > > > for unit tests.
> > > > > In case, we need to maintain this piece of code then above needs to
> fix
> > > as
> > > > > well.
> > > > >
> > > > >
> > > > > > /
> > > > > >     Leif
> > > > > _______________________________________________
> > > > > edk2-devel mailing list
> > > > > edk2-devel@lists.01.org
> > > > >
> > > >
> > >
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> > > > > s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> > > > >
> > > >
> > >
> >
> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7Cc2673b1a07e
> > > > >
> > > >
> > >
> >
> 94e9f937b08d5272c6e5b%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > > > >
> > > >
> > >
> >
> %7C636457994167308954&sdata=gBy9RA5d1NpsvxQkmET0HFzsJB8FK7KLueE
> > > > > NXFTjSHY%3D&reserved=0
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C6f1efaa8f701
> 48a23c7b08d572f79ec1%7Cbd8a2a2207224ec7b35f1c4f0497e341%7C0%7C0%
> 7C636541330242351234&sdata=qOkb3XaVjUhsi3cVkoskQN6z%2Bv8ySuCq1W
> gFk90mZh4%3D&reserved=0