From: "Jonathan Watt" <jwatt@jwatt.org>
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
Date: Tue, 11 Jun 2019 22:53:45 +0100 [thread overview]
Message-ID: <0af6d246-c6df-26ee-d407-9ba56953b17b@jwatt.org> (raw)
In-Reply-To: <019301d50532$265d73a0$73185ae0$@insyde.com>
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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>
>
>
>
>
>
next prev parent reply other threads:[~2019-06-11 21:53 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-06 13:02 [PATCH v1 0/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option number handling Jonathan Watt
2019-05-06 13:02 ` [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option Jonathan Watt
2019-05-06 14:02 ` Ni, Ray
2019-05-07 9:51 ` [edk2-devel] " Gao, Zhichao
2019-05-07 14:35 ` Carsey, Jaben
2019-05-07 15:05 ` Dandan Bi
2019-05-07 16:20 ` Tim Lewis
2019-05-07 17:40 ` Carsey, Jaben
2019-05-07 17:43 ` Tim Lewis
2019-05-07 19:00 ` Jonathan Watt
2019-05-07 19:06 ` Jonathan Watt
2019-05-07 20:04 ` Tim Lewis
2019-05-07 20:30 ` Jim.Dailey
2019-05-07 20:48 ` Tim Lewis
2019-05-07 20:52 ` Jim.Dailey
2019-05-07 21:04 ` Jonathan Watt
2019-05-07 20:51 ` Jonathan Watt
2019-05-07 21:02 ` Tim Lewis
2019-05-07 21:07 ` Jonathan Watt
2019-05-07 23:59 ` Carsey, Jaben
2019-05-08 0:08 ` Tim Lewis
2019-06-11 21:53 ` Jonathan Watt [this message]
[not found] ` <15A74385D3E8CBEB.24554@groups.io>
2019-08-02 20:28 ` Jonathan Watt
2019-08-02 21:23 ` Carsey, Jaben
2019-08-05 0:51 ` Gao, Zhichao
2019-08-12 16:31 ` Jonathan Watt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0af6d246-c6df-26ee-d407-9ba56953b17b@jwatt.org \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox