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

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

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