It's a bug and should be fixed. Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=589
Thanks, Tapan -----Original Message----- From: Ni, Ruiyu [mailto:[email protected]] Sent: Saturday, June 03, 2017 7:48 PM To: Shah, Tapan <[email protected]>; Carsey, Jaben <[email protected]>; [email protected] Cc: Kinney, Michael D <[email protected]> Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias It's the original behavior of alias, not changed by this fix. If you think it's a bug, could you please submit a Bugzilla to record it? Regards, Ray >-----Original Message----- >From: Shah, Tapan [mailto:[email protected]] >Sent: Saturday, June 3, 2017 12:13 AM >To: Carsey, Jaben <[email protected]>; Ni, Ruiyu ><[email protected]>; [email protected] >Cc: Kinney, Michael D <[email protected]> >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case >alias > >When attempting to delete an alias that does not exist, it does not >return an error in lasterror status indiciating alias was not found. > >-----Original Message----- >From: Carsey, Jaben [mailto:[email protected]] >Sent: Friday, June 02, 2017 10:04 AM >To: Ni, Ruiyu <[email protected]>; [email protected] >Cc: Kinney, Michael D <[email protected]>; Shah, Tapan ><[email protected]> >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case >alias > >Ok. I see now. Thanks! > >Reviewed-by: Jaben Carsey <[email protected]> > >> -----Original Message----- >> From: Ni, Ruiyu >> Sent: Thursday, June 01, 2017 7:32 PM >> To: Carsey, Jaben <[email protected]>; [email protected] >> Cc: Kinney, Michael D <[email protected]>; Shah, Tapan >> ([email protected]) <[email protected]> >> Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case >> alias >> Importance: High >> >> Jaben, >> Old code also honors the returning status when delete an alias. >> Please check the line I marked as "<----------" in below. >> >> // >> // 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)); >> } >> >> Regards, >> Ray >> >> >-----Original Message----- >> >From: Carsey, Jaben >> >Sent: Thursday, June 1, 2017 11:42 PM >> >To: Ni, Ruiyu <[email protected]>; [email protected] >> >Cc: Kinney, Michael D <[email protected]>; Shah, Tapan >> ([email protected]) <[email protected]> >> >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case >> >alias >> > >> >I think we have to leave the behavior the same. The spec says this: >> >" If the >> environment variable does not exist and the >> >Value is an empty string, there is no action." >> > >> >I do not think we can change that to an error return without a spec change. >> > >> >-Jaben >> > >> >> -----Original Message----- >> >> From: Carsey, Jaben >> >> Sent: Thursday, June 01, 2017 8:40 AM >> >> To: Ni, Ruiyu <[email protected]>; [email protected] >> >> Cc: Kinney, Michael D <[email protected]>; Shah, Tapan >> >> ([email protected]) <[email protected]> >> >> Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case >> >> alias >> >> >> >> I just think we may want to have the behavior act the same as it >> >> does >> today >> >> for delete. >> >> >> >> > -----Original Message----- >> >> > From: Ni, Ruiyu >> >> > Sent: Thursday, June 01, 2017 8:19 AM >> >> > To: Carsey, Jaben <[email protected]>; >> >> > [email protected] >> >> > Cc: Kinney, Michael D <[email protected]>; Shah, Tapan >> >> > ([email protected]) <[email protected]> >> >> > Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support >> >> > upper-case alias >> >> > Importance: High >> >> > >> >> > I was using AliasLower. >> >> > I am not sure whether the change is smallest. But I tried best >> >> > to make >> the >> >> > new implementation cleaner. I think that's what we really need. >> >> > >> >> > Did you see any issue if we return EFI_NOT_FOUND (when variable >> doesn't >> >> > exist)? >> >> > >> >> > Regards, >> >> > Ray >> >> > >> >> > >-----Original Message----- >> >> > >From: Carsey, Jaben >> >> > >Sent: Thursday, June 1, 2017 11:02 PM >> >> > >To: Ni, Ruiyu <[email protected]>; [email protected] >> >> > >Cc: Kinney, Michael D <[email protected]>; Shah, Tapan >> >> > ([email protected]) <[email protected]> >> >> > >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support >> >> > >upper-case alias >> >> > > >> >> > >Why not just use the AliasLower and make the overall change >> >> > >much >> >> > smaller? Looks like the old version did the conversion, >> >> > >but didn't use the result. >> >> > > >> >> > >Also, I notice that we are now checking the return value upon >> >> > >delete, >> >> which >> >> > was explicitly not done in the old version. >> >> > >There was this comment before: "We dont check the error return >> >> > >on >> >> > purpose since the variable may not exist." >> >> > > >> >> > >-Jaben >> >> > > >> >> > > >> >> > >> -----Original Message----- >> >> > >> From: Ni, Ruiyu >> >> > >> Sent: Thursday, June 01, 2017 7:12 AM >> >> > >> To: [email protected] >> >> > >> Cc: Carsey, Jaben <[email protected]>; Kinney, Michael D >> >> > >> <[email protected]> >> >> > >> Subject: [PATCH] ShellPkg/alias: Fix bug to support >> >> > >> upper-case alias >> >> > >> Importance: High >> >> > >> >> >> > >> alias in UEFI Shell is case insensitive. >> >> > >> Old code saves the alias to variable storage without >> >> > >> converting the alias to lower-case, which results upper case >> >> > >> alias setting doesn't work. >> >> > >> The patch converts the alias to lower case before saving to >> >> > >> variable storage. >> >> > >> >> >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 >> >> > >> Signed-off-by: Ruiyu Ni <[email protected]> >> >> > >> Cc: Jaben Carsey <[email protected]> >> >> > >> Cc: Michael D Kinney <[email protected]> >> >> > >> --- >> >> > >> ShellPkg/Application/Shell/ShellProtocol.c | 50 >> >> > >> +++++++++++++++-- >> ---- >> >> -- >> >> > ---- >> >> > >> --- >> >> > >> 1 file changed, 25 insertions(+), 25 deletions(-) >> >> > >> >> >> > >> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c >> >> > >> b/ShellPkg/Application/Shell/ShellProtocol.c >> >> > >> index 347e162e62..b3b8acc0d0 100644 >> >> > >> --- a/ShellPkg/Application/Shell/ShellProtocol.c >> >> > >> +++ b/ShellPkg/Application/Shell/ShellProtocol.c >> >> > >> @@ -3463,40 +3463,40 @@ InternalSetAlias( { >> >> > >> EFI_STATUS Status; >> >> > >> CHAR16 *AliasLower; >> >> > >> + BOOLEAN DeleteAlias; >> >> > >> >> >> > >> - // 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 >> >> > >> - // >> >> > >> + DeleteAlias = FALSE; >> >> > >> if (Alias == NULL) { >> >> > >> // >> >> > >> + // We must be trying to remove one if Alias is 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); >> >> > >> + Alias = Command; >> >> > >> + DeleteAlias = TRUE; >> >> > >> + } >> >> > >> + ASSERT (Alias != NULL); >> >> > >> >> >> > >> - Status = (gRT->SetVariable((CHAR16*)Alias, &gShellAliasGuid, >> >> > >> >> >> > >> >> >> EFI_VARIABLE_BOOTSERVICE_ACCESS|(Volatile?0:EFI_VARIABLE_NON_VOL >> >> > >> ATILE), StrSize(Command), (VOID*)Command)); >> >> > >> + // >> >> > >> + // Convert to lowercase to make aliases case-insensitive >> >> > >> + // AliasLower = AllocateCopyPool (StrSize (Alias), Alias); >> >> > >> + if (AliasLower == NULL) { >> >> > >> + return EFI_OUT_OF_RESOURCES; >> >> > >> } >> >> > >> + ToLower (AliasLower); >> >> > >> >> >> > >> - if (Alias != NULL) { >> >> > >> - FreePool (AliasLower); >> >> > >> + if (DeleteAlias) { >> >> > >> + Status = gRT->SetVariable (AliasLower, &gShellAliasGuid, >> >> > >> + 0, 0, >> NULL); >> >> > >> + } else { >> >> > >> + Status = gRT->SetVariable ( >> >> > >> + AliasLower, &gShellAliasGuid, >> >> > >> + EFI_VARIABLE_BOOTSERVICE_ACCESS | (Volatile ? 0 >> >> > >> : >> >> > >> EFI_VARIABLE_NON_VOLATILE), >> >> > >> + StrSize (Command), (VOID *) Command >> >> > >> + ); >> >> > >> } >> >> > >> + >> >> > >> + FreePool (AliasLower); >> >> > >> + >> >> > >> return Status; >> >> > >> } >> >> > >> >> >> > >> -- >> >> > >> 2.12.2.windows.2 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
