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 (#40142): https://edk2.groups.io/g/devel/message/40142 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] -=-=-=-=-=-=-=-=-=-=-=-
