[PATCH v1 0/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option number handling

[PATCH v1 0/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option number handling

[Jonathan Watt]

From: Jonathan Watt <jwatt@jwatt.org>

Fixes `bcfg boot -opt #` handling of the "#" option number to be
consistent with the other commands.  Removes a footgun that bit me
and corrupted a different boot option than the one I meant to edit.

https://github.com/jwatt/edk2/tree/bcfg_args_fix

Jonathan Watt (1):
  ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

 ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.21.0

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

[Jonathan Watt]

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

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

[Ni, Ray]

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

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

[Gao, Zhichao]

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

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

[Carsey, Jaben]

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

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

[Dandan Bi]

Also agree to update Shell Spec to add description for "#" in "-opt" part to make it consistent with other options since this patch has updated the code behavior to be consistent.

For the patch
Reviewed-by: Bi Dandan <dandan.bi@intel.com>


Thanks,
Dandan

> -----Original Message-----
> From: Carsey, Jaben
> Sent: Tuesday, May 07, 2019 10:36 PM
> 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
> > >
> > >
> > > 

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

[Tim Lewis]

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

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

[Carsey, Jaben]

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

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

[Tim Lewis]

Yes. And we have been recommending the usage of bcfg to our customers for
years.

Tim

-----Original Message-----
From: Carsey, Jaben <jaben.carsey@intel.com> 
Sent: Tuesday, May 7, 2019 10:41 AM
To: devel@edk2.groups.io; tim.lewis@insyde.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

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

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

[Jonathan Watt]

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

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

[Jonathan Watt]

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

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

[Tim Lewis]

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

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

[Jim.Dailey]

Tim,

Out of curiosity, what does the specification you refer to that was used to write the non-EDK2 implementations say about the -opt switch?

Regards,
Jim

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Tim Lewis
Sent: Tuesday, May 7, 2019 3:04 PM
To: 'Jonathan Watt'; 'Carsey, Jaben'; devel@edk2.groups.io; 'Gao, Zhichao'; 'Ni, Ray'
Cc: 'Bi, Dandan'
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

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

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

[Tim Lewis]

Jim --

Well, speaking of shooting-oneself-in-the-foot, it turns out that our non-EDK2 implementation followed the recommendation in the patch. 

I agree that the spec is ambiguous and, it turns out that our largest use case already uses the recommended behavior.

Sorry to one and all.

Tim

-----Original Message-----
From: Jim.Dailey@dell.com <Jim.Dailey@dell.com> 
Sent: Tuesday, May 7, 2019 1:30 PM
To: devel@edk2.groups.io; tim.lewis@insyde.com
Cc: dandan.bi@intel.com; jwatt@jwatt.org; jaben.carsey@intel.com; zhichao.gao@intel.com; ray.ni@intel.com
Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

Tim,

Out of curiosity, what does the specification you refer to that was used to write the non-EDK2 implementations say about the -opt switch?

Regards,
Jim

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Tim Lewis
Sent: Tuesday, May 7, 2019 3:04 PM
To: 'Jonathan Watt'; 'Carsey, Jaben'; devel@edk2.groups.io; 'Gao, Zhichao'; 'Ni, Ray'
Cc: 'Bi, Dandan'
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

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

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

[Jim.Dailey]

That is good news!  Mostly because assuming it was decimal should make no sense to an engineer! :-)

-----Original Message-----
From: Tim Lewis <tim.lewis@insyde.com> 
Sent: Tuesday, May 7, 2019 3:49 PM
To: Dailey, Jim; devel@edk2.groups.io
Cc: dandan.bi@intel.com; jwatt@jwatt.org; jaben.carsey@intel.com; zhichao.gao@intel.com; ray.ni@intel.com
Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

Jim --

Well, speaking of shooting-oneself-in-the-foot, it turns out that our non-EDK2 implementation followed the recommendation in the patch. 

I agree that the spec is ambiguous and, it turns out that our largest use case already uses the recommended behavior.

Sorry to one and all.

Tim

-----Original Message-----
From: Jim.Dailey@dell.com <Jim.Dailey@dell.com> 
Sent: Tuesday, May 7, 2019 1:30 PM
To: devel@edk2.groups.io; tim.lewis@insyde.com
Cc: dandan.bi@intel.com; jwatt@jwatt.org; jaben.carsey@intel.com; zhichao.gao@intel.com; ray.ni@intel.com
Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

Tim,

Out of curiosity, what does the specification you refer to that was used to write the non-EDK2 implementations say about the -opt switch?

Regards,
Jim

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Tim Lewis
Sent: Tuesday, May 7, 2019 3:04 PM
To: 'Jonathan Watt'; 'Carsey, Jaben'; devel@edk2.groups.io; 'Gao, Zhichao'; 'Ni, Ray'
Cc: 'Bi, Dandan'
Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option

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

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

[Jonathan Watt]

Ah, hopefully that simplifies the decision. At any rate, thank you for checking
and clarifying.

For those working on the spec, in addition to clarifying -opt to avoid this
confusion, one further thing to do would be to clarify the format of the "in
hexadecimal" option. It would be good if it could state which of leading "0x",
just leading zeros, and no prefix at all, are valid. The edk2 implementation
only appeared to support leading "0x" and no prefix at all based on my previous
testing, but I saw (what appeared to nowadays be incorrect) comments in the code
stating that leading zeros were also valid.

Jonathan

On 07/05/2019 21:48, Tim Lewis wrote:
> Jim --
> 
> Well, speaking of shooting-oneself-in-the-foot, it turns out that our non-EDK2 implementation followed the recommendation in the patch. 
> 
> I agree that the spec is ambiguous and, it turns out that our largest use case already uses the recommended behavior.
> 
> Sorry to one and all.
> 
> Tim
> 
> -----Original Message-----
> From: Jim.Dailey@dell.com <Jim.Dailey@dell.com> 
> Sent: Tuesday, May 7, 2019 1:30 PM
> To: devel@edk2.groups.io; tim.lewis@insyde.com
> Cc: dandan.bi@intel.com; jwatt@jwatt.org; jaben.carsey@intel.com; zhichao.gao@intel.com; ray.ni@intel.com
> Subject: RE: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option
> 
> Tim,
> 
> Out of curiosity, what does the specification you refer to that was used to write the non-EDK2 implementations say about the -opt switch?
> 
> Regards,
> Jim
> 
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Tim Lewis
> Sent: Tuesday, May 7, 2019 3:04 PM
> To: 'Jonathan Watt'; 'Carsey, Jaben'; devel@edk2.groups.io; 'Gao, Zhichao'; 'Ni, Ray'
> Cc: 'Bi, Dandan'
> Subject: Re: [edk2-devel] [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' option
> 
> 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
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
> 
> 
> 
> 
> 
> 
> 

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

[Jonathan Watt]

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

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

[Tim Lewis]

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

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

[Jonathan Watt]

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

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

[Carsey, Jaben]

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

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

[Tim Lewis]

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

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

[Jonathan Watt]

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

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

[Jonathan Watt]

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

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

[Carsey, Jaben]

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

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

[Gao, Zhichao]

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

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

[Jonathan Watt]

I see this landed last week. Many thanks to everyone for their input and for
getting this landed.

Jonathan

On 05/08/2019 01:51, Gao, Zhichao wrote:
> 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
>>