From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@jwatt.org header.s=jwatt.org header.b=yMdshWgm; spf=softfail (domain: jwatt.org, ip: 23.83.214.99, mailfrom: jwatt@jwatt.org) Received: from lavender.maple.relay.mailchannels.net (lavender.maple.relay.mailchannels.net [23.83.214.99]) by groups.io with SMTP; Tue, 07 May 2019 14:07:49 -0700 X-Sender-Id: dreamhost|x-authsender|jwatt@jwatt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 75C085E1FAB; Tue, 7 May 2019 21:07:44 +0000 (UTC) Received: from pdx1-sub0-mail-a62.g.dreamhost.com (100-96-78-10.trex.outbound.svc.cluster.local [100.96.78.10]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id DB34E5E20DA; Tue, 7 May 2019 21:07:43 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|jwatt@jwatt.org Received: from pdx1-sub0-mail-a62.g.dreamhost.com ([TEMPUNAVAIL]. [64.90.62.162]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384) by 0.0.0.0:2500 (trex/5.17.2); Tue, 07 May 2019 21:07:44 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|jwatt@jwatt.org X-MailChannels-Auth-Id: dreamhost X-Society-Celery: 791efb7856886dd8_1557263264299_1411096921 X-MC-Loop-Signature: 1557263264299:1663414571 X-MC-Ingress-Time: 1557263264298 Received: from pdx1-sub0-mail-a62.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a62.g.dreamhost.com (Postfix) with ESMTP id 7D27F7FEA8; Tue, 7 May 2019 14:07:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=jwatt.org; h=subject:to:cc :references:from:message-id:date:mime-version:in-reply-to :content-type:content-transfer-encoding; s=jwatt.org; bh=6FFI3Tw fXpEl88hOKghSYv1iGkk=; b=yMdshWgmOLBe6EnflCini7Rzx6tgXjTZ7zTXoJF kF9OV1/RVULeAXarkxXh3w2G1roDhT4t+IOBYFEyZIx21KTZfwz95RlBPaG9+XfH hJR7wqoOysgAX418IPMvFiLJif3VcRNSAaGUC54Mg0VUl/mhHfwCQvHxyFcyKv4W m0ho= Received: from MacBook-Pro-6.localdomain (cpc120804-nrwh12-2-0-cust660.4-4.cable.virginm.net [82.40.106.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: jwatt@jwatt.org) by pdx1-sub0-mail-a62.g.dreamhost.com (Postfix) with ESMTPSA id B3DC57FF7D; Tue, 7 May 2019 14:07:36 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option To: devel@edk2.groups.io, tim.lewis@insyde.com, "'Carsey, Jaben'" , "'Gao, Zhichao'" , "'Ni, Ray'" Cc: "'Bi, Dandan'" References: <001601d50518$3a0ea550$ae2beff0$@insyde.com> X-DH-BACKEND: pdx1-sub0-mail-a62 From: "Jonathan Watt" Openpgp: preference=signencrypt Message-ID: Date: Tue, 7 May 2019 22:07:35 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <001601d50518$3a0ea550$ae2beff0$@insyde.com> X-VR-OUT-STATUS: OK X-VR-OUT-SCORE: -100 X-VR-OUT-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeduuddrkedtgdduiedvucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuggftfghnshhusghstghrihgsvgdpffftgfetoffjqffuvfenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhepuffvfhfhkffffgggjggtgfesthekredttdefjeenucfhrhhomheplfhonhgrthhhrghnucghrghtthcuoehjfigrthhtsehjfigrthhtrdhorhhgqeenucfkphepkedvrdegtddruddtiedrudegleenucfrrghrrghmpehmohguvgepshhmthhppdhhvghlohepofgrtgeuohhokhdqrfhrohdqiedrlhhotggrlhguohhmrghinhdpihhnvghtpeekvddrgedtrddutdeirddugeelpdhrvghtuhhrnhdqphgrthhhpeflohhnrghthhgrnhcuhggrthhtuceojhifrghtthesjhifrghtthdrohhrgheqpdhmrghilhhfrhhomhepjhifrghtthesjhifrghtthdrohhrghdpnhhrtghpthhtohepuggvvhgvlhesvggukhdvrdhgrhhouhhpshdrihhonecuvehluhhsthgvrhfuihiivgeptd Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable No apologies necessary! Raising compatibility concerns is very valid. As I = said, I just wanted to provide some other considerations I saw to weigh in the d= ecision. All the best, Jonathan On 07/05/2019 22:02, Tim Lewis wrote: > Jonathan -- >=20 > My apologies. I jumped because we've been bitten by shell "clarification= s" in the past. >=20 > As you've probably read in the other thread, it turns out that I (we) ac= tually did agree with your interpretation of the spec in our alternate impl= ementation and have been using it that way for 2+ years. And it didn't caus= e us grief with our other product which does use an EDK2-derived shell.=20 >=20 > Best regards, > Tim >=20 > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Jonathan = Watt > Sent: Tuesday, May 7, 2019 1:51 PM > To: Tim Lewis ; 'Carsey, Jaben' ; devel@edk2.groups.io; 'Gao, Zhichao' ; 'Ni, = Ray' > Cc: 'Bi, Dandan' > Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLi= b: Fix '-opt' option >=20 > Hi Tim, >=20 > For context, I'm just some random guy who tripped over this issue on his= home workstation and thought he'd try and remove the footgun to save anyon= e else the same pain. I was specifically replying to the unconditional stat= ement "It will break existing scripts." (not made by you) to provide what I= hope was some qualification and balance to the face value of that statemen= t, and to suggest some other things that should be considered. As far as de= ciding what the best resolution is, I'm not qualified for that. >=20 > I am curious about one thing though. The sentence you wrote that ends wi= th "that are implemented to the specification" sounds like you're saying ma= king the proposed change would violate the specification. That does not see= m to be the case from my reading, and my reading would be that it would act= ually make it do what most people would expect from reading the specificati= on. >=20 > Specifically, the usage block for bcfg in the specification says: >=20 > 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] | [m= odp # file] | > [modh # handle] > bcfg driver|boot [-opt # [[filename]|[=E2=80=9Ddata=E2=80=9D]] | > [KeyData *]] >=20 > It seems natural to assume from that that the "#" for all options is the= "same thing" and would be handled the same way. >=20 > The comment for the -opt option does not indicate otherwise: >=20 > -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. >=20 > In fact the use of the term "driver or boot option" for -opt and the oth= er options indicates that it is the same thing as for the other options (wh= ich explicitly say that the "#" is a hexadecimal number), even if "#" isn't= described explicitly in this case. >=20 > I'm glad to hear there are other implementations, because given the disa= greement over what the spec intends, it would be useful to compare them and= consider converging. >=20 > Anyway, that's probably enough from me. :) >=20 > Jonathan >=20 > On 07/05/2019 21:04, Tim Lewis wrote: >> Jonathan -- >> >> The bcfg command pre-dates the UEFI shell specification. I know of at l= east two non-EDK2 implementations, including one maintained by my company, = that are implemented to the specification. Server platforms that use the "a= pplication" style boot options can regularly run over 10 options.=20 >> >> I believe the better alternative is to add a new option in the specifi= cation 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;=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 >> >> I should add, for me personally, once I noticed the inconsistency I cha= nged my scripts to use the "0x" prefix to avoid this real footgun. I imagin= e that 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=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 = the 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 env= ironment 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 existi= ng 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 '-o= pt' >>>>>>>> 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 it= s "#" >>>>>>>> (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 >=20 >=20 >=20 >=20 >=20