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

["Gao, Zhichao" <zhichao.gao@intel.com>]

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; jwatt@jwatt.org
> Cc: tim.lewis@insyde.com; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Rothman, Michael
> A <michael.a.rothman@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.lewis@insyde.com; Carsey, Jaben <jaben.carsey@intel.com>; Gao,
> > Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Bi,
> > Dandan <dandan.bi@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.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
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >
> > >
> > >
> > >
> > >
> >
> >
> >