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

["Jonathan Watt" <jwatt@jwatt.org>]

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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>
>>
>>
>>
>>
>>
> 
> 
> 
> 
>