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

[Leif Lindholm <leif.lindholm@linaro.org>]

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
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?

/
    Leif