* [PATCH] ShellPkg/alias: Fix bug to support upper-case alias @ 2017-06-01 14:11 Ruiyu Ni 2017-06-01 14:14 ` Ni, Ruiyu 2017-06-01 15:01 ` Carsey, Jaben 0 siblings, 2 replies; 11+ messages in thread From: Ruiyu Ni @ 2017-06-01 14:11 UTC (permalink / raw) To: edk2-devel; +Cc: Jaben Carsey, Michael D Kinney 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 <ruiyu.ni@intel.com> Cc: Jaben Carsey <jaben.carsey@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> --- 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_VOLATILE), 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-01 14:11 [PATCH] ShellPkg/alias: Fix bug to support upper-case alias Ruiyu Ni @ 2017-06-01 14:14 ` Ni, Ruiyu 2017-06-01 15:01 ` Carsey, Jaben 1 sibling, 0 replies; 11+ messages in thread From: Ni, Ruiyu @ 2017-06-01 14:14 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org Cc: Carsey, Jaben, Kinney, Michael D, Shah, Tapan Including Tapan for review. Thanks/Ray > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Ruiyu Ni > Sent: Thursday, June 1, 2017 10:12 PM > To: edk2-devel@lists.01.org > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com> > Subject: [edk2] [PATCH] ShellPkg/alias: Fix bug to support upper-case alias > > 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 <ruiyu.ni@intel.com> > Cc: Jaben Carsey <jaben.carsey@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > --- > 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 > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-01 14:11 [PATCH] ShellPkg/alias: Fix bug to support upper-case alias Ruiyu Ni 2017-06-01 14:14 ` Ni, Ruiyu @ 2017-06-01 15:01 ` Carsey, Jaben 2017-06-01 15:18 ` Ni, Ruiyu 1 sibling, 1 reply; 11+ messages in thread From: Carsey, Jaben @ 2017-06-01 15:01 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org Cc: Kinney, Michael D, Shah, Tapan (tapandshah@hpe.com) 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: edk2-devel@lists.01.org > Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D > <michael.d.kinney@intel.com> > 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 <ruiyu.ni@intel.com> > Cc: Jaben Carsey <jaben.carsey@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > --- > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-01 15:01 ` Carsey, Jaben @ 2017-06-01 15:18 ` Ni, Ruiyu 2017-06-01 15:39 ` Carsey, Jaben 2017-06-01 15:42 ` Carsey, Jaben 0 siblings, 2 replies; 11+ messages in thread From: Ni, Ruiyu @ 2017-06-01 15:18 UTC (permalink / raw) To: Carsey, Jaben, edk2-devel@lists.01.org Cc: Kinney, Michael D, Shah, Tapan (tapandshah@hpe.com) 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan (tapandshah@hpe.com) <tapandshah@hpe.com> >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: edk2-devel@lists.01.org >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D >> <michael.d.kinney@intel.com> >> 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 <ruiyu.ni@intel.com> >> Cc: Jaben Carsey <jaben.carsey@intel.com> >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-01 15:18 ` Ni, Ruiyu @ 2017-06-01 15:39 ` Carsey, Jaben 2017-06-01 15:42 ` Carsey, Jaben 1 sibling, 0 replies; 11+ messages in thread From: Carsey, Jaben @ 2017-06-01 15:39 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org Cc: Kinney, Michael D, Shah, Tapan (tapandshah@hpe.com) 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 <jaben.carsey@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > (tapandshah@hpe.com) <tapandshah@hpe.com> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > (tapandshah@hpe.com) <tapandshah@hpe.com> > >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: edk2-devel@lists.01.org > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D > >> <michael.d.kinney@intel.com> > >> 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 <ruiyu.ni@intel.com> > >> Cc: Jaben Carsey <jaben.carsey@intel.com> > >> Cc: Michael D Kinney <michael.d.kinney@intel.com> > >> --- > >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-01 15:18 ` Ni, Ruiyu 2017-06-01 15:39 ` Carsey, Jaben @ 2017-06-01 15:42 ` Carsey, Jaben 2017-06-02 2:31 ` Ni, Ruiyu 1 sibling, 1 reply; 11+ messages in thread From: Carsey, Jaben @ 2017-06-01 15:42 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org Cc: Kinney, Michael D, Shah, Tapan (tapandshah@hpe.com) 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > (tapandshah@hpe.com) <tapandshah@hpe.com> > 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 <jaben.carsey@intel.com>; edk2-devel@lists.01.org > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > > (tapandshah@hpe.com) <tapandshah@hpe.com> > > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > > (tapandshah@hpe.com) <tapandshah@hpe.com> > > >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: edk2-devel@lists.01.org > > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D > > >> <michael.d.kinney@intel.com> > > >> 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 <ruiyu.ni@intel.com> > > >> Cc: Jaben Carsey <jaben.carsey@intel.com> > > >> Cc: Michael D Kinney <michael.d.kinney@intel.com> > > >> --- > > >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-01 15:42 ` Carsey, Jaben @ 2017-06-02 2:31 ` Ni, Ruiyu 2017-06-02 15:04 ` Carsey, Jaben 0 siblings, 1 reply; 11+ messages in thread From: Ni, Ruiyu @ 2017-06-02 2:31 UTC (permalink / raw) To: Carsey, Jaben, edk2-devel@lists.01.org Cc: Kinney, Michael D, Shah, Tapan (tapandshah@hpe.com) 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_VOLATILE), StrSize(Command), (VOID*)Command)); } Regards, Ray >-----Original Message----- >From: Carsey, Jaben >Sent: Thursday, June 1, 2017 11:42 PM >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan (tapandshah@hpe.com) <tapandshah@hpe.com> >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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> (tapandshah@hpe.com) <tapandshah@hpe.com> >> 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 <jaben.carsey@intel.com>; edk2-devel@lists.01.org >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> > (tapandshah@hpe.com) <tapandshah@hpe.com> >> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> > (tapandshah@hpe.com) <tapandshah@hpe.com> >> > >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: edk2-devel@lists.01.org >> > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D >> > >> <michael.d.kinney@intel.com> >> > >> 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 <ruiyu.ni@intel.com> >> > >> Cc: Jaben Carsey <jaben.carsey@intel.com> >> > >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> > >> --- >> > >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-02 2:31 ` Ni, Ruiyu @ 2017-06-02 15:04 ` Carsey, Jaben 2017-06-02 16:12 ` Shah, Tapan 0 siblings, 1 reply; 11+ messages in thread From: Carsey, Jaben @ 2017-06-02 15:04 UTC (permalink / raw) To: Ni, Ruiyu, edk2-devel@lists.01.org Cc: Kinney, Michael D, Shah, Tapan (tapandshah@hpe.com) Ok. I see now. Thanks! Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > -----Original Message----- > From: Ni, Ruiyu > Sent: Thursday, June 01, 2017 7:32 PM > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > (tapandshah@hpe.com) <tapandshah@hpe.com> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > (tapandshah@hpe.com) <tapandshah@hpe.com> > >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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > >> (tapandshah@hpe.com) <tapandshah@hpe.com> > >> 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 <jaben.carsey@intel.com>; edk2-devel@lists.01.org > >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > >> > (tapandshah@hpe.com) <tapandshah@hpe.com> > >> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > >> > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > >> > (tapandshah@hpe.com) <tapandshah@hpe.com> > >> > >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: edk2-devel@lists.01.org > >> > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D > >> > >> <michael.d.kinney@intel.com> > >> > >> 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 <ruiyu.ni@intel.com> > >> > >> Cc: Jaben Carsey <jaben.carsey@intel.com> > >> > >> Cc: Michael D Kinney <michael.d.kinney@intel.com> > >> > >> --- > >> > >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-02 15:04 ` Carsey, Jaben @ 2017-06-02 16:12 ` Shah, Tapan 2017-06-04 0:47 ` Ni, Ruiyu 0 siblings, 1 reply; 11+ messages in thread From: Shah, Tapan @ 2017-06-02 16:12 UTC (permalink / raw) To: Carsey, Jaben, Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Kinney, Michael D 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:jaben.carsey@intel.com] Sent: Friday, June 02, 2017 10:04 AM To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan <tapandshah@hpe.com> Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias Ok. I see now. Thanks! Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > -----Original Message----- > From: Ni, Ruiyu > Sent: Thursday, June 01, 2017 7:32 PM > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > (tapandshah@hpe.com) <tapandshah@hpe.com> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > (tapandshah@hpe.com) <tapandshah@hpe.com> > >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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > >> (tapandshah@hpe.com) <tapandshah@hpe.com> > >> 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 <jaben.carsey@intel.com>; > >> > edk2-devel@lists.01.org > >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > >> > (tapandshah@hpe.com) <tapandshah@hpe.com> > >> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org > >> > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan > >> > (tapandshah@hpe.com) <tapandshah@hpe.com> > >> > >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: edk2-devel@lists.01.org > >> > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D > >> > >> <michael.d.kinney@intel.com> > >> > >> 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 <ruiyu.ni@intel.com> > >> > >> Cc: Jaben Carsey <jaben.carsey@intel.com> > >> > >> Cc: Michael D Kinney <michael.d.kinney@intel.com> > >> > >> --- > >> > >> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-02 16:12 ` Shah, Tapan @ 2017-06-04 0:47 ` Ni, Ruiyu 2017-06-05 15:25 ` Shah, Tapan 0 siblings, 1 reply; 11+ messages in thread From: Ni, Ruiyu @ 2017-06-04 0:47 UTC (permalink / raw) To: Shah, Tapan, Carsey, Jaben, edk2-devel@lists.01.org; +Cc: Kinney, Michael D 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:tapandshah@hpe.com] >Sent: Saturday, June 3, 2017 12:13 AM >To: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >Cc: Kinney, Michael D <michael.d.kinney@intel.com> >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:jaben.carsey@intel.com] >Sent: Friday, June 02, 2017 10:04 AM >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan <tapandshah@hpe.com> >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias > >Ok. I see now. Thanks! > >Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > >> -----Original Message----- >> From: Ni, Ruiyu >> Sent: Thursday, June 01, 2017 7:32 PM >> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> (tapandshah@hpe.com) <tapandshah@hpe.com> >> 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> (tapandshah@hpe.com) <tapandshah@hpe.com> >> >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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> >> (tapandshah@hpe.com) <tapandshah@hpe.com> >> >> 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 <jaben.carsey@intel.com>; >> >> > edk2-devel@lists.01.org >> >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> >> > (tapandshah@hpe.com) <tapandshah@hpe.com> >> >> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> >> > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> >> > (tapandshah@hpe.com) <tapandshah@hpe.com> >> >> > >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: edk2-devel@lists.01.org >> >> > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D >> >> > >> <michael.d.kinney@intel.com> >> >> > >> 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 <ruiyu.ni@intel.com> >> >> > >> Cc: Jaben Carsey <jaben.carsey@intel.com> >> >> > >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> >> > >> --- >> >> > >> 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ShellPkg/alias: Fix bug to support upper-case alias 2017-06-04 0:47 ` Ni, Ruiyu @ 2017-06-05 15:25 ` Shah, Tapan 0 siblings, 0 replies; 11+ messages in thread From: Shah, Tapan @ 2017-06-05 15:25 UTC (permalink / raw) To: Ni, Ruiyu, Carsey, Jaben, edk2-devel@lists.01.org; +Cc: Kinney, Michael D 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:ruiyu.ni@intel.com] Sent: Saturday, June 03, 2017 7:48 PM To: Shah, Tapan <tapandshah@hpe.com>; Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org Cc: Kinney, Michael D <michael.d.kinney@intel.com> 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:tapandshah@hpe.com] >Sent: Saturday, June 3, 2017 12:13 AM >To: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu ><ruiyu.ni@intel.com>; edk2-devel@lists.01.org >Cc: Kinney, Michael D <michael.d.kinney@intel.com> >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:jaben.carsey@intel.com] >Sent: Friday, June 02, 2017 10:04 AM >To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan ><tapandshah@hpe.com> >Subject: RE: [PATCH] ShellPkg/alias: Fix bug to support upper-case >alias > >Ok. I see now. Thanks! > >Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> > >> -----Original Message----- >> From: Ni, Ruiyu >> Sent: Thursday, June 01, 2017 7:32 PM >> To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> (tapandshah@hpe.com) <tapandshah@hpe.com> >> 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> (tapandshah@hpe.com) <tapandshah@hpe.com> >> >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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> >> (tapandshah@hpe.com) <tapandshah@hpe.com> >> >> 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 <jaben.carsey@intel.com>; >> >> > edk2-devel@lists.01.org >> >> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> >> > (tapandshah@hpe.com) <tapandshah@hpe.com> >> >> > 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 <ruiyu.ni@intel.com>; edk2-devel@lists.01.org >> >> > >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Shah, Tapan >> >> > (tapandshah@hpe.com) <tapandshah@hpe.com> >> >> > >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: edk2-devel@lists.01.org >> >> > >> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Kinney, Michael D >> >> > >> <michael.d.kinney@intel.com> >> >> > >> 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 <ruiyu.ni@intel.com> >> >> > >> Cc: Jaben Carsey <jaben.carsey@intel.com> >> >> > >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> >> > >> --- >> >> > >> 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-06-05 15:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-06-01 14:11 [PATCH] ShellPkg/alias: Fix bug to support upper-case alias Ruiyu Ni 2017-06-01 14:14 ` Ni, Ruiyu 2017-06-01 15:01 ` Carsey, Jaben 2017-06-01 15:18 ` Ni, Ruiyu 2017-06-01 15:39 ` Carsey, Jaben 2017-06-01 15:42 ` Carsey, Jaben 2017-06-02 2:31 ` Ni, Ruiyu 2017-06-02 15:04 ` Carsey, Jaben 2017-06-02 16:12 ` Shah, Tapan 2017-06-04 0:47 ` Ni, Ruiyu 2017-06-05 15:25 ` Shah, Tapan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox