I am in complete agreement. The implementation must meet the spec constraints.
> -----Original Message----- > From: Tim Lewis [mailto:tim.le...@insyde.com] > Sent: Friday, October 28, 2016 10:04 AM > To: Carsey, Jaben <jaben.car...@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.car...@intel.com] > Sent: Friday, October 28, 2016 9:59 AM > To: Tim Lewis <tim.le...@insyde.com>; edk2-devel@lists.01.org > Cc: Carsey, Jaben <jaben.car...@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.le...@insyde.com] > > Sent: Friday, October 28, 2016 9:49 AM > > To: Carsey, Jaben <jaben.car...@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.car...@intel.com] > > Sent: Friday, October 28, 2016 8:20 AM > > To: Tim Lewis <tim.le...@insyde.com>; edk2-devel@lists.01.org > > Cc: Carsey, Jaben <jaben.car...@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-boun...@lists.01.org] On Behalf > > > Of Tim Lewis > > > Sent: Thursday, October 27, 2016 2:29 PM > > > To: Tim Lewis <tim.le...@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-boun...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
