Hi Laszlo,
This series is looking great so far, I have yet to test it as I need
to set my platform up to build Shell before I can do that, however,
I've had a review of the patches and the only thing I've noticed so
far is....
On 21 January 2016 at 01:11, Laszlo Ersek <ler...@redhat.com> wrote:
> Don't exit the command immediately when a variable access fails; continue
> processing after printing the error message. Let the final return status
> reflect any encountered errors.
>
> This patch is intended as a functional improvement.
>
> Cc: Jaben Carsey <jaben.car...@intel.com>
> Cc: Ryan Harkin <ryan.har...@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 9
> +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git
> a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> index f5ae7bc..36d04d4 100644
> --- a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> +++ b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
> @@ -1051,12 +1051,15 @@ BcfgDisplayDump(
> UINTN LoopVar2;
> CHAR16 *DevPathString;
> VOID *DevPath;
> + UINTN Errors;
>
> if (OrderCount == 0) {
> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN(STR_BCFG_NONE),
> gShellBcfgHiiHandle, L"bcfg");
> return (SHELL_SUCCESS);
> }
>
> + Errors = 0;
> +
> for (LoopVar = 0 ; LoopVar < OrderCount ; LoopVar++) {
> Buffer = NULL;
> BufferSize = 0;
> @@ -1083,7 +1086,8 @@ BcfgDisplayDump(
>
> if (EFI_ERROR(Status) || Buffer == NULL) {
> ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_BCFG_READ_FAIL),
> gShellBcfgHiiHandle, L"bcfg", VariableName);
> - return (SHELL_INVALID_PARAMETER);
> + ++Errors;
> + goto Cleanup;
Does this goto mean that we exit immediately and don't continue
accumulating errors?
A subsequent patch, "ShellPkg: UefiShellBcfgCommandLib: enforce
minimum size for Boot#### and co.", also add a "goto Cleanup;".
Should this be a "continue;"?
> }
>
> if ((*(UINT16*)(Buffer+4)) != 0) {
> @@ -1120,6 +1124,7 @@ BcfgDisplayDump(
> L"\r\n");
> }
>
> +Cleanup:
> if (Buffer != NULL) {
> FreePool(Buffer);
> }
> @@ -1130,7 +1135,7 @@ BcfgDisplayDump(
> FreePool(DevPathString);
> }
> }
> - return (SHELL_SUCCESS);
> + return (Errors > 0) ? SHELL_INVALID_PARAMETER : SHELL_SUCCESS;
> }
>
> /**
> --
> 1.8.3.1
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
