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 > > -----Original Message----- > From: Carsey, Jaben <jaben.car...@intel.com> > Sent: Tuesday, May 7, 2019 5:00 PM > To: Jonathan Watt <jw...@jwatt.org>; 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 > > 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 (#42226): https://edk2.groups.io/g/devel/message/42226 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] -=-=-=-=-=-=-=-=-=-=-=-
