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

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

[Ruiyu Ni]

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_VOLATILE), 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

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

[Ni, Ruiyu]

Including Tapan for review.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ruiyu Ni
> Sent: Thursday, June 1, 2017 10:12 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [edk2] [PATCH] ShellPkg/alias: Fix bug to support upper-case alias
> 
> 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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Carsey, Jaben]

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

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

[Ni, Ruiyu]

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

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

[Carsey, Jaben]

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

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

[Carsey, Jaben]

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

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

[Ni, Ruiyu]

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_VOLATILE), 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

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

[Carsey, Jaben]

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

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

[Shah, Tapan]

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

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

[Ni, Ruiyu]

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
>

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

[Shah, Tapan]

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
>