From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: insyde.com, ip: 210.71.195.42, mailfrom: tim.lewis@insyde.com) Received: from out04.hibox.biz (out04.hibox.biz [210.71.195.42]) by groups.io with SMTP; Tue, 07 May 2019 13:48:57 -0700 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2DpAADE7tFc/w00GKxkGwEBAQEDAQE?= =?us-ascii?q?BBwMBAQGBZYJ6chIog0hIiHuMKTUBjggRhEgjhxMQIwkBhDoEAgKCOTgTAQM?= =?us-ascii?q?BAQUBAQEBAm0cDIVKAQEFCAIZBSYICBsMAQUGAw0EBAEBAQICIwMCGxgVCQg?= =?us-ascii?q?CBAESCwUNBIMCggoPrXx+MRoChBYBEUOFKIELJ4EPUYtFP4ERgxI+hA0hgyC?= =?us-ascii?q?CWASKdwcZghOGJoZcjDFlBwICgglZAYVDhDKFU4ItG4IQYoVig10Dh1mBSow?= =?us-ascii?q?kgSGFLI5RgWYhgVZwgzwJNoIHgieDdYIFhgoiMIEhCBONS4JSAQE?= X-IronPort-AV: E=Sophos;i="5.60,443,1549900800"; d="scan'208";a="12580229" Received: from unknown (HELO hb3-BKT203.hibox.biz) ([172.24.52.13]) by out04.hibox.biz with ESMTP; 08 May 2019 04:48:51 +0800 Received: from unknown (HELO hb3-BKT103.hibox.biz) ([172.24.51.13]) by hb3-BKT203.hibox.biz with ESMTP; 08 May 2019 04:48:51 +0800 Received: from unknown (HELO hb3-IN01.hibox.biz) ([172.24.12.11]) by hb3-BKT103.hibox.biz with ESMTP; 08 May 2019 04:48:52 +0800 X-Remote-IP: 73.116.1.175 X-Remote-Host: c-73-116-1-175.hsd1.ca.comcast.net X-SBRS: -10.0 X-MID: 22336959 X-Auth-ID: tim.lewis@insyde.com X-EnvelopeFrom: tim.lewis@insyde.com hiBox-Sender: 1 Received: from c-73-116-1-175.hsd1.ca.comcast.net (HELO TIMSLAPTOP) ([73.116.1.175]) by hb3-IN01.hibox.biz with ESMTP/TLS/AES256-SHA; 08 May 2019 04:48:48 +0800 From: "Tim Lewis" To: , Cc: , , , , In-Reply-To: <93e3ac82d2d6445bbb2b647af900ce2c@ausx13mps335.AMER.DELL.COM> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option Date: Tue, 7 May 2019 13:48:48 -0700 Message-ID: <000301d50516$473ac170$d5b04450$@insyde.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQGSBsoXrCoeseeDC3ehKSYnQpo+s6bmHoYg Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: en-us Jim -- Well, speaking of shooting-oneself-in-the-foot, it turns out that our non-= EDK2 implementation followed the recommendation in the patch.=20 I agree that the spec is ambiguous and, it turns out that our largest use = case already uses the recommended behavior. Sorry to one and all. Tim -----Original Message----- From: Jim.Dailey@dell.com =20 Sent: Tuesday, May 7, 2019 1:30 PM To: devel@edk2.groups.io; tim.lewis@insyde.com Cc: dandan.bi@intel.com; jwatt@jwatt.org; jaben.carsey@intel.com; zhichao.= gao@intel.com; ray.ni@intel.com Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib:= Fix '-opt' option Tim, Out of curiosity, what does the specification you refer to that was used t= o write the non-EDK2 implementations say about the -opt switch? Regards, Jim -----Original Message----- From: devel@edk2.groups.io On Behalf Of Tim Lewis Sent: Tuesday, May 7, 2019 3:04 PM To: 'Jonathan Watt'; 'Carsey, Jaben'; devel@edk2.groups.io; 'Gao, Zhichao'= ; 'Ni, Ray' Cc: 'Bi, Dandan' Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib:= Fix '-opt' option Jonathan -- The bcfg command pre-dates the UEFI shell specification. I know of at leas= t two non-EDK2 implementations, including one maintained by my company, tha= t are implemented to the specification. Server platforms that use the "appl= ication" style boot options can regularly run over 10 options.=20 I believe the better alternative is to add a new option in the specificat= ion and leave the existing syntax for -opt. Thanks, Tim -----Original Message----- From: Jonathan Watt Sent: Tuesday, May 7, 2019 12:06 PM To: Carsey, Jaben ; devel@edk2.groups.io; tim.lewi= s@insyde.com; Gao, Zhichao ; Ni, Ray Cc: Bi, Dandan Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib:= Fix '-opt' option I should add, for me personally, once I noticed the inconsistency I change= d my scripts to use the "0x" prefix to avoid this real footgun. I imagine t= hat anyone else that may have encountered this would have done the same and= so, like me, wouldn't be affected by the change if it were to happen. On 07/05/2019 20:00, Jonathan Watt wrote: > There is potential for that, but it's not certain. For it to happen=20 > scripts would need to be both omitting the "0x" prefix and be pass an=20 > option number greater than 9. The fact this very unexpected=20 > inconsistency (which will corrupt the wrong option when those same two= =20 > things are true!) hasn't been reported before would seem to indicate=20 > this combination doesn't really happen/is rare in practice. >=20 > Also, is TianoCore's bcfg the only implementation people are using? If= =20 > there are other implementations, would this bring TianoCore's=20 > implementation into or out of line with them? That may impact whether th= e spec could/should change. >=20 > On 07/05/2019 18:40, Carsey, Jaben wrote: >> It will break existing scripts. Do you have such scripts in your envir= onment dependent on this parameter? >> >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf=20 >>> Of Tim Lewis >>> Sent: Tuesday, May 07, 2019 9:20 AM >>> To: devel@edk2.groups.io; Carsey, Jaben ;=20 >>> Gao, Zhichao ; Ni, Ray ;=20 >>> jwatt@jwatt.org >>> Cc: Bi, Dandan >>> Subject: Re: [edk2-devel] [PATCH v1 1/1] >>> ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option >>> Importance: High >>> >>> The question is whether this will break compatibility with existing=20 >>> shell scripts. In order to maintain that compatibility, it may be=20 >>> necessary to add a new option rather than trying to update an existing= one. >>> >>> Tim >>> >>> -----Original Message----- >>> From: devel@edk2.groups.io On Behalf Of=20 >>> Carsey, Jaben >>> Sent: Tuesday, May 7, 2019 7:36 AM >>> To: Gao, Zhichao ; devel@edk2.groups.io; Ni,=20 >>> Ray ; jwatt@jwatt.org >>> Cc: Bi, Dandan >>> Subject: Re: [edk2-devel] [PATCH v1 1/1] >>> ShellPkg/UefiShellBcfgCommandLib: >>> Fix '-opt' option >>> >>> Zhichao, >>> I can help submit errata for shell spec if needed. >>> >>> Per patch, >>> I agree. This looks good. >>> Reviewed-by: Jaben Carsey >>> >>> >>>> -----Original Message----- >>>> From: Gao, Zhichao >>>> Sent: Tuesday, May 07, 2019 2:52 AM >>>> To: devel@edk2.groups.io; Ni, Ray ;=20 >>>> jwatt@jwatt.org >>>> Cc: Carsey, Jaben ; Bi, Dandan=20 >>>> >>>> Subject: RE: [edk2-devel] [PATCH v1 1/1] >>>> ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option >>>> Importance: High >>>> >>>> This patch looks good for me. >>>> Reviewed-by: Zhichao Gao >>>> >>>> But when I view the command in UEFI SHELL 2.2 spec: >>>> ... >>>> bcfg driver|boot [-opt # [[filename]|["data"]] | [KeyData >>> UnicodeChar>*]] >>>> ... >>>> -opt >>>> Modify the optional data associated with a driver or boot option. >>>> Followed either by the filename of the file which contains the=20 >>>> binary data to be associated with the driver or boot option=20 >>>> optional data, or else the quote- delimited data that will be=20 >>>> associated with the driver or boot option optional data. >>>> ... >>>> >>>> This description lack the comment of '#' parameter and that may=20 >>>> make the consumer confused. Usually consumers would regard it as=20 >>>> the same in other option, such as ' bcfg driver|boot [rm #]'. The=20 >>>> '#' is clearly descripted as a hexadecimal parameter: >>>> rm >>>> Remove an option. The # parameter lists the option number to remove= =20 >>>> in hexadecimal. >>>> >>>> So I think we should update the shell spec by the way. >>>> >>>> Thanks, >>>> Zhichao >>>> >>>>> -----Original Message----- >>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf= =20 >>>>> Of >>>> Ni, >>>>> Ray >>>>> Sent: Monday, May 6, 2019 10:02 PM >>>>> To: jwatt@jwatt.org; devel@edk2.groups.io >>>>> Cc: Carsey, Jaben ; Bi, Dandan=20 >>>>> >>>>> Subject: Re: [edk2-devel] [PATCH v1 1/1] >>>> ShellPkg/UefiShellBcfgCommandLib: >>>>> Fix '-opt' option >>>>> >>>>> Dandan, >>>>> Can you please help to review? >>>>> >>>>> Thanks, >>>>> Ray >>>>> >>>>>> -----Original Message----- >>>>>> From: jwatt@jwatt.org [mailto:jwatt@jwatt.org] >>>>>> Sent: Monday, May 6, 2019 9:03 PM >>>>>> To: devel@edk2.groups.io >>>>>> Cc: Carsey, Jaben ; Ni, Ray=20 >>>>>> >>>>>> Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt= ' >>>>>> option >>>>>> >>>>>> From: Jonathan Watt >>>>>> >>>>>> For all other bcfg commands the "#" (option number) argument(s)=20 >>>>>> are treated as hexedecimal values regardless of whether or not=20 >>>>>> they are prefixed by "0x". This change fixes '-opt' to handle its = "#" >>>>>> (option number) argument consistently with the other commands. >>>>>> >>>>>> Making this change removes a potential footgun whereby a user=20 >>>>>> that has been using a number without a "0x" prefix with other=20 >>>>>> bcfg commands finds that, on using that exact same number with=20 >>>>>> '-opt', it has this time unexpectedly been interpreted as a=20 >>>>>> decimal number and they have modified >>>>>> (corrupted) an unrelated load option. For example, a user may=20 >>>>>> have been specifying "10" to other commands to have them act on=20 >>>>>> the 16th option (because simply "10", without any prefix, is how=20 >>>>>> 'bcfg boot dump' displayed the option number for the 16th option). >>>>>> Unfortunately for them, if they also use '-opt' with "10" it=20 >>>>>> would unexpectedly and inconsistently act on the 10th option. >>>>>> >>>>>> CC: Jaben Carsey >>>>>> CC: Ray Ni >>>>>> Signed-off-by: Jonathan Watt >>>>>> --- >>>>>> >>>>>> >>> ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c >>>> | >>>>>> 2 >>>>>> +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git >>>>>> >>>> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib. >>>> c >>>>>> >>>> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib. >>>> c >>>>>> index d033c7c1dc59..e8b48b4990dd 100644 >>>>>> --- >>>>>> >>>> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib. >>>> c >>>>>> +++ >>>>>> >>>> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib. >>>> c >>>>>> @@ -1019,7 +1019,7 @@ BcfgAddOpt( >>>>>> // >>>>>> // Get the index of the variable we are changing. >>>>>> // >>>>>> - Status =3D ShellConvertStringToUint64(Walker, &Intermediate,=20 >>>>>> FALSE, TRUE); >>>>>> + Status =3D ShellConvertStringToUint64(Walker, &Intermediate,=20 >>>>>> + TRUE, TRUE); >>>>>> if (EFI_ERROR(Status) || (((UINT16)Intermediate) !=3D >>>>>> Intermediate) >>>>>> || StrStr(Walker, L" ") =3D=3D NULL || ((UINT16)Intermediate) > >>>>>> ((UINT16)OrderCount)) { >>>>>> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN=20 >>>>>> (STR_GEN_PARAM_INV), gShellBcfgHiiHandle, L"bcfg", L"Option >>> Index"); >>>>>> ShellStatus =3D SHELL_INVALID_PARAMETER; >>>>>> -- >>>>>> 2.21.0 >>>>> >>>>> >>>>> >>> >>> >>> >>> >>> >>> >>>=20 >> >> >=20