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....@intel.com>; devel@edk2.groups.io > Cc: Ni, Ray <ray...@intel.com>; Augustine, Linson > <linson.august...@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:phi...@redhat.com] > >> Sent: Monday, December 2, 2019 6:21 PM > >> To: devel@edk2.groups.io; Gao, Zhichao <zhichao....@intel.com> > >> Cc: Ni, Ray <ray...@intel.com>; Augustine, Linson > >> <linson.august...@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...@intel.com> > >>> Cc: Linson Augustine <linson.august...@intel.com> > >>> Signed-off-by: Zhichao Gao <zhichao....@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 <phi...@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". > >>> > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#52145): https://edk2.groups.io/g/devel/message/52145 Mute This Topic: https://groups.io/mt/64492759/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-