Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

["Tim Lewis" <tim.lewis@insyde.com>]

Yes, I would support it. Tim

-----Original Message-----
From: Carsey, Jaben <jaben.carsey@intel.com> 
Sent: Tuesday, May 7, 2019 5:00 PM
To: Jonathan Watt <jwatt@jwatt.org>; devel@edk2.groups.io; tim.lewis@insyde.com; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: Bi, Dandan <dandan.bi@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:jwatt@jwatt.org]
> Sent: Tuesday, May 7, 2019 2:08 PM
> To: devel@edk2.groups.io; tim.lewis@insyde.com; Carsey, Jaben 
> <jaben.carsey@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ni, 
> Ray <ray.ni@intel.com>
> Cc: Bi, Dandan <dandan.bi@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.lewis@insyde.com>; 'Carsey, Jaben'
> > <jaben.carsey@intel.com>; devel@edk2.groups.io; 'Gao, Zhichao'
> > <zhichao.gao@intel.com>; 'Ni, Ray' <ray.ni@intel.com>
> > Cc: 'Bi, Dandan' <dandan.bi@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 <jwatt@jwatt.org>
> >> Sent: Tuesday, May 7, 2019 12:06 PM
> >> To: Carsey, Jaben <jaben.carsey@intel.com>; devel@edk2.groups.io; 
> >> tim.lewis@insyde.com; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray 
> >> <ray.ni@intel.com>
> >> Cc: Bi, Dandan <dandan.bi@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.carsey@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; 
> >>>>> Ni, Ray <ray.ni@intel.com>; jwatt@jwatt.org
> >>>>> Cc: Bi, Dandan <dandan.bi@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.gao@intel.com>; devel@edk2.groups.io; 
> >>>>> Ni, Ray <ray.ni@intel.com>; jwatt@jwatt.org
> >>>>> Cc: Bi, Dandan <dandan.bi@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.carsey@intel.com>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Gao, Zhichao
> >>>>>> Sent: Tuesday, May 07, 2019 2:52 AM
> >>>>>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; 
> >>>>>> jwatt@jwatt.org
> >>>>>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Bi, Dandan 
> >>>>>> <dandan.bi@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.gao@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: jwatt@jwatt.org; devel@edk2.groups.io
> >>>>>>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Bi, Dandan 
> >>>>>>> <dandan.bi@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: jwatt@jwatt.org [mailto:jwatt@jwatt.org]
> >>>>>>>> Sent: Monday, May 6, 2019 9:03 PM
> >>>>>>>> To: devel@edk2.groups.io
> >>>>>>>> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray 
> >>>>>>>> <ray.ni@intel.com>
> >>>>>>>> Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix 
> >>>>>>>> '-
> opt'
> >>>>>>>> option
> >>>>>>>>
> >>>>>>>> From: Jonathan Watt <jwatt@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.carsey@intel.com>
> >>>>>>>> CC: Ray Ni <ray.ni@intel.com>
> >>>>>>>> Signed-off-by: Jonathan Watt <jwatt@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
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
> > 
> >
> >