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...@intel.com> Thanks, Dandan > -----Original Message----- > From: Carsey, Jaben > Sent: Tuesday, May 07, 2019 10:36 PM > To: Gao, Zhichao <zhichao....@intel.com>; devel@edk2.groups.io; Ni, Ray > <ray...@intel.com>; jw...@jwatt.org > Cc: Bi, Dandan <dandan...@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.car...@intel.com> > > > > -----Original Message----- > > From: Gao, Zhichao > > Sent: Tuesday, May 07, 2019 2:52 AM > > To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; jw...@jwatt.org > > Cc: Carsey, Jaben <jaben.car...@intel.com>; Bi, Dandan > > <dandan...@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....@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: jw...@jwatt.org; devel@edk2.groups.io > > > Cc: Carsey, Jaben <jaben.car...@intel.com>; Bi, Dandan > > > <dandan...@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: jw...@jwatt.org [mailto:jw...@jwatt.org] > > > > Sent: Monday, May 6, 2019 9:03 PM > > > > To: devel@edk2.groups.io > > > > Cc: Carsey, Jaben <jaben.car...@intel.com>; Ni, Ray > > > > <ray...@intel.com> > > > > Subject: [PATCH v1 1/1] ShellPkg/UefiShellBcfgCommandLib: Fix '-opt' > > > > option > > > > > > > > From: Jonathan Watt <jw...@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.car...@intel.com> > > > > CC: Ray Ni <ray...@intel.com> > > > > Signed-off-by: Jonathan Watt <jw...@jwatt.org> > > > > --- > > > > > > > > > ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > > | > > > > 2 > > > > +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git > > > > > > > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > > > > > > > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > > > > index d033c7c1dc59..e8b48b4990dd 100644 > > > > --- > > > > > > > a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > > > > +++ > > > > > > > b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c > > > > @@ -1019,7 +1019,7 @@ BcfgAddOpt( > > > > // > > > > // Get the index of the variable we are changing. > > > > // > > > > - Status = ShellConvertStringToUint64(Walker, &Intermediate, > > > > FALSE, TRUE); > > > > + Status = ShellConvertStringToUint64(Walker, &Intermediate, > > > > + TRUE, TRUE); > > > > if (EFI_ERROR(Status) || (((UINT16)Intermediate) != > > > > Intermediate) > > > > || StrStr(Walker, L" ") == NULL || ((UINT16)Intermediate) > > > > > ((UINT16)OrderCount)) { > > > > ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN > > > > (STR_GEN_PARAM_INV), gShellBcfgHiiHandle, L"bcfg", L"Option > Index"); > > > > ShellStatus = SHELL_INVALID_PARAMETER; > > > > -- > > > > 2.21.0 > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40120): https://edk2.groups.io/g/devel/message/40120 Mute This Topic: https://groups.io/mt/31520134/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-