[shell] AliasLower never used in InternalSetAlias

[shell] AliasLower never used in InternalSetAlias

[Tim Lewis]

In the function InternalSetAlias, it appears that AliasLower is duplicated (fromAlias), converted to lower case and freed ,but never actually used. Am I missing something?

  // 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
  //
  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));
  }

  if (Alias != NULL) {
    FreePool (AliasLower);
  }

Re: [shell] AliasLower never used in InternalSetAlias

[Tim Lewis]

I would also note that GetAlias() has similar logic, but does, in fact use the AliasLower. As far as I can tell, the specification does not say anything about case-insensitive, so I believe this to be in error.

Tim

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Tim Lewis
Sent: Thursday, October 27, 2016 2:11 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [shell] AliasLower never used in InternalSetAlias

In the function InternalSetAlias, it appears that AliasLower is duplicated (fromAlias), converted to lower case and freed ,but never actually used. Am I missing something?

  // 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
  //
  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));
  }

  if (Alias != NULL) {
    FreePool (AliasLower);
  }
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [shell] AliasLower never used in InternalSetAlias

[Carsey, Jaben]

Tim,

Given that all commands are case insensitive, I couldn't imagine why we would want case-sensitive alias.

Do we really want "Dir" to fail, while "dir" works fine?  Remember that the real command is case insensitive "ls" in either case.

-Jaben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Tim Lewis
> Sent: Thursday, October 27, 2016 2:29 PM
> To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [shell] AliasLower never used in InternalSetAlias
> Importance: High
> 
> I would also note that GetAlias() has similar logic, but does, in fact use the
> AliasLower. As far as I can tell, the specification does not say anything about
> case-insensitive, so I believe this to be in error.
> 
> Tim
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Tim Lewis
> Sent: Thursday, October 27, 2016 2:11 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [shell] AliasLower never used in InternalSetAlias
> 
> In the function InternalSetAlias, it appears that AliasLower is duplicated
> (fromAlias), converted to lower case and freed ,but never actually used. Am I
> missing something?
> 
>   // 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
>   //
>   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));
>   }
> 
>   if (Alias != NULL) {
>     FreePool (AliasLower);
>   }
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [shell] AliasLower never used in InternalSetAlias

[Tim Lewis]

Jaben --

1) I'm not talking about desired behavior, I'm talking about spec behavior. As an external user of GetAlias() and SetAlias() (and a non-EDK2 shell implementer), I have become aware of a number of places where the EDK2 shell makes assumptions.  The spec requirement is that commands are case-insensitive, but does not place this requirement on Alias. The way in which the command-line uses aliases is not relevant to how the API responds. The fact is: there might be entries for both Dir and dir, and the command-line usage in this case is non-deterministic as the specification sits now. Obviously the spec needs an update to deal with what is currently a behavior which can lead to unexpected API failures.
2) As I pointed out separately, the current code doesn't work as you (or I) describe. It does not use AliasLower (the lower case version of the input string) for the variable name in SetAlias, but does use AliasLower in GetAlias. 
3) The specification also states that aliases are identifiers, but the internal usage for "cd.." and "cd\" do not follow this rule. Neither the command or the API enforce this rule.

BTW, another place I have found is in the mapping API, where the "cd" command currently relies on the fact that the 2nd parameter can have a mapping name in it (while leaving the first parameter as a NULL). This is unexpected behavior on the API part.

Tim
-----Original Message-----
From: Carsey, Jaben [mailto:jaben.carsey@intel.com] 
Sent: Friday, October 28, 2016 8:20 AM
To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
Cc: Carsey, Jaben <jaben.carsey@intel.com>
Subject: RE: [shell] AliasLower never used in InternalSetAlias

Tim,

Given that all commands are case insensitive, I couldn't imagine why we would want case-sensitive alias.

Do we really want "Dir" to fail, while "dir" works fine?  Remember that the real command is case insensitive "ls" in either case.

