Re: [shell] AliasLower never used in InternalSetAlias

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

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