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

[Udit Kumar <udit.kumar@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Tuesday, November 07, 2017 11:25 PM
> To: Vabhav Sharma <vabhav.sharma@nxp.com>
> Cc: 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; Udit
> Kumar <udit.kumar@nxp.com>
> Subject: Re: [PATCH] Tftp assert fix for openfile failure case
> 
> On Fri, Nov 03, 2017 at 07:56:32PM +0530, Vabhav wrote:
> > Issue:
> > when file open is failed, assert was seen due to freeing 0 size page
> >
> > Reason:
> > DataSize is remain zero if error is reported in ShellOpenFileByName
> >
> > Fix:
> > Update DataSize as soon as FileSize is available
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Udit Kumar <udit.kumar@nxp.com>
> > Signed-off-by: Vabhav <vabhav.sharma@nxp.com>
> > ---
> >  ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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. 
 
> >      }
> > +    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. 

> Or should we delete it from the tree?
 
 
> 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().
> 
> /
>     Leif