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=Udderxa7; spf=softfail (domain: jwatt.org, ip: 23.83.212.15, mailfrom: jwatt@jwatt.org) Received: from beetle.elm.relay.mailchannels.net (beetle.elm.relay.mailchannels.net [23.83.212.15]) by groups.io with SMTP; Tue, 11 Jun 2019 14:53:57 -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 7C31C2C158B; Tue, 11 Jun 2019 21:53:56 +0000 (UTC) Received: from pdx1-sub0-mail-a90.g.dreamhost.com (100-96-89-88.trex.outbound.svc.cluster.local [100.96.89.88]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 633832C1684; Tue, 11 Jun 2019 21:53:55 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|jwatt@jwatt.org Received: from pdx1-sub0-mail-a90.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, 11 Jun 2019 21:53:56 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|jwatt@jwatt.org X-MailChannels-Auth-Id: dreamhost X-White-Fearful: 0f89ae1334637a28_1560290036289_3095318727 X-MC-Loop-Signature: 1560290036289:1626293056 X-MC-Ingress-Time: 1560290036288 Received: from pdx1-sub0-mail-a90.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a90.g.dreamhost.com (Postfix) with ESMTP id 497B57F81E; Tue, 11 Jun 2019 14:53:51 -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=PZORxtD EXEU4LyuoD4brN49gUP8=; b=Udderxa78jBmPeWUL/MHL+NmJAEGi/BczoI+00y DxDDZ0y52CJBD8jjqvinHb7YV1mS+YbNtD9g9AyDKxpz3ltspZLxJB2zMK/335lH 8kwtVBgotxz31+9W4Pfneyo39U2LJlHUjY7z4ZNN+HQry7AUdjeRDGu5MPmHo/Bo tzmA= 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-a90.g.dreamhost.com (Postfix) with ESMTPSA id C92EC7F822; Tue, 11 Jun 2019 14:53:46 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option To: devel@edk2.groups.io Cc: tim.lewis@insyde.com, "'Carsey, Jaben'" , "'Gao, Zhichao'" , "'Ni, Ray'" , "'Bi, Dandan'" References: <019301d50532$265d73a0$73185ae0$@insyde.com> X-DH-BACKEND: pdx1-sub0-mail-a90 From: "Jonathan Watt" Openpgp: preference=signencrypt Message-ID: <0af6d246-c6df-26ee-d407-9ba56953b17b@jwatt.org> Date: Tue, 11 Jun 2019 22:53:45 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 MIME-Version: 1.0 In-Reply-To: <019301d50532$265d73a0$73185ae0$@insyde.com> X-VR-OUT-STATUS: OK X-VR-OUT-SCORE: -100 X-VR-OUT-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeduuddrudehiecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucggtfgfnhhsuhgsshgtrhhisggvpdfftffgtefojffquffvnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenucfjughrpefuvfhfhffkffgfgggjtgfgsehtkeertddtfeejnecuhfhrohhmpeflohhnrghthhgrnhcuhggrthhtuceojhifrghtthesjhifrghtthdrohhrgheqnecukfhppeekvddrgedtrddutdeirddugeelnecurfgrrhgrmhepmhhouggvpehsmhhtphdphhgvlhhopeforggtuehoohhkqdfrrhhoqdeirdhlohgtrghlughomhgrihhnpdhinhgvthepkedvrdegtddruddtiedrudegledprhgvthhurhhnqdhprghthheplfhonhgrthhhrghnucghrghtthcuoehjfigrthhtsehjfigrthhtrdhorhhgqedpmhgrihhlfhhrohhmpehjfigrthhtsehjfigrthhtrdhorhhgpdhnrhgtphhtthhopeguvghvvghlsegvughkvddrghhrohhuphhsrdhiohenucevlhhushhtvghrufhiiigvpedt Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Since I haven't contributed before I'm not sure what the timeline for these things generally is. It's been a month though. Can the patch be pushed now= ? Regards, Jonathan On 08/05/2019 01:08, Tim Lewis wrote: > Yes, I would support it. Tim >=20 > -----Original Message----- > From: Carsey, Jaben =20 > Sent: Tuesday, May 7, 2019 5:00 PM > To: Jonathan Watt ; devel@edk2.groups.io; tim.lewis@ins= yde.com; Gao, Zhichao ; Ni, Ray > Cc: Bi, Dandan > Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLi= b: Fix '-opt' option >=20 > Tim, >=20 > Does this mean you would support such an errata? I would like to get the= spec to a place where the behavior is at least nailed down one way or the = other... >=20 > -Jaben >=20 >> -----Original Message----- >> From: Jonathan Watt [mailto:jwatt@jwatt.org] >> Sent: Tuesday, May 7, 2019 2:08 PM >> To: devel@edk2.groups.io; tim.lewis@insyde.com; Carsey, Jaben=20 >> ; Gao, Zhichao ; Ni,=20 >> Ray >> Cc: Bi, Dandan >> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandL= ib: >> Fix '-opt' option >> Importance: High >> >> No apologies necessary! Raising compatibility concerns is very valid.= =20 >> As I said, I just wanted to provide some other considerations I saw to= =20 >> weigh in the decision. >> >> All the best, >> Jonathan >> >> On 07/05/2019 22:02, Tim Lewis wrote: >>> Jonathan -- >>> >>> My apologies. I jumped because we've been bitten by shell "clarificati= ons" >> in the past. >>> >>> As you've probably read in the other thread, it turns out that I=20 >>> (we) actually >> did agree with your interpretation of the spec in our alternate=20 >> implementation and have been using it that way for 2+ years. And it=20 >> didn't cause us grief with our other product which does use an EDK2-der= ived shell. >>> >>> Best regards, >>> Tim >>> >>> -----Original Message----- >>> From: devel@edk2.groups.io On Behalf Of=20 >>> 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/UefiShellBcfgCommandLib: Fix '-opt' option >>> >>> Hi Tim, >>> >>> For context, I'm just some random guy who tripped over this issue on= =20 >>> his >> home workstation and thought he'd try and remove the footgun to save=20 >> anyone else the same pain. I was specifically replying to the=20 >> unconditional statement "It will break existing scripts." (not made by= =20 >> you) to provide what I hope was some qualification and balance to the= =20 >> face value of that statement, and to suggest some other things that=20 >> should be considered. As far as deciding what the best resolution is, I= 'm not qualified for that. >>> >>> I am curious about one thing though. The sentence you wrote that=20 >>> ends >> with "that are implemented to the specification" sounds like you're=20 >> saying making the proposed change would violate the specification.=20 >> That does not seem to be the case from my reading, and my reading=20 >> would be that it would actually make it do what most people would=20 >> 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] | = [modp # 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= =20 >>> 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= =20 >>> other >> options indicates that it is the same thing as for the other options=20 >> (which explicitly say that the "#" is a hexadecimal number), even if=20 >> "#" isn't described explicitly in this case. >>> >>> I'm glad to hear there are other implementations, because given the >> disagreement over what the spec intends, it would be useful to compare= =20 >> them and consider converging. >>> >>> Anyway, that's probably enough from me. :) >>> >>> Jonathan >>> >>> On 07/05/2019 21:04, Tim Lewis wrote: >>>> Jonathan -- >>>> >>>> The bcfg command pre-dates the UEFI shell specification. I know of=20 >>>> at >> least two non-EDK2 implementations, including one maintained by my=20 >> company, that are implemented to the specification. Server platforms=20 >> that use the "application" style boot options can regularly run over 10= options. >>>> >>>> I believe the better alternative is to add a new option in the=20 >>>> specification >> 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] >>>> ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option >>>> >>>> I should add, for me personally, once I noticed the inconsistency I >> changed my scripts to use the "0x" prefix to avoid this real footgun.= =20 >> I imagine that anyone else that may have encountered this would have=20 >> 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=20 >>>>> happen scripts would need to be both omitting the "0x" prefix and=20 >>>>> be pass an option number greater than 9. The fact this very=20 >>>>> unexpected inconsistency (which will corrupt the wrong option when= =20 >>>>> those same two things are true!) hasn't been reported before would= =20 >>>>> seem to indicate this combination doesn't really happen/is rare in p= ractice. >>>>> >>>>> Also, is TianoCore's bcfg the only implementation people are using? >>>>> If there are other implementations, would this bring TianoCore's=20 >>>>> implementation into or out of line with them? That may impact=20 >>>>> 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 >> environment dependent on this parameter? >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On >> Behalf >>>>>>> Of Tim Lewis >>>>>>> Sent: Tuesday, May 07, 2019 9:20 AM >>>>>>> To: devel@edk2.groups.io; Carsey, Jaben=20 >>>>>>> ; Gao, Zhichao ;=20 >>>>>>> Ni, Ray ; 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=20 >>>>>>> existing shell scripts. In order to maintain that compatibility,= =20 >>>>>>> it may be necessary to add a new option rather than trying to=20 >>>>>>> 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;=20 >>>>>>> Ni, 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=20 >>>>>>>> as the same in other option, such as ' bcfg driver|boot [rm=20 >>>>>>>> #]'. The '#' 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= =20 >>>>>>>>>> '- >> opt' >>>>>>>>>> option >>>>>>>>>> >>>>>>>>>> From: Jonathan Watt >>>>>>>>>> >>>>>>>>>> For all other bcfg commands the "#" (option number)=20 >>>>>>>>>> argument(s) are treated as hexedecimal values regardless of=20 >>>>>>>>>> whether or not 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=20 >>>>>>>>>> with '-opt', it has this time unexpectedly been interpreted=20 >>>>>>>>>> as a decimal number and they have modified >>>>>>>>>> (corrupted) an unrelated load option. For example, a user=20 >>>>>>>>>> may have been specifying "10" to other commands to have them=20 >>>>>>>>>> act on the 16th option (because simply "10", without any=20 >>>>>>>>>> prefix, is how 'bcfg boot dump' displayed the option number=20 >>>>>>>>>> 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