-Jaben

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Tim Lewis
> Sent: Thursday, October 27, 2016 2:29 PM
> To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> Subject: Re: [edk2] [shell] AliasLower never used in InternalSetAlias
> Importance: High
> 
> I would also note that GetAlias() has similar logic, but does, in fact 
> use the AliasLower. As far as I can tell, the specification does not 
> say anything about case-insensitive, so I believe this to be in error.
> 
> Tim
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Tim Lewis
> Sent: Thursday, October 27, 2016 2:11 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [shell] AliasLower never used in InternalSetAlias
> 
> In the function InternalSetAlias, it appears that AliasLower is 
> duplicated (fromAlias), converted to lower case and freed ,but never 
> actually used. Am I missing something?
> 
>   // 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
>   //
>   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));
>   }
> 
>   if (Alias != NULL) {
>     FreePool (AliasLower);
>   }
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [shell] AliasLower never used in InternalSetAlias

[Carsey, Jaben]

I agree with the goal of being more closely matched to the spec.

I think that some things we need to adjust the spec, others we adjust the code, and others may go beyond the spec.  We have no prohibition that an implementation cannot have additional features as long as they do not violate the requirements of the spec.

The unused code for #2 is a simple bug I think (as is any unused code I would argue).

-Jaben

> -----Original Message-----
> From: Tim Lewis [mailto:tim.lewis@insyde.com]
> Sent: Friday, October 28, 2016 9:49 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [shell] AliasLower never used in InternalSetAlias
> Importance: High
> 
> Jaben --
> 
> 1) I'm not talking about desired behavior, I'm talking about spec behavior. As
> an external user of GetAlias() and SetAlias() (and a non-EDK2 shell
> implementer), I have become aware of a number of places where the EDK2
> shell makes assumptions.  The spec requirement is that commands are case-
> insensitive, but does not place this requirement on Alias. The way in which
> the command-line uses aliases is not relevant to how the API responds. The
> fact is: there might be entries for both Dir and dir, and the command-line
> usage in this case is non-deterministic as the specification sits now. Obviously
> the spec needs an update to deal with what is currently a behavior which can
> lead to unexpected API failures.
> 2) As I pointed out separately, the current code doesn't work as you (or I)
> describe. It does not use AliasLower (the lower case version of the input
> string) for the variable name in SetAlias, but does use AliasLower in GetAlias.
> 3) The specification also states that aliases are identifiers, but the internal
> usage for "cd.." and "cd\" do not follow this rule. Neither the command or
> the API enforce this rule.
> 
> BTW, another place I have found is in the mapping API, where the "cd"
> command currently relies on the fact that the 2nd parameter can have a
> mapping name in it (while leaving the first parameter as a NULL). This is
> unexpected behavior on the API part.
> 
> Tim
> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Friday, October 28, 2016 8:20 AM
> To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: [shell] AliasLower never used in InternalSetAlias
> 
> Tim,
> 
> Given that all commands are case insensitive, I couldn't imagine why we
> would want case-sensitive alias.
> 
> Do we really want "Dir" to fail, while "dir" works fine?  Remember that the
> real command is case insensitive "ls" in either case.
> 
> -Jaben
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Tim Lewis
> > Sent: Thursday, October 27, 2016 2:29 PM
> > To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> > Subject: Re: [edk2] [shell] AliasLower never used in InternalSetAlias
> > Importance: High
> >
> > I would also note that GetAlias() has similar logic, but does, in fact
> > use the AliasLower. As far as I can tell, the specification does not
> > say anything about case-insensitive, so I believe this to be in error.
> >
> > Tim
> >
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Tim Lewis
> > Sent: Thursday, October 27, 2016 2:11 PM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [shell] AliasLower never used in InternalSetAlias
> >
> > In the function InternalSetAlias, it appears that AliasLower is
> > duplicated (fromAlias), converted to lower case and freed ,but never
> > actually used. Am I missing something?
> >
> >   // 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
> >   //
> >   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));
> >   }
> >
> >   if (Alias != NULL) {
> >     FreePool (AliasLower);
> >   }
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [shell] AliasLower never used in InternalSetAlias

[Tim Lewis]

