Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias

["Carsey, Jaben" <jaben.carsey@intel.com>]

Why not just use the AliasLower and make the overall change much smaller?  Looks like the old version did the conversion, but didn't use the result.

Also, I notice that we are now checking the return value upon delete, which was explicitly not done in the old version.  There was this comment before: "We dont check the error return on purpose since the variable may not exist."

-Jaben


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, June 01, 2017 7:12 AM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias
> Importance: High
> 
> alias in UEFI Shell is case insensitive.
> Old code saves the alias to variable storage without
> converting the alias to lower-case, which results
> upper case alias setting doesn't work.
> The patch converts the alias to lower case before saving
> to variable storage.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> ---
>  ShellPkg/Application/Shell/ShellProtocol.c | 50 +++++++++++++++------------
> ---
>  1 file changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
> b/ShellPkg/Application/Shell/ShellProtocol.c
> index 347e162e62..b3b8acc0d0 100644
> --- a/ShellPkg/Application/Shell/ShellProtocol.c
> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> @@ -3463,40 +3463,40 @@ InternalSetAlias(
>  {
>    EFI_STATUS  Status;
>    CHAR16      *AliasLower;
> +  BOOLEAN     DeleteAlias;
> 
> -  // Convert to lowercase to make aliases case-insensitive
> -  if (Alias != NULL) {
> -    AliasLower = AllocateCopyPool (StrSize (Alias), Alias);
> -    if (AliasLower == NULL) {
> -      return EFI_OUT_OF_RESOURCES;
> -    }
> -    ToLower (AliasLower);
> -  } else {
> -    AliasLower = NULL;
> -  }
> -
> -  //
> -  // We must be trying to remove one if Alias is NULL
> -  //
> +  DeleteAlias = FALSE;
>    if (Alias == NULL) {
>      //
> +    // We must be trying to remove one if Alias is NULL
>      // remove an alias (but passed in COMMAND parameter)
>      //
> -    Status = (gRT->SetVariable((CHAR16*)Command, &gShellAliasGuid, 0, 0,
> NULL));
> -  } else {
> -    //
> -    // Add and replace are the same
> -    //
> -
> -    // We dont check the error return on purpose since the variable may not
> exist.
> -    gRT->SetVariable((CHAR16*)Command, &gShellAliasGuid, 0, 0, NULL);
> +    Alias       = Command;
> +    DeleteAlias = TRUE;
> +  }
> +  ASSERT (Alias != NULL);
> 
> -    Status = (gRT->SetVariable((CHAR16*)Alias, &gShellAliasGuid,
> EFI_VARIABLE_BOOTSERVICE_ACCESS|(Volatile?0:EFI_VARIABLE_NON_VOL
> ATILE), StrSize(Command), (VOID*)Command));
> +  //
> +  // Convert to lowercase to make aliases case-insensitive
> +  //
> +  AliasLower = AllocateCopyPool (StrSize (Alias), Alias);
> +  if (AliasLower == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
>    }
> +  ToLower (AliasLower);
> 
> -  if (Alias != NULL) {
> -    FreePool (AliasLower);
> +  if (DeleteAlias) {
> +    Status = gRT->SetVariable (AliasLower, &gShellAliasGuid, 0, 0, NULL);
> +  } else {
> +    Status = gRT->SetVariable (
> +                    AliasLower, &gShellAliasGuid,
> +                    EFI_VARIABLE_BOOTSERVICE_ACCESS | (Volatile ? 0 :
> EFI_VARIABLE_NON_VOLATILE),
> +                    StrSize (Command), (VOID *) Command
> +                    );
>    }
> +
> +  FreePool (AliasLower);
> +
>    return Status;
>  }
> 
> --
> 2.12.2.windows.2