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=q3lb3GAZ; spf=softfail (domain: jwatt.org, ip: 23.83.213.18, mailfrom: jwatt@jwatt.org) Received: from bisque.larch.relay.mailchannels.net (bisque.larch.relay.mailchannels.net [23.83.213.18]) by groups.io with SMTP; Fri, 02 Aug 2019 13:28:41 -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 AE262501A54; Fri, 2 Aug 2019 20:28:40 +0000 (UTC) Received: from pdx1-sub0-mail-a73.g.dreamhost.com (100-96-11-214.trex.outbound.svc.cluster.local [100.96.11.214]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 9FEBE501B6E; Fri, 2 Aug 2019 20:28:39 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|jwatt@jwatt.org Received: from pdx1-sub0-mail-a73.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.5); Fri, 02 Aug 2019 20:28:40 +0000 X-MC-Relay: Junk X-MailChannels-SenderId: dreamhost|x-authsender|jwatt@jwatt.org X-MailChannels-Auth-Id: dreamhost X-Tangy-Shade: 054838aa1235763e_1564777720508_2719850689 X-MC-Loop-Signature: 1564777720508:302299098 X-MC-Ingress-Time: 1564777720508 Received: from pdx1-sub0-mail-a73.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a73.g.dreamhost.com (Postfix) with ESMTP id 675AE835C9; Fri, 2 Aug 2019 13:28:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=jwatt.org; h=subject:from :to:cc:reply-to:references:message-id:date:mime-version :in-reply-to:content-type:content-transfer-encoding; s= jwatt.org; bh=QUz8QTofhsUKW2gxB3kd8Gq2NTw=; b=q3lb3GAZHPGW1jEPf2 VRdpGD7Ojj49bq2nAzyxQ/fxbVUJgaaSaqwKzwdSbuw1yhDik2tKjguK4Gwc7yC1 0K5f3xKD2AN8CwPJDGpGACH8ZeLjkafXuXnc8ZPISSD/KopFtHQDtxumzjRfvXbN o6ablSBuZumXjP3e7KotqEwXk= Received: from MacBook-Pro-6.local (unknown [185.93.2.168]) (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-a73.g.dreamhost.com (Postfix) with ESMTPSA id 50F56835C7; Fri, 2 Aug 2019 13:28:29 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option X-DH-BACKEND: pdx1-sub0-mail-a73 From: "Jonathan Watt" To: devel@edk2.groups.io Cc: tim.lewis@insyde.com, "'Carsey, Jaben'" , "'Gao, Zhichao'" , "'Ni, Ray'" , "'Bi, Dandan'" Reply-To: devel@edk2.groups.io, jwatt@jwatt.org References: <019301d50532$265d73a0$73185ae0$@insyde.com> <15A74385D3E8CBEB.24554@groups.io> Openpgp: preference=signencrypt Message-ID: <1fa21ab2-1afe-b578-603f-7d908b6daad4@jwatt.org> Date: Fri, 2 Aug 2019 21:28:27 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <15A74385D3E8CBEB.24554@groups.io> X-VR-OUT-STATUS: OK X-VR-OUT-SCORE: -100 X-VR-OUT-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeduvddrleelgdduvdduucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuggftfghnshhusghstghrihgsvgdpffftgfetoffjqffuvfenuceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhepuffhvfhrfhfkffgfgggjtgfgsehtkeertddtfeejnecuhfhrohhmpeflohhnrghthhgrnhcuhggrthhtuceojhifrghtthesjhifrghtthdrohhrgheqnecukfhppedukeehrdelfedrvddrudeikeenucfrrghrrghmpehmohguvgepshhmthhppdhhvghlohepofgrtgeuohhokhdqrfhrohdqiedrlhhotggrlhdpihhnvghtpedukeehrdelfedrvddrudeikedprhgvthhurhhnqdhprghthheplfhonhgrthhhrghnucghrghtthcuoehjfigrthhtsehjfigrthhtrdhorhhgqedpmhgrihhlfhhrohhmpehjfigrthhtsehjfigrthhtrdhorhhgpdhnrhgtphhtthhopeguvghvvghlsegvughkvddrghhrohhuphhsrdhiohenucevlhhushhtvghrufhiiigvpedt Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable It's been three months now since I contributed the patch. Could someone upd= ate me on the progress on getting it landed? On 11/06/2019 22:53, Jonathan Watt wrote: > Since I haven't contributed before I'm not sure what the timeline for th= ese > things generally is. It's been a month though. Can the patch be pushed n= ow? >=20 > Regards, > Jonathan >=20 > On 08/05/2019 01:08, Tim Lewis wrote: >> Yes, I would support it. Tim >> >> -----Original Message----- >> From: Carsey, Jaben =20 >> Sent: Tuesday, May 7, 2019 5:00 PM >> To: Jonathan Watt ; devel@edk2.groups.io; tim.lewis@in= syde.com; Gao, Zhichao ; Ni, Ray >> Cc: Bi, Dandan >> Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandL= ib: Fix '-opt' option >> >> Tim, >> >> Does this mean you would support such an errata? I would like to get th= e spec to a place where the behavior is at least nailed down one way or the= other... >> >> -Jaben >> >>> -----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/UefiShellBcfgCommand= Lib: >>> 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 "clarificat= ions" >>> 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-de= rived 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 1= 0 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 i= t 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 = practice. >>>>>> >>>>>> 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