It's been three months now since I contributed the patch. Could someone update 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 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 (#44867): https://edk2.groups.io/g/devel/message/44867 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] -=-=-=-=-=-=-=-=-=-=-=-
