Re: [shell] AliasLower never used in InternalSetAlias

[Tim Lewis <tim.lewis@insyde.com>]

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