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

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

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?

>      }
> +    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.
Am I being too harsh?
Are there practical uses for this?
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