Re: [shell] AliasLower never used in InternalSetAlias

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

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