public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Carsey, Jaben" <jaben.carsey@intel.com>
To: Tim Lewis <tim.lewis@insyde.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Carsey, Jaben" <jaben.carsey@intel.com>
Subject: Re: [shell] AliasLower never used in InternalSetAlias
Date: Fri, 28 Oct 2016 16:58:45 +0000	[thread overview]
Message-ID: <CB6E33457884FA40993F35157061515C54AB3B6D@FMSMSX103.amr.corp.intel.com> (raw)
In-Reply-To: <7236196A5DF6C040855A6D96F556A53F3FD93F@msmail.insydesw.com.tw>

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


  reply	other threads:[~2016-10-28 16:58 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-10-27 21:11 [shell] AliasLower never used in InternalSetAlias Tim Lewis
2016-10-27 21:29 ` Tim Lewis
2016-10-28 15:20   ` Carsey, Jaben
2016-10-28 16:48     ` Tim Lewis
2016-10-28 16:58       ` Carsey, Jaben [this message]
2016-10-28 17:03         ` Tim Lewis
2016-10-28 17:04           ` Carsey, Jaben

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CB6E33457884FA40993F35157061515C54AB3B6D@FMSMSX103.amr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox