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.41, mailfrom: tim.lewis@insyde.com) Received: from out02.hibox.biz (out02.hibox.biz [210.71.195.41]) by groups.io with SMTP; Tue, 07 May 2019 17:08:26 -0700 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2DpAABfHNJc/ww0GKxkGwEBAQEDAQE?= =?us-ascii?q?BBwMBAQGBZYJ6chIog0hIiHuMKTUBgwOLBRGESCOHExAjCQGEOgQCAoI5OBM?= =?us-ascii?q?BAwEBBQEBAQECbRwMhUoBAQUIAgQVKwgIBxQMAQUGAw0EBAEBAQICIwMCGxg?= =?us-ascii?q?VCQgCBAESCwUNBIMCggoPrUCBLxoCg2oBKwERQ4UngQsngQ9Ri0U/gRGCFH4?= =?us-ascii?q?+hA0hgyCCWASKdwcZAYg4kw1lBwICgglZAYVDhDKFU4ItG4IQYoVig10DD4k?= =?us-ascii?q?UjCSBIYUsjlGBZiGBVnCDPAk2gVwXFIIng3WCBYUzAVYiMIEhCBONS4JSAQE?= X-IronPort-AV: E=Sophos;i="5.60,443,1549900800"; d="scan'208";a="13037213" Received: from unknown (HELO hb3-BKT202.hibox.biz) ([172.24.52.12]) by out02.hibox.biz with ESMTP; 08 May 2019 08:08:23 +0800 Received: from unknown (HELO hb3-BKT102.hibox.biz) ([172.24.51.12]) by hb3-BKT202.hibox.biz with ESMTP; 08 May 2019 08:08:22 +0800 Received: from unknown (HELO hb3-IN03.hibox.biz) ([172.24.12.13]) by hb3-BKT102.hibox.biz with ESMTP; 08 May 2019 08:08:22 +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: 23883575 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-IN03.hibox.biz with ESMTP/TLS/AES256-SHA; 08 May 2019 08:08:21 +0800 From: "Tim Lewis" To: "'Carsey, Jaben'" , "'Jonathan Watt'" , , "'Gao, Zhichao'" , "'Ni, Ray'" Cc: "'Bi, Dandan'" In-Reply-To: Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option Date: Tue, 7 May 2019 17:08:18 -0700 Message-ID: <019301d50532$265d73a0$73185ae0$@insyde.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQJkMvt+gx5XMQrWX1VopM/wD/XvkKVB/kVQ Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: en-us 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@insyd= e.com; Gao, Zhichao ; Ni, Ray Cc: Bi, Dandan Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib:= Fix '-opt' option Tim, Does this mean you would support such an errata? I would like to get the s= pec to a place where the behavior is at least nailed down one way or the ot= her... -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/UefiShellBcfgCommandLi= b: > Fix '-opt' option > Importance: High >=20 > 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. >=20 > All the best, > Jonathan >=20 > 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-deri= ved 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 > > > >