From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web09.2429.1575284998128488703 for ; Mon, 02 Dec 2019 03:09:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VcXP6jg5; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575284997; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ua1WERwulpMhC8L9bBKbJ1fDFrQ+393LyLwUFuWQFNc=; b=VcXP6jg5hlYpQLAS2G1NQ2KFiCKfR1LBXnusffdb2sldgwPY9LWV0Qsoei5dXIEbSZbI+Z A4bgvR7eEFWDfFp8T7SUADtsCikcMrxXNtr7mqok5e1JEQqJlpsqJSsQRid5/j8houh/O2 L3MLk7OzfJGArg6LF0HfEonrYHP9K/s= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-379-QJTK6xnLM4mqqWhEGSIv4w-1; Mon, 02 Dec 2019 06:09:56 -0500 Received: by mail-wm1-f69.google.com with SMTP id q21so874879wmc.7 for ; Mon, 02 Dec 2019 03:09:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=wXCRattymoRp3fr/Q8LT73jvLvwDs9CkpG/TPVWrTsY=; b=nowTgdSEdzQlFJMpnPt7Ww8crB4FJ9XUMdysD8WXAdfVVNbyUf6QEc+C7jwo4QJ5hl /KXJi1YMncO9+tWDRsC5+WBv08Vtgyat8D3pFHxyHOPe3uKMxmgYt3qTr1YY+DqwKY5r kLUuNVD9Nt3SMhnRoocUxLJPgXQlvZ1219uFV3dWDLkY75gQAYuDbrRBMRpp/4QDUrdF YMhdXTQdDLuzPBjFs3mMwU2ax6dzWJcReAdHTrJT0qgr8rQ06tH2c9rXT3Bmzznk3FHy GNTmXwDAh8xeaN5jEwAIhpfcvwRXL5gKdyu4LzGzdah/QmKCr9ax4WJZeuzUqXocLcxA 8UCw== X-Gm-Message-State: APjAAAV7JJXX116pUpkPaHk7HvQh6+q4+ZRvn8Q+wC5duxv1qMO/qT55 IJoNYzv8ZYI1qxdwRLqzDNey4A9yuhD/Ar94gN4Gr/IMYpm7IPAxCKQZwrCPRbH8Gr+Aqn+nHIb KeIiEPXBqPrjIqg== X-Received: by 2002:a1c:7715:: with SMTP id t21mr18981518wmi.149.1575284994943; Mon, 02 Dec 2019 03:09:54 -0800 (PST) X-Google-Smtp-Source: APXvYqzC9bVHzZaOXfIzZvUnykfG16tKY2rlCv+Lfr/y3LL+zs5T+jCDEpJHAUsFdVIw/N59cmWIZA== X-Received: by 2002:a1c:7715:: with SMTP id t21mr18981479wmi.149.1575284994619; Mon, 02 Dec 2019 03:09:54 -0800 (PST) Return-Path: Received: from [192.168.1.35] (182.red-88-21-103.staticip.rima-tde.net. [88.21.103.182]) by smtp.gmail.com with ESMTPSA id e18sm17031457wrr.95.2019.12.02.03.09.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 02 Dec 2019 03:09:53 -0800 (PST) Subject: Re: [edk2-devel] [PATCH] ShellPkg/ShellProtocol: Return error code while fail parsing cmd-line To: "Gao, Zhichao" , "devel@edk2.groups.io" Cc: "Ni, Ray" , "Augustine, Linson" References: <20191202005348.22208-1-zhichao.gao@intel.com> <6f2965c6-862a-e043-420a-9dceb17e63f9@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B88BF58@SHSMSX101.ccr.corp.intel.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: Date: Mon, 2 Dec 2019 12:09:52 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B88BF58@SHSMSX101.ccr.corp.intel.com> X-MC-Unique: QJTK6xnLM4mqqWhEGSIv4w-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252; format=flowed Content-Transfer-Encoding: quoted-printable Hi Zhichao, 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 c= ode >> 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 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)) { >> >> UpdateArgcArgv() is documented to only return EFI_OUT_OF_RESOURCES as >> error, however it calls ParseCommandLineToArgs() which - also not docume= nted >> - 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 St= atus is >> EFI_OUT_OF_RESOURCES? >=20 > It is fine to return EFI_OUT_OF_RESOURCES of this function as it descript= s in its function comments. And all the EFI_OUT_OF_RESOURCES is returned du= e 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 "E= FI_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=20 v2 IMO (and I prepared those patches locally). Reviewed-by: Philippe Mathieu-Daude >> >>> + goto UnloadImage; >>> + } >>> + >>> // >>> // Replace Argv[0] with the full path of the binary we're execut= ing: >>> // If the command line was "foo", the binary might be called "fo= o.efi". >>> >=20