Sure. I think we should adjust the specification in this case. My problem has been when someone says "but the EDK2 version does X"

Tim

-----Original Message-----
From: Carsey, Jaben [mailto:jaben.carsey@intel.com] 
Sent: Friday, October 28, 2016 9:59 AM
To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
Cc: Carsey, Jaben <jaben.carsey@intel.com>
Subject: RE: [shell] AliasLower never used in InternalSetAlias

I agree with the goal of being more closely matched to the spec.

I think that some things we need to adjust the spec, others we adjust the code, and others may go beyond the spec.  We have no prohibition that an implementation cannot have additional features as long as they do not violate the requirements of the spec.

The unused code for #2 is a simple bug I think (as is any unused code I would argue).

-Jaben

> -----Original Message-----
> From: Tim Lewis [mailto:tim.lewis@insyde.com]
> Sent: Friday, October 28, 2016 9:49 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [shell] AliasLower never used in InternalSetAlias
> Importance: High
> 
> Jaben --
> 
> 1) I'm not talking about desired behavior, I'm talking about spec 
> behavior. As an external user of GetAlias() and SetAlias() (and a 
> non-EDK2 shell implementer), I have become aware of a number of places 
> where the EDK2 shell makes assumptions.  The spec requirement is that 
> commands are case- insensitive, but does not place this requirement on 
> Alias. The way in which the command-line uses aliases is not relevant 
> to how the API responds. The fact is: there might be entries for both 
> Dir and dir, and the command-line usage in this case is 
> non-deterministic as the specification sits now. Obviously the spec 
> needs an update to deal with what is currently a behavior which can lead to unexpected API failures.
> 2) As I pointed out separately, the current code doesn't work as you 
> (or I) describe. It does not use AliasLower (the lower case version of 
> the input
> string) for the variable name in SetAlias, but does use AliasLower in GetAlias.
> 3) The specification also states that aliases are identifiers, but the 
> internal usage for "cd.." and "cd\" do not follow this rule. Neither 
> the command or the API enforce this rule.
> 
> BTW, another place I have found is in the mapping API, where the "cd"
> command currently relies on the fact that the 2nd parameter can have a 
> mapping name in it (while leaving the first parameter as a NULL). This 
> is unexpected behavior on the API part.
> 
> Tim
> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Friday, October 28, 2016 8:20 AM
> To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: [shell] AliasLower never used in InternalSetAlias
> 
> Tim,
> 
> Given that all commands are case insensitive, I couldn't imagine why 
> we would want case-sensitive alias.
> 
> Do we really want "Dir" to fail, while "dir" works fine?  Remember 
> that the real command is case insensitive "ls" in either case.
> 
> -Jaben
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
> > Of Tim Lewis
> > Sent: Thursday, October 27, 2016 2:29 PM
> > To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> > Subject: Re: [edk2] [shell] AliasLower never used in 
> > InternalSetAlias
> > Importance: High
> >
> > I would also note that GetAlias() has similar logic, but does, in 
> > fact use the AliasLower. As far as I can tell, the specification 
> > does not say anything about case-insensitive, so I believe this to be in error.
> >
> > Tim
> >
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 
> > Of Tim Lewis
> > Sent: Thursday, October 27, 2016 2:11 PM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [shell] AliasLower never used in InternalSetAlias
> >
> > In the function InternalSetAlias, it appears that AliasLower is 
> > duplicated (fromAlias), converted to lower case and freed ,but never 
> > actually used. Am I missing something?
> >
> >   // 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
> >   //
> >   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));
> >   }
> >
> >   if (Alias != NULL) {
> >     FreePool (AliasLower);
> >   }
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [shell] AliasLower never used in InternalSetAlias

[Carsey, Jaben]

I am in complete agreement.  The implementation must meet the spec constraints.

