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 out02.hibox.biz (out02.hibox.biz [210.71.195.42]) by groups.io with SMTP; Tue, 07 May 2019 14:02:54 -0700 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2BWAAAM8tFc/ws0GKxkGgEBAQEBAgE?= =?us-ascii?q?BAQEHAgEBAQGBZYFigRhyEiiDSEiIe4wpNQGOCBGESCOHExAjCQGEOgQCAoI?= =?us-ascii?q?5OBMBAwEBBQEBAQECbRwMhUoBAQUIAgQVBSYICAcUDAEFBgMNBAQBAQECAiM?= =?us-ascii?q?DAhsYFQkIAgQBEgsFDQSDAoIKD618fjEaAoNqASsBEUOFKIELJ4EPUYtFP4E?= =?us-ascii?q?RghR+PoQNIYMgglgEincHGQGIOJMNZQcCAoIJWQGFQ4QyhVOCLRuCEGKFYoN?= =?us-ascii?q?dAw+JFIwkgSGFLI5RgWYhgVZwgzwJNoFcFxSCJ4N1ggWFMwFWIjCBIQgTjUu?= =?us-ascii?q?CUgEB?= X-IronPort-AV: E=Sophos;i="5.60,443,1549900800"; d="scan'208";a="13036234" Received: from unknown (HELO hb3-BKT201.hibox.biz) ([172.24.52.11]) by out02.hibox.biz with ESMTP; 08 May 2019 05:02:49 +0800 Received: from unknown (HELO hb3-BKT102.hibox.biz) ([172.24.51.12]) by hb3-BKT201.hibox.biz with ESMTP; 08 May 2019 05:02:49 +0800 Received: from unknown (HELO hb3-IN01.hibox.biz) ([172.24.12.11]) by hb3-BKT102.hibox.biz with ESMTP; 08 May 2019 05:02:49 +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: 22336979 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 05:02:47 +0800 From: "Tim Lewis" To: , , "'Carsey, Jaben'" , "'Gao, Zhichao'" , "'Ni, Ray'" Cc: "'Bi, Dandan'" In-Reply-To: <4e7fdcdb-9ec6-543c-8093-5b77666fdd6a@jwatt.org> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option Date: Tue, 7 May 2019 14:02:45 -0700 Message-ID: <001601d50518$3a0ea550$ae2beff0$@insyde.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHv8INph07ECK71uBadIpyUz+3uG6YqTqCQ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: en-us Jonathan -- My apologies. I jumped because we've been bitten by shell "clarifications"= in the past. As you've probably read in the other thread, it turns out that I (we) actu= ally did agree with your interpretation of the spec in our alternate implem= entation and have been using it that way for 2+ years. And it didn't cause = us grief with our other product which does use an EDK2-derived shell.=20 Best regards, Tim -----Original Message----- From: devel@edk2.groups.io On Behalf Of Jonathan Wa= tt Sent: Tuesday, May 7, 2019 1:51 PM To: Tim Lewis ; 'Carsey, Jaben' ; devel@edk2.groups.io; 'Gao, Zhichao' ; 'Ni, Ra= y' Cc: 'Bi, Dandan' Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib:= Fix '-opt' option Hi Tim, For context, I'm just some random guy who tripped over this issue on his h= ome workstation and thought he'd try and remove the footgun to save anyone = else the same pain. I was specifically replying to the unconditional statem= ent "It will break existing scripts." (not made by you) to provide what I h= ope was some qualification and balance to the face value of that statement,= and to suggest some other things that should be considered. As far as deci= ding what the best resolution is, I'm not qualified for that. I am curious about one thing though. The sentence you wrote that ends with= "that are implemented to the specification" sounds like you're saying maki= ng the proposed change would violate the specification. That does not seem = to be the case from my reading, and my reading would be that it would actua= lly make it do what most people would expect from reading the specification= . Specifically, the usage block for bcfg in the specification says: Usage: bcfg driver|boot [dump [-v]] bcfg driver|boot [add # file "desc"] [addp # file =E2=80=9Cdesc=E2=80= =9D] [addh # handle =E2=80=9Cdesc=E2=80=9D] bcfg driver|boot [rm #] bcfg driver|boot [mv # #] bcfg driver|boot [mod # =E2=80=9Cdesc=E2=80=9D] | [modf # file] | [mod= p # file] | [modh # handle] bcfg driver|boot [-opt # [[filename]|[=E2=80=9Ddata=E2=80=9D]] | [KeyData *]] It seems natural to assume from that that the "#" for all options is the "= same thing" and would be handled the same way. The comment for the -opt option does not indicate otherwise: -opt Modify the optional data associated with a driver or boot option. Followed either by the filename of the file which contains the binary data to be associated with the driver or boot option optional data, or else the quote-delimited data that will be associated with the driver or boot option optional data. In fact the use of the term "driver or boot option" for -opt and the other= options indicates that it is the same thing as for the other options (whic= h explicitly say that the "#" is a hexadecimal number), even if "#" isn't d= escribed explicitly in this case. I'm glad to hear there are other implementations, because given the disagr= eement over what the spec intends, it would be useful to compare them and c= onsider converging. Anyway, that's probably enough from me. :) Jonathan On 07/05/2019 21:04, Tim Lewis wrote: > Jonathan -- >=20 > The bcfg command pre-dates the UEFI shell specification. I know of at le= ast two non-EDK2 implementations, including one maintained by my company, t= hat are implemented to the specification. Server platforms that use the "ap= plication" style boot options can regularly run over 10 options.=20 >=20 > I believe the better alternative is to add a new option in the specific= ation and leave the existing syntax for -opt. >=20 > Thanks, >=20 > Tim >=20 > -----Original Message----- > From: Jonathan Watt > Sent: Tuesday, May 7, 2019 12:06 PM > To: Carsey, Jaben ; devel@edk2.groups.io;=20 > tim.lewis@insyde.com; Gao, Zhichao ; Ni, Ray=20 > > Cc: Bi, Dandan > Subject: Re: [edk2-devel] [PATCH v1 1/1]=20 > ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option >=20 > I should add, for me personally, once I noticed the inconsistency I chan= ged my scripts to use the "0x" prefix to avoid this real footgun. I imagine= that anyone else that may have encountered this would have done the same a= nd so, like me, wouldn't be affected by the change if it were to happen. >=20 > 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=20 >> two things are true!) hasn't been reported before would seem to=20 >> indicate this combination doesn't really happen/is rare in practice. >> >> Also, is TianoCore's bcfg the only implementation people are using?=20 >> If there are other implementations, would this bring TianoCore's=20 >> implementation into or out of line with them? That may impact whether t= he spec could/should change. >> >> On 07/05/2019 18:40, Carsey, Jaben wrote: >>> It will break existing scripts. Do you have such scripts in your envi= ronment 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 existin= g 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=20 >>>>> >>>> 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=20 >>>>> remove 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=20 >>>>>> Behalf 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 '-op= t' >>>>>>> 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 >=20 >=20