Re: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return error code while fail parsing cmd-line

["Gao, Zhichao" <zhichao.gao@intel.com>]

Hi Ray,

Can you help to review the patch?

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Philippe Mathieu-Daudé
> Sent: Monday, December 2, 2019 7:10 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Augustine, Linson
> <linson.augustine@intel.com>
> Subject: Re: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return error code
> while fail parsing cmd-line
> 
> Hi Zhichao,
> 
> On 12/2/19 12:04 PM, Gao, Zhichao wrote:
> >> -----Original Message-----
> >> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> >> Sent: Monday, December 2, 2019 6:21 PM
> >> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> >> Cc: Ni, Ray <ray.ni@intel.com>; Augustine, Linson
> >> <linson.augustine@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return
> >> error code while fail parsing cmd-line
> >>
> >> On 12/2/19 1:53 AM, Gao, Zhichao via Groups.Io wrote:
> >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2395
> >>>
> >>> Errors happened in the arguments parsing is not a critical error.
> >>> And it would miss the error status code in the release version of shell.
> >>> So replace the ASSERT with returning error status code while fail
> >>> parsing command-line in UpdateArgcArgv.
> >>>
> >>> Cc: Ray Ni <ray.ni@intel.com>
> >>> Cc: Linson Augustine <linson.augustine@intel.com>
> >>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> >>> ---
> >>>    ShellPkg/Application/Shell/ShellProtocol.c | 5 ++++-
> >>>    1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
> >>> b/ShellPkg/Application/Shell/ShellProtocol.c
> >>> index 5e529b6568..f0362a42d8 100644
> >>> --- a/ShellPkg/Application/Shell/ShellProtocol.c
> >>> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> >>> @@ -1497,7 +1497,10 @@ InternalShellExecuteDevicePath(
> >>>        ShellParamsProtocol.StdOut  =
> >> ShellInfoObject.NewShellParametersProtocol->StdOut;
> >>>        ShellParamsProtocol.StdErr  =
> >> ShellInfoObject.NewShellParametersProtocol->StdErr;
> >>>        Status = UpdateArgcArgv(&ShellParamsProtocol, NewCmdLine,
> >> Efi_Application, NULL, NULL);
> >>> -    ASSERT_EFI_ERROR(Status);
> >>> +    if (EFI_ERROR (Status)) {
> >>
> >> UpdateArgcArgv() is documented to only return
> EFI_OUT_OF_RESOURCES as
> >> error, however it calls ParseCommandLineToArgs() which - also not
> >> documented
> >> - returns EFI_INVALID_PARAMETER.
> >> I suppose this is the "not critical" error this BZ is trying to catch.
> >>
> >> Should we force Status to EFI_INVALID_PARAMETER before returning, is
> >> it safer to return EFI_OUT_OF_RESOURCES if it ever occurs? Should we
> >> assert if Status is EFI_OUT_OF_RESOURCES?
> >
> > It is fine to return EFI_OUT_OF_RESOURCES of this function as it descripts
> in its function comments. And all the EFI_OUT_OF_RESOURCES is returned
> due to the failure of memory allocating.
> 
> OK, thanks for clearing that with me.
> 
> > And here is an issue of this patch, missing add the return description
> "EFI_INVALID_PARAMETER" of UpdateArgcArgv  and
> ParseCommandLineToArgs.
> > I would add that in the V2 version.
> 
> This is not related to the BZ you are trying to solve, so no need for a
> v2 IMO (and I prepared those patches locally).
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 
> >>
> >>> +      goto UnloadImage;
> >>> +    }
> >>> +
> >>>        //
> >>>        // Replace Argv[0] with the full path of the binary we're executing:
> >>>        // If the command line was "foo", the binary might be called
> "foo.efi".
> >>>
> >
> 
> 
>