> -----Original Message-----
> From: Tim Lewis [mailto:tim.lewis@insyde.com]
> Sent: Friday, October 28, 2016 10:04 AM
> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> Subject: RE: [shell] AliasLower never used in InternalSetAlias
> Importance: High
> 
> Sure. I think we should adjust the specification in this case. My problem has
> been when someone says "but the EDK2 version does X"
> 
> Tim
> 
> -----Original Message-----
> From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> Sent: Friday, October 28, 2016 9:59 AM
> To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>
> Subject: RE: [shell] AliasLower never used in InternalSetAlias
> 
> I agree with the goal of being more closely matched to the spec.
> 
> I think that some things we need to adjust the spec, others we adjust the
> code, and others may go beyond the spec.  We have no prohibition that an
> implementation cannot have additional features as long as they do not
> violate the requirements of the spec.
> 
> The unused code for #2 is a simple bug I think (as is any unused code I would
> argue).
> 
> -Jaben
> 
> > -----Original Message-----
> > From: Tim Lewis [mailto:tim.lewis@insyde.com]
> > Sent: Friday, October 28, 2016 9:49 AM
> > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
> > Subject: RE: [shell] AliasLower never used in InternalSetAlias
> > Importance: High
> >
> > Jaben --
> >
> > 1) I'm not talking about desired behavior, I'm talking about spec
> > behavior. As an external user of GetAlias() and SetAlias() (and a
> > non-EDK2 shell implementer), I have become aware of a number of places
> > where the EDK2 shell makes assumptions.  The spec requirement is that
> > commands are case- insensitive, but does not place this requirement on
> > Alias. The way in which the command-line uses aliases is not relevant
> > to how the API responds. The fact is: there might be entries for both
> > Dir and dir, and the command-line usage in this case is
> > non-deterministic as the specification sits now. Obviously the spec
> > needs an update to deal with what is currently a behavior which can lead to
> unexpected API failures.
> > 2) As I pointed out separately, the current code doesn't work as you
> > (or I) describe. It does not use AliasLower (the lower case version of
> > the input
> > string) for the variable name in SetAlias, but does use AliasLower in
> GetAlias.
> > 3) The specification also states that aliases are identifiers, but the
> > internal usage for "cd.." and "cd\" do not follow this rule. Neither
> > the command or the API enforce this rule.
> >
> > BTW, another place I have found is in the mapping API, where the "cd"
> > command currently relies on the fact that the 2nd parameter can have a
> > mapping name in it (while leaving the first parameter as a NULL). This
> > is unexpected behavior on the API part.
> >
> > Tim
> > -----Original Message-----
> > From: Carsey, Jaben [mailto:jaben.carsey@intel.com]
> > Sent: Friday, October 28, 2016 8:20 AM
> > To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> > Cc: Carsey, Jaben <jaben.carsey@intel.com>
> > Subject: RE: [shell] AliasLower never used in InternalSetAlias
> >
> > Tim,
> >
> > Given that all commands are case insensitive, I couldn't imagine why
> > we would want case-sensitive alias.
> >
> > Do we really want "Dir" to fail, while "dir" works fine?  Remember
> > that the real command is case insensitive "ls" in either case.
> >
> > -Jaben
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Tim Lewis
> > > Sent: Thursday, October 27, 2016 2:29 PM
> > > To: Tim Lewis <tim.lewis@insyde.com>; edk2-devel@lists.01.org
> > > Subject: Re: [edk2] [shell] AliasLower never used in
> > > InternalSetAlias
> > > Importance: High
> > >
> > > I would also note that GetAlias() has similar logic, but does, in
> > > fact use the AliasLower. As far as I can tell, the specification
> > > does not say anything about case-insensitive, so I believe this to be in
> error.
> > >
> > > Tim
> > >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> > > Of Tim Lewis
> > > Sent: Thursday, October 27, 2016 2:11 PM
> > > To: edk2-devel@lists.01.org
> > > Subject: [edk2] [shell] AliasLower never used in InternalSetAlias
> > >
> > > In the function InternalSetAlias, it appears that AliasLower is
> > > duplicated (fromAlias), converted to lower case and freed ,but never
> > > actually used. Am I missing something?
> > >
> > >   // 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
> > >   //
> > >   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));
> > >   }
> > >
> > >   if (Alias != NULL) {
> > >     FreePool (AliasLower);
> > >   }
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel