From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web10.3027.1576129468426332133 for ; Wed, 11 Dec 2019 21:44:28 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: zhichao.gao@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Dec 2019 21:44:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,304,1571727600"; d="scan'208";a="207953737" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga008.jf.intel.com with ESMTP; 11 Dec 2019 21:44:27 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 11 Dec 2019 21:44:27 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 11 Dec 2019 21:44:27 -0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.19]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.29]) with mapi id 14.03.0439.000; Thu, 12 Dec 2019 13:44:19 +0800 From: "Gao, Zhichao" To: "devel@edk2.groups.io" , "philmd@redhat.com" 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//9+fgCAD+JbEA== Date: Thu, 12 Dec 2019 05:44:18 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B8935CF@SHSMSX101.ccr.corp.intel.com> References: <20191202005348.22208-1-zhichao.gao@intel.com> <6f2965c6-862a-e043-420a-9dceb17e63f9@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B88BF58@SHSMSX101.ccr.corp.intel.com> In-Reply-To: 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 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=E9 > Sent: Monday, December 2, 2019 7:10 PM > To: Gao, Zhichao ; devel@edk2.groups.io > Cc: Ni, Ray ; Augustine, Linson > > Subject: Re: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return error c= ode > while fail parsing cmd-line >=20 > Hi Zhichao, >=20 > On 12/2/19 12:04 PM, Gao, Zhichao wrote: > >> -----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 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=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 sh= ell. > >>> 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)) { > >> > >> 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 descr= ipts > in its function comments. And all the EFI_OUT_OF_RESOURCES is returned > due to the failure of memory allocating. >=20 > OK, thanks for clearing that with me. >=20 > > 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. >=20 > 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). >=20 > Reviewed-by: Philippe Mathieu-Daude >=20 > >> > >>> + goto UnloadImage; > >>> + } > >>> + > >>> // > >>> // Replace Argv[0] with the full path of the binary we're exe= cuting: > >>> // If the command line was "foo", the binary might be called > "foo.efi". > >>> > > >=20 >=20 >=20