From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mx.groups.io with SMTP id smtpd.web09.2373.1575284701530392302 for ; Mon, 02 Dec 2019 03:05:01 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: zhichao.gao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Dec 2019 03:05:01 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,268,1571727600"; d="scan'208";a="208089770" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga007.fm.intel.com with ESMTP; 02 Dec 2019 03:05:01 -0800 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 2 Dec 2019 03:05:00 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.19]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.236]) with mapi id 14.03.0439.000; Mon, 2 Dec 2019 19:04:59 +0800 From: "Gao, Zhichao" To: =?iso-8859-1?Q?Philippe_Mathieu-Daud=E9?= , "devel@edk2.groups.io" CC: "Ni, Ray" , "Augustine, Linson" Subject: Re: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return error code while fail parsing cmd-line Thread-Topic: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return error code while fail parsing cmd-line Thread-Index: AQHVqPokmrfvgBb4Q0+JWCoLuO6Beaemq3sQ Date: Mon, 2 Dec 2019 11:04:58 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B88BF58@SHSMSX101.ccr.corp.intel.com> References: <20191202005348.22208-1-zhichao.gao@intel.com> <6f2965c6-862a-e043-420a-9dceb17e63f9@redhat.com> In-Reply-To: <6f2965c6-862a-e043-420a-9dceb17e63f9@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Philippe Mathieu-Daud=E9 [mailto:philmd@redhat.com] > Sent: Monday, December 2, 2019 6:21 PM > To: devel@edk2.groups.io; Gao, Zhichao > Cc: Ni, Ray ; Augustine, Linson > Subject: Re: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return error co= de > while fail parsing cmd-line >=20 > On 12/2/19 1:53 AM, Gao, Zhichao via Groups.Io wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2395 > > > > 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 > > Cc: Linson Augustine > > Signed-off-by: Zhichao Gao > > --- > > 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 =3D > ShellInfoObject.NewShellParametersProtocol->StdOut; > > ShellParamsProtocol.StdErr =3D > ShellInfoObject.NewShellParametersProtocol->StdErr; > > Status =3D UpdateArgcArgv(&ShellParamsProtocol, NewCmdLine, > Efi_Application, NULL, NULL); > > - ASSERT_EFI_ERROR(Status); > > + if (EFI_ERROR (Status)) { >=20 > UpdateArgcArgv() is documented to only return EFI_OUT_OF_RESOURCES as > error, however it calls ParseCommandLineToArgs() which - also not documen= ted > - returns EFI_INVALID_PARAMETER. > I suppose this is the "not critical" error this BZ is trying to catch. >=20 > Should we force Status to EFI_INVALID_PARAMETER before returning, is it s= afer > to return EFI_OUT_OF_RESOURCES if it ever occurs? Should we assert if Sta= tus 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. 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. Thanks, Zhichao >=20 > > + goto UnloadImage; > > + } > > + > > // > > // Replace Argv[0] with the full path of the binary we're executi= ng: > > // If the command line was "foo", the binary might be called "foo= .efi". > >