Agree. I would prepare this patch for push. Thanks, Zhichao
> -----Original Message----- > From: Carsey, Jaben > Sent: Saturday, August 3, 2019 5:24 AM > To: devel@edk2.groups.io; jw...@jwatt.org > Cc: tim.le...@insyde.com; Gao, Zhichao <zhichao....@intel.com>; Ni, Ray > <ray...@intel.com>; Bi, Dandan <dandan...@intel.com>; Rothman, Michael > A <michael.a.roth...@intel.com> > Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: > Fix '-opt' option > > I think we can push this in now. > > Zhichao, > Do you agree? If yes, can you prep this for merging? > > Thanks > -Jaben > > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Jonathan Watt > > Sent: Friday, August 02, 2019 1:28 PM > > To: devel@edk2.groups.io > > Cc: tim.le...@insyde.com; Carsey, Jaben <jaben.car...@intel.com>; Gao, > > Zhichao <zhichao....@intel.com>; Ni, Ray <ray...@intel.com>; Bi, > > Dandan <dandan...@intel.com> > > Subject: Re: [edk2-devel] [PATCH v1 1/1] > > ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option > > > > 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 (#44884): https://edk2.groups.io/g/devel/message/44884 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] -=-=-=-=-=-=-=-=-=-=-=-
