[edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option
Carsey, Jaben
jaben.carsey at intel.com
Fri Aug 2 21:23:33 UTC 2019
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 at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> Jonathan Watt
> Sent: Friday, August 02, 2019 1:28 PM
> To: devel at edk2.groups.io
> Cc: tim.lewis at insyde.com; Carsey, Jaben <jaben.carsey at intel.com>; Gao,
> Zhichao <zhichao.gao at intel.com>; Ni, Ray <ray.ni at intel.com>; Bi, Dandan
> <dandan.bi at 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 at intel.com>
> >> Sent: Tuesday, May 7, 2019 5:00 PM
> >> To: Jonathan Watt <jwatt at jwatt.org>; devel at edk2.groups.io;
> tim.lewis at insyde.com; Gao, Zhichao <zhichao.gao at intel.com>; Ni, Ray
> <ray.ni at intel.com>
> >> Cc: Bi, Dandan <dandan.bi at 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 at jwatt.org]
> >>> Sent: Tuesday, May 7, 2019 2:08 PM
> >>> To: devel at edk2.groups.io; tim.lewis at insyde.com; Carsey, Jaben
> >>> <jaben.carsey at intel.com>; Gao, Zhichao <zhichao.gao at intel.com>; Ni,
> >>> Ray <ray.ni at intel.com>
> >>> Cc: Bi, Dandan <dandan.bi at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> >>>> Jonathan Watt
> >>>> Sent: Tuesday, May 7, 2019 1:51 PM
> >>>> To: Tim Lewis <tim.lewis at insyde.com>; 'Carsey, Jaben'
> >>>> <jaben.carsey at intel.com>; devel at edk2.groups.io; 'Gao, Zhichao'
> >>>> <zhichao.gao at intel.com>; 'Ni, Ray' <ray.ni at intel.com>
> >>>> Cc: 'Bi, Dandan' <dandan.bi at 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 at jwatt.org>
> >>>>> Sent: Tuesday, May 7, 2019 12:06 PM
> >>>>> To: Carsey, Jaben <jaben.carsey at intel.com>; devel at edk2.groups.io;
> >>>>> tim.lewis at insyde.com; Gao, Zhichao <zhichao.gao at intel.com>; Ni,
> Ray
> >>>>> <ray.ni at intel.com>
> >>>>> Cc: Bi, Dandan <dandan.bi at 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 at edk2.groups.io [mailto:devel at edk2.groups.io] On
> >>> Behalf
> >>>>>>>> Of Tim Lewis
> >>>>>>>> Sent: Tuesday, May 07, 2019 9:20 AM
> >>>>>>>> To: devel at edk2.groups.io; Carsey, Jaben
> >>>>>>>> <jaben.carsey at intel.com>; Gao, Zhichao
> <zhichao.gao at intel.com>;
> >>>>>>>> Ni, Ray <ray.ni at intel.com>; jwatt at jwatt.org
> >>>>>>>> Cc: Bi, Dandan <dandan.bi at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf
> Of
> >>>>>>>> Carsey, Jaben
> >>>>>>>> Sent: Tuesday, May 7, 2019 7:36 AM
> >>>>>>>> To: Gao, Zhichao <zhichao.gao at intel.com>;
> devel at edk2.groups.io;
> >>>>>>>> Ni, Ray <ray.ni at intel.com>; jwatt at jwatt.org
> >>>>>>>> Cc: Bi, Dandan <dandan.bi at 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 at intel.com>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Gao, Zhichao
> >>>>>>>>> Sent: Tuesday, May 07, 2019 2:52 AM
> >>>>>>>>> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>;
> >>>>>>>>> jwatt at jwatt.org
> >>>>>>>>> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Bi, Dandan
> >>>>>>>>> <dandan.bi at 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 at 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 at edk2.groups.io [mailto:devel at edk2.groups.io]
> On
> >>>>>>>>>> Behalf Of
> >>>>>>>>> Ni,
> >>>>>>>>>> Ray
> >>>>>>>>>> Sent: Monday, May 6, 2019 10:02 PM
> >>>>>>>>>> To: jwatt at jwatt.org; devel at edk2.groups.io
> >>>>>>>>>> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Bi, Dandan
> >>>>>>>>>> <dandan.bi at 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 at jwatt.org [mailto:jwatt at jwatt.org]
> >>>>>>>>>>> Sent: Monday, May 6, 2019 9:03 PM
> >>>>>>>>>>> To: devel at edk2.groups.io
> >>>>>>>>>>> Cc: Carsey, Jaben <jaben.carsey at intel.com>; Ni, Ray
> >>>>>>>>>>> <ray.ni at intel.com>
> >>>>>>>>>>> Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib:
> Fix
> >>>>>>>>>>> '-
> >>> opt'
> >>>>>>>>>>> option
> >>>>>>>>>>>
> >>>>>>>>>>> From: Jonathan Watt <jwatt at 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 at intel.com>
> >>>>>>>>>>> CC: Ray Ni <ray.ni at intel.com>
> >>>>>>>>>>> Signed-off-by: Jonathan Watt <jwatt at 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 (#44869): https://edk2.groups.io/g/devel/message/44869
Mute This Topic: https://groups.io/mt/31520134/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list