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

["Shah, Tapan" <tapandshah@hpe.com>]

It's a bug and should be fixed.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=589

Thanks,
Tapan

-----Original Message-----
From: Ni, Ruiyu [mailto:ruiyu.ni@intel.com] 
Sent: Saturday, June 03, 2017 7:48 PM
To: Shah, Tapan <tapandshah@hpe.com>; Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias

It's the original behavior of alias, not changed by this fix.
If you think it's a bug, could you please submit a Bugzilla to record it?

Regards,
Ray

>-----Original Message-----
>From: Shah, Tapan [mailto:tapandshah@hpe.com]
>Sent: Saturday, June 3, 2017 12:13 AM
>To: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu 
><ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>
>Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case 
>alias
>
>When attempting to delete an alias that  does not exist, it does not 
>return an error in lasterror status indiciating alias was not found.
>
>-----Original Message-----
>From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
>Sent: Friday, June 02, 2017 10:04 AM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan 
><tapandshah@hpe.com>
>Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case 
>alias
>
>Ok.  I see now. Thanks!
>
>Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
>
>> -----Original Message-----
>> From: Ni, Ruiyu
>> Sent: Thursday, June 01, 2017 7:32 PM
>> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan
>> (tapandshah@hpe.com) <tapandshah@hpe.com>
>> Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case 
>> alias
>> Importance: High
>>
>> Jaben,
>> Old code also honors the returning status when delete an alias.
>> Please check the line I marked as "<----------" in below.
>>
>>   //
>>   // We must be trying to remove one if Alias is NULL
>>   //
>>   if (Alias == 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);
>>
>>     Status = (gRT->SetVariable((CHAR16*)Alias, &gShellAliasGuid, 
>> EFI_VARIABLE_BOOTSERVICE_ACCESS|(Volatile?0:EFI_VARIABLE_NON_VOL
>> ATILE), StrSize(Command), (VOID*)Command));
>>   }
>>
>> Regards,
>> Ray
>>
>> >-----Original Message-----
>> >From: Carsey, Jaben
>> >Sent: Thursday, June 1, 2017 11:42 PM
>> >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan
>> (tapandshah@hpe.com) <tapandshah@hpe.com>
>> >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case 
>> >alias
>> >
>> >I think we have to leave the behavior the same.  The spec says this:
>> >" If the
>> environment variable does not exist and the
>> >Value is an empty string, there is no action."
>> >
>> >I do not think we can change that to an error return without a spec change.
>> >
>> >-Jaben
>> >
>> >> -----Original Message-----
>> >> From: Carsey, Jaben
>> >> Sent: Thursday, June 01, 2017 8:40 AM
>> >> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan
>> >> (tapandshah@hpe.com) <tapandshah@hpe.com>
>> >> Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case 
>> >> alias
>> >>
>> >> I just think we may want to have the behavior act the same as it 
>> >> does
>> today
>> >> for delete.
>> >>
>> >> > -----Original Message-----
>> >> > From: Ni, Ruiyu
>> >> > Sent: Thursday, June 01, 2017 8:19 AM
>> >> > To: Carsey, Jaben <jaben.carsey@intel.com>; 
>> >> > edk2-devel@lists.01.org
>> >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan
>> >> > (tapandshah@hpe.com) <tapandshah@hpe.com>
>> >> > Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support 
>> >> > upper-case alias
>> >> > Importance: High
>> >> >
>> >> > I was using AliasLower.
>> >> > I am not sure whether the change is smallest. But I tried best 
>> >> > to make
>> the
>> >> > new implementation cleaner. I think that's what we really need.
>> >> >
>> >> > Did you see any issue if we return EFI_NOT_FOUND (when variable
>> doesn't
>> >> > exist)?
>> >> >
>> >> > Regards,
>> >> > Ray
>> >> >
>> >> > >-----Original Message-----
>> >> > >From: Carsey, Jaben
>> >> > >Sent: Thursday, June 1, 2017 11:02 PM
>> >> > >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>> >> > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan
>> >> > (tapandshah@hpe.com) <tapandshah@hpe.com>
>> >> > >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support 
>> >> > >upper-case alias
>> >> > >
>> >> > >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
>