Tim, 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...
-Jaben > -----Original Message----- > From: Jonathan Watt [mailto:jw...@jwatt.org] > Sent: Tuesday, May 7, 2019 2:08 PM > To: devel@edk2.groups.io; tim.le...@insyde.com; Carsey, Jaben > <jaben.car...@intel.com>; Gao, Zhichao <zhichao....@intel.com>; Ni, Ray > <ray...@intel.com> > Cc: Bi, Dandan <dandan...@intel.com> > Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: > Fix '-opt' option > Importance: High > > 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 > 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 "clarifications" > in the past. > > > > As you've probably read in the other thread, it turns out that I (we) > > actually > did agree with your interpretation of the spec in our alternate > implementation 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. > > > > Best regards, > > Tim > > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > > Jonathan Watt > > Sent: Tuesday, May 7, 2019 1:51 PM > > To: Tim Lewis <tim.le...@insyde.com>; 'Carsey, Jaben' > > <jaben.car...@intel.com>; devel@edk2.groups.io; 'Gao, Zhichao' > > <zhichao....@intel.com>; 'Ni, Ray' <ray...@intel.com> > > Cc: 'Bi, Dandan' <dandan...@intel.com> > > 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 > home workstation and thought he'd try and remove the footgun to save > anyone else the same pain. I was specifically replying to the unconditional > statement "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 statement, > and to suggest some other things that 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 ends > with "that are implemented to the specification" sounds like you're saying > making 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 > actually 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 “desc”] > > [addh # handle “desc”] > > bcfg driver|boot [rm #] > > bcfg driver|boot [mv # #] > > bcfg driver|boot [mod # “desc”] | [modf # file] | [modp # file] | > > [modh # handle] > > bcfg driver|boot [-opt # [[filename]|[”data”]] | > > [KeyData <ScanCode UnicodeChar>*]] > > > > 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 (which > explicitly say that the "#" is a hexadecimal number), even if "#" 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 > 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 at > least two non-EDK2 implementations, including one maintained by my > company, that are implemented to the specification. Server platforms 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 > >> specification > and leave the existing syntax for -opt. > >> > >> Thanks, > >> > >> Tim > >> > >> -----Original Message----- > >> From: Jonathan Watt <jw...@jwatt.org> > >> Sent: Tuesday, May 7, 2019 12:06 PM > >> To: Carsey, Jaben <jaben.car...@intel.com>; devel@edk2.groups.io; > >> tim.le...@insyde.com; Gao, Zhichao <zhichao....@intel.com>; Ni, Ray > >> <ray...@intel.com> > >> Cc: Bi, Dandan <dandan...@intel.com> > >> 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. I > imagine > 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 > >>> scripts would need to be both omitting the "0x" prefix and be pass > >>> an option number greater than 9. The fact this very unexpected > >>> inconsistency (which will corrupt the wrong option when those same > >>> two things are true!) hasn't been reported before would 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 > >>> 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 > 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 <jaben.car...@intel.com>; > >>>>> Gao, Zhichao <zhichao....@intel.com>; Ni, Ray <ray...@intel.com>; > >>>>> jw...@jwatt.org > >>>>> Cc: Bi, Dandan <dandan...@intel.com> > >>>>> 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 shell scripts. In order to maintain that compatibility, > >>>>> it may be necessary to add a new option rather than trying to update > an existing one. > >>>>> > >>>>> Tim > >>>>> > >>>>> -----Original Message----- > >>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > >>>>> Carsey, Jaben > >>>>> Sent: Tuesday, May 7, 2019 7:36 AM > >>>>> To: Gao, Zhichao <zhichao....@intel.com>; devel@edk2.groups.io; > >>>>> Ni, Ray <ray...@intel.com>; jw...@jwatt.org > >>>>> Cc: Bi, Dandan <dandan...@intel.com> > >>>>> 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 <jaben.car...@intel.com> > >>>>> > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Gao, Zhichao > >>>>>> Sent: Tuesday, May 07, 2019 2:52 AM > >>>>>> To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; > >>>>>> jw...@jwatt.org > >>>>>> Cc: Carsey, Jaben <jaben.car...@intel.com>; Bi, Dandan > >>>>>> <dandan...@intel.com> > >>>>>> 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 <zhichao....@intel.com> > >>>>>> > >>>>>> But when I view the command in UEFI SHELL 2.2 spec: > >>>>>> ... > >>>>>> bcfg driver|boot [-opt # [[filename]|["data"]] | [KeyData > >>>>>> <ScanCode > >>>>>> UnicodeChar>*]] > >>>>>> ... > >>>>>> -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. > >>>>>> ... > >>>>>> > >>>>>> This description lack the comment of '#' parameter and that may > >>>>>> make the consumer confused. Usually consumers would regard it as > >>>>>> the same in other option, such as ' bcfg driver|boot [rm #]'. The > >>>>>> '#' is clearly descripted as a hexadecimal parameter: > >>>>>> rm > >>>>>> Remove an option. The # parameter lists the option number to > >>>>>> 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 > >>>>>>> Behalf Of > >>>>>> Ni, > >>>>>>> Ray > >>>>>>> Sent: Monday, May 6, 2019 10:02 PM > >>>>>>> To: jw...@jwatt.org; devel@edk2.groups.io > >>>>>>> Cc: Carsey, Jaben <jaben.car...@intel.com>; Bi, Dandan > >>>>>>> <dandan...@intel.com> > >>>>>>> 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: jw...@jwatt.org [mailto:jw...@jwatt.org] > >>>>>>>> Sent: Monday, May 6, 2019 9:03 PM > >>>>>>>> To: devel@edk2.groups.io > >>>>>>>> Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray > >>>>>>>> <ray...@intel.com> > >>>>>>>> Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '- > opt' > >>>>>>>> option > >>>>>>>> > >>>>>>>> From: Jonathan Watt <jw...@jwatt.org> > >>>>>>>> > >>>>>>>> For all other bcfg commands the "#" (option number) argument(s) > >>>>>>>> are treated as hexedecimal values regardless of 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 > >>>>>>>> that has been using a number without a "0x" prefix with other > >>>>>>>> bcfg commands finds that, on using that exact same number with > >>>>>>>> '-opt', it has this time unexpectedly been interpreted as a > >>>>>>>> decimal number and they have modified > >>>>>>>> (corrupted) an unrelated load option. For example, a user may > >>>>>>>> have been specifying "10" to other commands to have them act on > >>>>>>>> the 16th option (because simply "10", without any prefix, is > >>>>>>>> how 'bcfg boot dump' displayed the option number for the 16th > option). > >>>>>>>> Unfortunately for them, if they also use '-opt' with "10" it > >>>>>>>> would unexpectedly and inconsistently act on the 10th option. > >>>>>>>> > >>>>>>>> CC: Jaben Carsey <jaben.car...@intel.com> > >>>>>>>> CC: Ray Ni <ray...@intel.com> > >>>>>>>> Signed-off-by: Jonathan Watt <jw...@jwatt.org> > >>>>>>>> --- > >>>>>>>> > >>>>>>>> > >>>>> > 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 = ShellConvertStringToUint64(Walker, &Intermediate, > >>>>>>>> FALSE, TRUE); > >>>>>>>> + Status = ShellConvertStringToUint64(Walker, &Intermediate, > >>>>>>>> + TRUE, TRUE); > >>>>>>>> if (EFI_ERROR(Status) || (((UINT16)Intermediate) != > >>>>>>>> Intermediate) > >>>>>>>> || StrStr(Walker, L" ") == NULL || ((UINT16)Intermediate) > > >>>>>>>> ((UINT16)OrderCount)) { > >>>>>>>> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN > >>>>>>>> (STR_GEN_PARAM_INV), gShellBcfgHiiHandle, L"bcfg", L"Option > >>>>> Index"); > >>>>>>>> ShellStatus = SHELL_INVALID_PARAMETER; > >>>>>>>> -- > >>>>>>>> 2.21.0 > >>>>>>> > >>>>>>> > >>>>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>> > >>>> > >>> > >> > >> > >> > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40147): https://edk2.groups.io/g/devel/message/40147 Mute This Topic: https://groups.io/mt/31520134/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-