[PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line

[PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line

[Vladimir Olovyannikov]

The current implementation of ShellSubstituteVariables() does not allow
substitution of variables names containing another variable name(s)
between %...%

Example: %text%var_name%% where var_name variable contains 0.
Here the variable value which should be returned on substitution
would be contents of text0 variable.
The current implementation would return nothing as %text0% would be
eliminated by StripUnreplacedEnvironmentVariables().

Modify the code so that StripUnreplacedEnvironmentVariables checks if
variable between %...% really exists. If it does not, delete %...%.
If it exists, preserve it and tell the caller that another run of
ShellConvertVariables() is needed. This way, any nested variable
between %...% can be easily retrieved.
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2452

Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
Contributed-under: TianoCore Contribution Agreement 1.1
---
 ShellPkg/Application/Shell/Shell.c | 71 ++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 14 deletions(-)

diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
index d16adae0ea30..3756a71794d1 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1510,12 +1510,16 @@ ShellConvertAlias(
 
 /**
   This function will eliminate unreplaced (and therefore non-found) environment variables.
-
+  If a variable exists between %...%, it will be preserved, and VarExists
+  would be TRUE.
   @param[in,out] CmdLine   The command line to update.
+  @param[out] VarExists    A pointer to the BOOLEAN which is set if
+                           a Shell variable between %...% exists.
 **/
 EFI_STATUS
 StripUnreplacedEnvironmentVariables(
-  IN OUT CHAR16 *CmdLine
+  IN OUT CHAR16 *CmdLine,
+  OUT BOOLEAN   *VarExists
   )
 {
   CHAR16 *FirstPercent;
@@ -1558,12 +1562,38 @@ StripUnreplacedEnvironmentVariables(
     if (FirstQuote == NULL || SecondPercent < FirstQuote) {
       if (IsValidEnvironmentVariableName(FirstPercent, SecondPercent)) {
         //
-        // We need to remove from FirstPercent to SecondPercent
-        //
-        CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent + 1));
-        //
-        // don't need to update the locator.  both % characters are gone.
+        // If this Shell variable exists, consider that another run is needed.
+        // For example, consider a variable %var%var2%% when var2 is 0.
+        // After the first run, it becomes %var0%, and after the second run
+        // it contains the value of var0 variable.
+        // Eliminate variable if it does not exist.
         //
+        CHAR16 *VarName;
+
+        // Consider NULL terminator as well
+        VarName = (CHAR16 *)AllocateZeroPool((SecondPercent - FirstPercent) *
+                                             sizeof(CHAR16));
+        if (VarName) {
+          CopyMem(VarName, FirstPercent + 1,
+                  (SecondPercent - FirstPercent - 1) * sizeof(CHAR16));
+        }
+
+        if (!VarName || !ShellGetEnvironmentVariable(VarName)) {
+          //
+          // We need to remove from FirstPercent to SecondPercent.
+          //
+          CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent + 1));
+          //
+          // don't need to update the locator.  both % characters are gone.
+          //
+        } else {
+          *VarExists = TRUE;
+          //
+          // In this case, locator needs to be updated as % characters present.
+          //
+          CurrentLocator = SecondPercent + 1;
+        }
+        SHELL_FREE_NON_NULL(VarName);
       } else {
         CurrentLocator = SecondPercent + 1;
       }
@@ -1581,13 +1611,18 @@ StripUnreplacedEnvironmentVariables(
   If the return value is not NULL the memory must be caller freed.
 
   @param[in] OriginalCommandLine    The original command line
+  @param[out] NeedExtraRun          Indication that the caller needs to repeat
+                                    this call to convert variables as there are
+                                    existing variable(s) between %..%
+                                    present after the call.
 
   @retval NULL                      An error occurred.
   @return                           The new command line with no environment variables present.
 **/
 CHAR16*
 ShellConvertVariables (
-  IN CONST CHAR16 *OriginalCommandLine
+  IN CONST CHAR16 *OriginalCommandLine,
+  OUT BOOLEAN     *NeedExtraRun
   )
 {
   CONST CHAR16        *MasterEnvList;
@@ -1601,11 +1636,13 @@ ShellConvertVariables (
   ALIAS_LIST          *AliasListNode;
 
   ASSERT(OriginalCommandLine != NULL);
+  ASSERT(NeedExtraRun != NULL);
 
   ItemSize          = 0;
   NewSize           = StrSize(OriginalCommandLine);
   CurrentScriptFile = ShellCommandGetCurrentScriptFile();
   Temp              = NULL;
+  *NeedExtraRun     = FALSE;
 
   ///@todo update this to handle the %0 - %9 for scripting only (borrow from line 1256 area) ? ? ?
 
@@ -1698,7 +1735,7 @@ ShellConvertVariables (
   //
   // Remove non-existent environment variables
   //
-  StripUnreplacedEnvironmentVariables(NewCommandLine1);
+  StripUnreplacedEnvironmentVariables(NewCommandLine1, NeedExtraRun);
 
   //
   // Now cleanup any straggler intentionally ignored "%" characters
@@ -1845,12 +1882,18 @@ ShellSubstituteVariables(
   )
 {
   CHAR16      *NewCmdLine;
-  NewCmdLine = ShellConvertVariables(*CmdLine);
-  SHELL_FREE_NON_NULL(*CmdLine);
-  if (NewCmdLine == NULL) {
-    return (EFI_OUT_OF_RESOURCES);
+  BOOLEAN     NeedExtraRun;
+
+  NeedExtraRun = TRUE;
+  while (NeedExtraRun) {
+    NewCmdLine = ShellConvertVariables(*CmdLine, &NeedExtraRun);
+    SHELL_FREE_NON_NULL(*CmdLine);
+    if (NewCmdLine == NULL) {
+      return (EFI_OUT_OF_RESOURCES);
+    }
+    *CmdLine = NewCmdLine;
   }
-  *CmdLine = NewCmdLine;
+  
   return (EFI_SUCCESS);
 }
 
-- 
2.17.1

Re: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line

[Ni, Ray]

Vladimir,
Is this enhancement an extension to Shell spec or violation of Shell spec?
If it's an extension, I am happy to have such feature implemented.
Otherwise, we may need to discuss in uefi.org.

Can you list the unit tests you have done?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vladimir Olovyannikov via Groups.Io
> Sent: Friday, January 10, 2020 6:07 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line
> 
> The current implementation of ShellSubstituteVariables() does not allow
> substitution of variables names containing another variable name(s)
> between %...%
> 
> Example: %text%var_name%% where var_name variable contains 0.
> Here the variable value which should be returned on substitution
> would be contents of text0 variable.
> The current implementation would return nothing as %text0% would be
> eliminated by StripUnreplacedEnvironmentVariables().
> 
> Modify the code so that StripUnreplacedEnvironmentVariables checks if
> variable between %...% really exists. If it does not, delete %...%.
> If it exists, preserve it and tell the caller that another run of
> ShellConvertVariables() is needed. This way, any nested variable
> between %...% can be easily retrieved.
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2452
> 
> Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> ---
>  ShellPkg/Application/Shell/Shell.c | 71 ++++++++++++++++++++++++------
>  1 file changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
> index d16adae0ea30..3756a71794d1 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1510,12 +1510,16 @@ ShellConvertAlias(
> 
>  /**
>    This function will eliminate unreplaced (and therefore non-found) environment variables.
> -
> +  If a variable exists between %...%, it will be preserved, and VarExists
> +  would be TRUE.
>    @param[in,out] CmdLine   The command line to update.
> +  @param[out] VarExists    A pointer to the BOOLEAN which is set if
> +                           a Shell variable between %...% exists.
>  **/
>  EFI_STATUS
>  StripUnreplacedEnvironmentVariables(
> -  IN OUT CHAR16 *CmdLine
> +  IN OUT CHAR16 *CmdLine,
> +  OUT BOOLEAN   *VarExists
>    )
>  {
>    CHAR16 *FirstPercent;
> @@ -1558,12 +1562,38 @@ StripUnreplacedEnvironmentVariables(
>      if (FirstQuote == NULL || SecondPercent < FirstQuote) {
>        if (IsValidEnvironmentVariableName(FirstPercent, SecondPercent)) {
>          //
> -        // We need to remove from FirstPercent to SecondPercent
> -        //
> -        CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent + 1));
> -        //
> -        // don't need to update the locator.  both % characters are gone.
> +        // If this Shell variable exists, consider that another run is needed.
> +        // For example, consider a variable %var%var2%% when var2 is 0.
> +        // After the first run, it becomes %var0%, and after the second run
> +        // it contains the value of var0 variable.
> +        // Eliminate variable if it does not exist.
>          //
> +        CHAR16 *VarName;
> +
> +        // Consider NULL terminator as well
> +        VarName = (CHAR16 *)AllocateZeroPool((SecondPercent - FirstPercent) *
> +                                             sizeof(CHAR16));
> +        if (VarName) {
> +          CopyMem(VarName, FirstPercent + 1,
> +                  (SecondPercent - FirstPercent - 1) * sizeof(CHAR16));
> +        }
> +
> +        if (!VarName || !ShellGetEnvironmentVariable(VarName)) {
> +          //
> +          // We need to remove from FirstPercent to SecondPercent.
> +          //
> +          CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent + 1));
> +          //
> +          // don't need to update the locator.  both % characters are gone.
> +          //
> +        } else {
> +          *VarExists = TRUE;
> +          //
> +          // In this case, locator needs to be updated as % characters present.
> +          //
> +          CurrentLocator = SecondPercent + 1;
> +        }
> +        SHELL_FREE_NON_NULL(VarName);
>        } else {
>          CurrentLocator = SecondPercent + 1;
>        }
> @@ -1581,13 +1611,18 @@ StripUnreplacedEnvironmentVariables(
>    If the return value is not NULL the memory must be caller freed.
> 
>    @param[in] OriginalCommandLine    The original command line
> +  @param[out] NeedExtraRun          Indication that the caller needs to repeat
> +                                    this call to convert variables as there are
> +                                    existing variable(s) between %..%
> +                                    present after the call.
> 
>    @retval NULL                      An error occurred.
>    @return                           The new command line with no environment variables present.
>  **/
>  CHAR16*
>  ShellConvertVariables (
> -  IN CONST CHAR16 *OriginalCommandLine
> +  IN CONST CHAR16 *OriginalCommandLine,
> +  OUT BOOLEAN     *NeedExtraRun
>    )
>  {
>    CONST CHAR16        *MasterEnvList;
> @@ -1601,11 +1636,13 @@ ShellConvertVariables (
>    ALIAS_LIST          *AliasListNode;
> 
>    ASSERT(OriginalCommandLine != NULL);
> +  ASSERT(NeedExtraRun != NULL);
> 
>    ItemSize          = 0;
>    NewSize           = StrSize(OriginalCommandLine);
>    CurrentScriptFile = ShellCommandGetCurrentScriptFile();
>    Temp              = NULL;
> +  *NeedExtraRun     = FALSE;
> 
>    ///@todo update this to handle the %0 - %9 for scripting only (borrow from line 1256 area) ? ? ?
> 
> @@ -1698,7 +1735,7 @@ ShellConvertVariables (
>    //
>    // Remove non-existent environment variables
>    //
> -  StripUnreplacedEnvironmentVariables(NewCommandLine1);
> +  StripUnreplacedEnvironmentVariables(NewCommandLine1, NeedExtraRun);
> 
>    //
>    // Now cleanup any straggler intentionally ignored "%" characters
> @@ -1845,12 +1882,18 @@ ShellSubstituteVariables(
>    )
>  {
>    CHAR16      *NewCmdLine;
> -  NewCmdLine = ShellConvertVariables(*CmdLine);
> -  SHELL_FREE_NON_NULL(*CmdLine);
> -  if (NewCmdLine == NULL) {
> -    return (EFI_OUT_OF_RESOURCES);
> +  BOOLEAN     NeedExtraRun;
> +
> +  NeedExtraRun = TRUE;
> +  while (NeedExtraRun) {
> +    NewCmdLine = ShellConvertVariables(*CmdLine, &NeedExtraRun);
> +    SHELL_FREE_NON_NULL(*CmdLine);
> +    if (NewCmdLine == NULL) {
> +      return (EFI_OUT_OF_RESOURCES);
> +    }
> +    *CmdLine = NewCmdLine;
>    }
> -  *CmdLine = NewCmdLine;
> +
>    return (EFI_SUCCESS);
>  }
> 
> --
> 2.17.1
> 
> 
> 

Re: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line

[Vladimir Olovyannikov]

Hi Ray,

As an example, let's say that you have multiple variants of serverips -
each interface has its own, and each interface has a property as say
interface no (eth_no).
It is logical to address a serverip for an interface as
%serverip%eth_no%%, and in the end get the contents of variable serverip0
(say for eth_no containing value 0).
In fact, Linux bash allows doing that easily.
We are doing most of our upgrade/configuration stuff via Shell scripts
which is very flexible, easy and efficient.
To support boot redundancy, we support multiple slots for kernel/dtbs,
etc, which are addressed as
%update_type_%upd_type%% where upd_type can be dtb, kernel, etc.
The spec says " Environment variables can be used on the command-line by
using %variable-name%
where variable-name is the environment variable's name. Variable
substitution is not recursive."
I treat it as an extension to %var% case; %var case is not touched. What
do you think?

Regarding unit tests.
I verified variables substitutions using %var% and %var%var1%% and
%%var1%var% to make sure there would be no regression.
Our scripts use variables extensively, and no issues were encountered.
Please let me know if you need the scripts we are using, and I will send
them to you.

Which unit tests are available?

Thank you,
Vladimir

-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Thursday, January 9, 2020 10:03 PM
To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com; Gao, Zhichao
<zhichao.gao@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested
variables substitution from cmd line

Vladimir,
Is this enhancement an extension to Shell spec or violation of Shell spec?
If it's an extension, I am happy to have such feature implemented.
Otherwise, we may need to discuss in uefi.org.

Can you list the unit tests you have done?

Thanks,
Ray

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Vladimir Olovyannikov via Groups.Io
> Sent: Friday, January 10, 2020 6:07 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested
> variables substitution from cmd line
>
> The current implementation of ShellSubstituteVariables() does not
> allow substitution of variables names containing another variable
> name(s) between %...%
>
> Example: %text%var_name%% where var_name variable contains 0.
> Here the variable value which should be returned on substitution would
> be contents of text0 variable.
> The current implementation would return nothing as %text0% would be
> eliminated by StripUnreplacedEnvironmentVariables().
>
> Modify the code so that StripUnreplacedEnvironmentVariables checks if
> variable between %...% really exists. If it does not, delete %...%.
> If it exists, preserve it and tell the caller that another run of
> ShellConvertVariables() is needed. This way, any nested variable
> between %...% can be easily retrieved.
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2452
>
> Signed-off-by: Vladimir Olovyannikov
> <vladimir.olovyannikov@broadcom.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> ---
>  ShellPkg/Application/Shell/Shell.c | 71
> ++++++++++++++++++++++++------
>  1 file changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/ShellPkg/Application/Shell/Shell.c
> b/ShellPkg/Application/Shell/Shell.c
> index d16adae0ea30..3756a71794d1 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1510,12 +1510,16 @@ ShellConvertAlias(
>
>  /**
>    This function will eliminate unreplaced (and therefore non-found)
environment variables.
> -
> +  If a variable exists between %...%, it will be preserved, and
> + VarExists  would be TRUE.
>    @param[in,out] CmdLine   The command line to update.
> +  @param[out] VarExists    A pointer to the BOOLEAN which is set if
> +                           a Shell variable between %...% exists.
>  **/
>  EFI_STATUS
>  StripUnreplacedEnvironmentVariables(
> -  IN OUT CHAR16 *CmdLine
> +  IN OUT CHAR16 *CmdLine,
> +  OUT BOOLEAN   *VarExists
>    )
>  {
>    CHAR16 *FirstPercent;
> @@ -1558,12 +1562,38 @@ StripUnreplacedEnvironmentVariables(
>      if (FirstQuote == NULL || SecondPercent < FirstQuote) {
>        if (IsValidEnvironmentVariableName(FirstPercent, SecondPercent))
{
>          //
> -        // We need to remove from FirstPercent to SecondPercent
> -        //
> -        CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent
+ 1));
> -        //
> -        // don't need to update the locator.  both % characters are
gone.
> +        // If this Shell variable exists, consider that another run is
needed.
> +        // For example, consider a variable %var%var2%% when var2 is 0.
> +        // After the first run, it becomes %var0%, and after the second
run
> +        // it contains the value of var0 variable.
> +        // Eliminate variable if it does not exist.
>          //
> +        CHAR16 *VarName;
> +
> +        // Consider NULL terminator as well
> +        VarName = (CHAR16 *)AllocateZeroPool((SecondPercent -
FirstPercent) *
> +                                             sizeof(CHAR16));
> +        if (VarName) {
> +          CopyMem(VarName, FirstPercent + 1,
> +                  (SecondPercent - FirstPercent - 1) * sizeof(CHAR16));
> +        }
> +
> +        if (!VarName || !ShellGetEnvironmentVariable(VarName)) {
> +          //
> +          // We need to remove from FirstPercent to SecondPercent.
> +          //
> +          CopyMem(FirstPercent, SecondPercent + 1,
StrSize(SecondPercent + 1));
> +          //
> +          // don't need to update the locator.  both % characters are
gone.
> +          //
> +        } else {
> +          *VarExists = TRUE;
> +          //
> +          // In this case, locator needs to be updated as % characters
present.
> +          //
> +          CurrentLocator = SecondPercent + 1;
> +        }
> +        SHELL_FREE_NON_NULL(VarName);
>        } else {
>          CurrentLocator = SecondPercent + 1;
>        }
> @@ -1581,13 +1611,18 @@ StripUnreplacedEnvironmentVariables(
>    If the return value is not NULL the memory must be caller freed.
>
>    @param[in] OriginalCommandLine    The original command line
> +  @param[out] NeedExtraRun          Indication that the caller needs to
repeat
> +                                    this call to convert variables as
there are
> +                                    existing variable(s) between %..%
> +                                    present after the call.
>
>    @retval NULL                      An error occurred.
>    @return                           The new command line with no
environment variables present.
>  **/
>  CHAR16*
>  ShellConvertVariables (
> -  IN CONST CHAR16 *OriginalCommandLine
> +  IN CONST CHAR16 *OriginalCommandLine,
> +  OUT BOOLEAN     *NeedExtraRun
>    )
>  {
>    CONST CHAR16        *MasterEnvList;
> @@ -1601,11 +1636,13 @@ ShellConvertVariables (
>    ALIAS_LIST          *AliasListNode;
>
>    ASSERT(OriginalCommandLine != NULL);
> +  ASSERT(NeedExtraRun != NULL);
>
>    ItemSize          = 0;
>    NewSize           = StrSize(OriginalCommandLine);
>    CurrentScriptFile = ShellCommandGetCurrentScriptFile();
>    Temp              = NULL;
> +  *NeedExtraRun     = FALSE;
>
>    ///@todo update this to handle the %0 - %9 for scripting only (borrow
from line 1256 area) ? ? ?
>
> @@ -1698,7 +1735,7 @@ ShellConvertVariables (
>    //
>    // Remove non-existent environment variables
>    //
> -  StripUnreplacedEnvironmentVariables(NewCommandLine1);
> +  StripUnreplacedEnvironmentVariables(NewCommandLine1, NeedExtraRun);
>
>    //
>    // Now cleanup any straggler intentionally ignored "%" characters
> @@ -1845,12 +1882,18 @@ ShellSubstituteVariables(
>    )
>  {
>    CHAR16      *NewCmdLine;
> -  NewCmdLine = ShellConvertVariables(*CmdLine);
> -  SHELL_FREE_NON_NULL(*CmdLine);
> -  if (NewCmdLine == NULL) {
> -    return (EFI_OUT_OF_RESOURCES);
> +  BOOLEAN     NeedExtraRun;
> +
> +  NeedExtraRun = TRUE;
> +  while (NeedExtraRun) {
> +    NewCmdLine = ShellConvertVariables(*CmdLine, &NeedExtraRun);
> +    SHELL_FREE_NON_NULL(*CmdLine);
> +    if (NewCmdLine == NULL) {
> +      return (EFI_OUT_OF_RESOURCES);
> +    }
> +    *CmdLine = NewCmdLine;
>    }
> -  *CmdLine = NewCmdLine;
> +
>    return (EFI_SUCCESS);
>  }
>
> --
> 2.17.1
>
>
> 

Re: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line

[Liming Gao]

Please remove Contributed-under: TianoCore Contribution Agreement 1.1 in the commit message, this is not required now. 

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vladimir Olovyannikov via Groups.Io
> Sent: Friday, January 10, 2020 6:07 AM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Subject: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line
> 
> The current implementation of ShellSubstituteVariables() does not allow
> substitution of variables names containing another variable name(s)
> between %...%
> 
> Example: %text%var_name%% where var_name variable contains 0.
> Here the variable value which should be returned on substitution
> would be contents of text0 variable.
> The current implementation would return nothing as %text0% would be
> eliminated by StripUnreplacedEnvironmentVariables().
> 
> Modify the code so that StripUnreplacedEnvironmentVariables checks if
> variable between %...% really exists. If it does not, delete %...%.
> If it exists, preserve it and tell the caller that another run of
> ShellConvertVariables() is needed. This way, any nested variable
> between %...% can be easily retrieved.
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2452
> 
> Signed-off-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> ---
>  ShellPkg/Application/Shell/Shell.c | 71 ++++++++++++++++++++++++------
>  1 file changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c
> index d16adae0ea30..3756a71794d1 100644
> --- a/ShellPkg/Application/Shell/Shell.c
> +++ b/ShellPkg/Application/Shell/Shell.c
> @@ -1510,12 +1510,16 @@ ShellConvertAlias(
> 
>  /**
>    This function will eliminate unreplaced (and therefore non-found) environment variables.
> -
> +  If a variable exists between %...%, it will be preserved, and VarExists
> +  would be TRUE.
>    @param[in,out] CmdLine   The command line to update.
> +  @param[out] VarExists    A pointer to the BOOLEAN which is set if
> +                           a Shell variable between %...% exists.
>  **/
>  EFI_STATUS
>  StripUnreplacedEnvironmentVariables(
> -  IN OUT CHAR16 *CmdLine
> +  IN OUT CHAR16 *CmdLine,
> +  OUT BOOLEAN   *VarExists
>    )
>  {
>    CHAR16 *FirstPercent;
> @@ -1558,12 +1562,38 @@ StripUnreplacedEnvironmentVariables(
>      if (FirstQuote == NULL || SecondPercent < FirstQuote) {
>        if (IsValidEnvironmentVariableName(FirstPercent, SecondPercent)) {
>          //
> -        // We need to remove from FirstPercent to SecondPercent
> -        //
> -        CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent + 1));
> -        //
> -        // don't need to update the locator.  both % characters are gone.
> +        // If this Shell variable exists, consider that another run is needed.
> +        // For example, consider a variable %var%var2%% when var2 is 0.
> +        // After the first run, it becomes %var0%, and after the second run
> +        // it contains the value of var0 variable.
> +        // Eliminate variable if it does not exist.
>          //
> +        CHAR16 *VarName;
> +
> +        // Consider NULL terminator as well
> +        VarName = (CHAR16 *)AllocateZeroPool((SecondPercent - FirstPercent) *
> +                                             sizeof(CHAR16));
> +        if (VarName) {
> +          CopyMem(VarName, FirstPercent + 1,
> +                  (SecondPercent - FirstPercent - 1) * sizeof(CHAR16));
> +        }
> +
> +        if (!VarName || !ShellGetEnvironmentVariable(VarName)) {
> +          //
> +          // We need to remove from FirstPercent to SecondPercent.
> +          //
> +          CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent + 1));
> +          //
> +          // don't need to update the locator.  both % characters are gone.
> +          //
> +        } else {
> +          *VarExists = TRUE;
> +          //
> +          // In this case, locator needs to be updated as % characters present.
> +          //
> +          CurrentLocator = SecondPercent + 1;
> +        }
> +        SHELL_FREE_NON_NULL(VarName);
>        } else {
>          CurrentLocator = SecondPercent + 1;
>        }
> @@ -1581,13 +1611,18 @@ StripUnreplacedEnvironmentVariables(
>    If the return value is not NULL the memory must be caller freed.
> 
>    @param[in] OriginalCommandLine    The original command line
> +  @param[out] NeedExtraRun          Indication that the caller needs to repeat
> +                                    this call to convert variables as there are
> +                                    existing variable(s) between %..%
> +                                    present after the call.
> 
>    @retval NULL                      An error occurred.
>    @return                           The new command line with no environment variables present.
>  **/
>  CHAR16*
>  ShellConvertVariables (
> -  IN CONST CHAR16 *OriginalCommandLine
> +  IN CONST CHAR16 *OriginalCommandLine,
> +  OUT BOOLEAN     *NeedExtraRun
>    )
>  {
>    CONST CHAR16        *MasterEnvList;
> @@ -1601,11 +1636,13 @@ ShellConvertVariables (
>    ALIAS_LIST          *AliasListNode;
> 
>    ASSERT(OriginalCommandLine != NULL);
> +  ASSERT(NeedExtraRun != NULL);
> 
>    ItemSize          = 0;
>    NewSize           = StrSize(OriginalCommandLine);
>    CurrentScriptFile = ShellCommandGetCurrentScriptFile();
>    Temp              = NULL;
> +  *NeedExtraRun     = FALSE;
> 
>    ///@todo update this to handle the %0 - %9 for scripting only (borrow from line 1256 area) ? ? ?
> 
> @@ -1698,7 +1735,7 @@ ShellConvertVariables (
>    //
>    // Remove non-existent environment variables
>    //
> -  StripUnreplacedEnvironmentVariables(NewCommandLine1);
> +  StripUnreplacedEnvironmentVariables(NewCommandLine1, NeedExtraRun);
> 
>    //
>    // Now cleanup any straggler intentionally ignored "%" characters
> @@ -1845,12 +1882,18 @@ ShellSubstituteVariables(
>    )
>  {
>    CHAR16      *NewCmdLine;
> -  NewCmdLine = ShellConvertVariables(*CmdLine);
> -  SHELL_FREE_NON_NULL(*CmdLine);
> -  if (NewCmdLine == NULL) {
> -    return (EFI_OUT_OF_RESOURCES);
> +  BOOLEAN     NeedExtraRun;
> +
> +  NeedExtraRun = TRUE;
> +  while (NeedExtraRun) {
> +    NewCmdLine = ShellConvertVariables(*CmdLine, &NeedExtraRun);
> +    SHELL_FREE_NON_NULL(*CmdLine);
> +    if (NewCmdLine == NULL) {
> +      return (EFI_OUT_OF_RESOURCES);
> +    }
> +    *CmdLine = NewCmdLine;
>    }
> -  *CmdLine = NewCmdLine;
> +
>    return (EFI_SUCCESS);
>  }
> 
> --
> 2.17.1
> 
> 
> 

Re: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line

[Gao, Zhichao]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> Sent: Saturday, January 11, 2020 12:33 AM
> To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested
> variables substitution from cmd line
> 
> Hi Ray,
> 
> As an example, let's say that you have multiple variants of serverips - each
> interface has its own, and each interface has a property as say interface no
> (eth_no).
> It is logical to address a serverip for an interface as %serverip%eth_no%%, and
> in the end get the contents of variable serverip0 (say for eth_no containing value
> 0).
> In fact, Linux bash allows doing that easily.
> We are doing most of our upgrade/configuration stuff via Shell scripts which is
> very flexible, easy and efficient.
> To support boot redundancy, we support multiple slots for kernel/dtbs, etc,
> which are addressed as %update_type_%upd_type%% where upd_type can be
> dtb, kernel, etc.
> The spec says " Environment variables can be used on the command-line by
> using %variable-name% where variable-name is the environment variable's
> name. Variable substitution is not recursive."
> I treat it as an extension to %var% case; %var case is not touched. What do you
> think?

"Variable substitution is not recursive" means the enviornment variable doesn't support nested. If you want to push the change, you should change the spec first. And there is one variable substitution flow figure, it should be update as well.

Thanks,
Zhichao

> 
> Regarding unit tests.
> I verified variables substitutions using %var% and %var%var1%%
> and %%var1%var% to make sure there would be no regression.
> Our scripts use variables extensively, and no issues were encountered.
> Please let me know if you need the scripts we are using, and I will send them to
> you.
> 
> Which unit tests are available?
> 
> Thank you,
> Vladimir
> 
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Thursday, January 9, 2020 10:03 PM
> To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com; Gao,
> Zhichao <zhichao.gao@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested
> variables substitution from cmd line
> 
> Vladimir,
> Is this enhancement an extension to Shell spec or violation of Shell spec?
> If it's an extension, I am happy to have such feature implemented.
> Otherwise, we may need to discuss in uefi.org.
> 
> Can you list the unit tests you have done?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Vladimir Olovyannikov via Groups.Io
> > Sent: Friday, January 10, 2020 6:07 AM
> > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > Subject: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested
> > variables substitution from cmd line
> >
> > The current implementation of ShellSubstituteVariables() does not
> > allow substitution of variables names containing another variable
> > name(s) between %...%
> >
> > Example: %text%var_name%% where var_name variable contains 0.
> > Here the variable value which should be returned on substitution would
> > be contents of text0 variable.
> > The current implementation would return nothing as %text0% would be
> > eliminated by StripUnreplacedEnvironmentVariables().
> >
> > Modify the code so that StripUnreplacedEnvironmentVariables checks if
> > variable between %...% really exists. If it does not, delete %...%.
> > If it exists, preserve it and tell the caller that another run of
> > ShellConvertVariables() is needed. This way, any nested variable
> > between %...% can be easily retrieved.
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2452
> >
> > Signed-off-by: Vladimir Olovyannikov
> > <vladimir.olovyannikov@broadcom.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > ---
> >  ShellPkg/Application/Shell/Shell.c | 71
> > ++++++++++++++++++++++++------
> >  1 file changed, 57 insertions(+), 14 deletions(-)
> >
> > diff --git a/ShellPkg/Application/Shell/Shell.c
> > b/ShellPkg/Application/Shell/Shell.c
> > index d16adae0ea30..3756a71794d1 100644
> > --- a/ShellPkg/Application/Shell/Shell.c
> > +++ b/ShellPkg/Application/Shell/Shell.c
> > @@ -1510,12 +1510,16 @@ ShellConvertAlias(
> >
> >  /**
> >    This function will eliminate unreplaced (and therefore non-found)
> environment variables.
> > -
> > +  If a variable exists between %...%, it will be preserved, and
> > + VarExists  would be TRUE.
> >    @param[in,out] CmdLine   The command line to update.
> > +  @param[out] VarExists    A pointer to the BOOLEAN which is set if
> > +                           a Shell variable between %...% exists.
> >  **/
> >  EFI_STATUS
> >  StripUnreplacedEnvironmentVariables(
> > -  IN OUT CHAR16 *CmdLine
> > +  IN OUT CHAR16 *CmdLine,
> > +  OUT BOOLEAN   *VarExists
> >    )
> >  {
> >    CHAR16 *FirstPercent;
> > @@ -1558,12 +1562,38 @@ StripUnreplacedEnvironmentVariables(
> >      if (FirstQuote == NULL || SecondPercent < FirstQuote) {
> >        if (IsValidEnvironmentVariableName(FirstPercent,
> > SecondPercent))
> {
> >          //
> > -        // We need to remove from FirstPercent to SecondPercent
> > -        //
> > -        CopyMem(FirstPercent, SecondPercent + 1, StrSize(SecondPercent
> + 1));
> > -        //
> > -        // don't need to update the locator.  both % characters are
> gone.
> > +        // If this Shell variable exists, consider that another run
> > + is
> needed.
> > +        // For example, consider a variable %var%var2%% when var2 is 0.
> > +        // After the first run, it becomes %var0%, and after the
> > + second
> run
> > +        // it contains the value of var0 variable.
> > +        // Eliminate variable if it does not exist.
> >          //
> > +        CHAR16 *VarName;
> > +
> > +        // Consider NULL terminator as well
> > +        VarName = (CHAR16 *)AllocateZeroPool((SecondPercent -
> FirstPercent) *
> > +                                             sizeof(CHAR16));
> > +        if (VarName) {
> > +          CopyMem(VarName, FirstPercent + 1,
> > +                  (SecondPercent - FirstPercent - 1) * sizeof(CHAR16));
> > +        }
> > +
> > +        if (!VarName || !ShellGetEnvironmentVariable(VarName)) {
> > +          //
> > +          // We need to remove from FirstPercent to SecondPercent.
> > +          //
> > +          CopyMem(FirstPercent, SecondPercent + 1,
> StrSize(SecondPercent + 1));
> > +          //
> > +          // don't need to update the locator.  both % characters are
> gone.
> > +          //
> > +        } else {
> > +          *VarExists = TRUE;
> > +          //
> > +          // In this case, locator needs to be updated as %
> > + characters
> present.
> > +          //
> > +          CurrentLocator = SecondPercent + 1;
> > +        }
> > +        SHELL_FREE_NON_NULL(VarName);
> >        } else {
> >          CurrentLocator = SecondPercent + 1;
> >        }
> > @@ -1581,13 +1611,18 @@ StripUnreplacedEnvironmentVariables(
> >    If the return value is not NULL the memory must be caller freed.
> >
> >    @param[in] OriginalCommandLine    The original command line
> > +  @param[out] NeedExtraRun          Indication that the caller needs to
> repeat
> > +                                    this call to convert variables as
> there are
> > +                                    existing variable(s) between %..%
> > +                                    present after the call.
> >
> >    @retval NULL                      An error occurred.
> >    @return                           The new command line with no
> environment variables present.
> >  **/
> >  CHAR16*
> >  ShellConvertVariables (
> > -  IN CONST CHAR16 *OriginalCommandLine
> > +  IN CONST CHAR16 *OriginalCommandLine,
> > +  OUT BOOLEAN     *NeedExtraRun
> >    )
> >  {
> >    CONST CHAR16        *MasterEnvList;
> > @@ -1601,11 +1636,13 @@ ShellConvertVariables (
> >    ALIAS_LIST          *AliasListNode;
> >
> >    ASSERT(OriginalCommandLine != NULL);
> > +  ASSERT(NeedExtraRun != NULL);
> >
> >    ItemSize          = 0;
> >    NewSize           = StrSize(OriginalCommandLine);
> >    CurrentScriptFile = ShellCommandGetCurrentScriptFile();
> >    Temp              = NULL;
> > +  *NeedExtraRun     = FALSE;
> >
> >    ///@todo update this to handle the %0 - %9 for scripting only
> > (borrow
> from line 1256 area) ? ? ?
> >
> > @@ -1698,7 +1735,7 @@ ShellConvertVariables (
> >    //
> >    // Remove non-existent environment variables
> >    //
> > -  StripUnreplacedEnvironmentVariables(NewCommandLine1);
> > +  StripUnreplacedEnvironmentVariables(NewCommandLine1, NeedExtraRun);
> >
> >    //
> >    // Now cleanup any straggler intentionally ignored "%" characters
> > @@ -1845,12 +1882,18 @@ ShellSubstituteVariables(
> >    )
> >  {
> >    CHAR16      *NewCmdLine;
> > -  NewCmdLine = ShellConvertVariables(*CmdLine);
> > -  SHELL_FREE_NON_NULL(*CmdLine);
> > -  if (NewCmdLine == NULL) {
> > -    return (EFI_OUT_OF_RESOURCES);
> > +  BOOLEAN     NeedExtraRun;
> > +
> > +  NeedExtraRun = TRUE;
> > +  while (NeedExtraRun) {
> > +    NewCmdLine = ShellConvertVariables(*CmdLine, &NeedExtraRun);
> > +    SHELL_FREE_NON_NULL(*CmdLine);
> > +    if (NewCmdLine == NULL) {
> > +      return (EFI_OUT_OF_RESOURCES);
> > +    }
> > +    *CmdLine = NewCmdLine;
> >    }
> > -  *CmdLine = NewCmdLine;
> > +
> >    return (EFI_SUCCESS);
> >  }
> >
> > --
> > 2.17.1
> >
> >
> > 

Re: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line

[Vladimir Olovyannikov]

Hi Gao,

> Hi Vladimir,
>
> > -----Original Message-----
> > From: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > Sent: Saturday, January 11, 2020 12:33 AM
> > To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io; Gao, Zhichao
> > <zhichao.gao@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support
> > nested variables substitution from cmd line
> >
> > Hi Ray,
> >
> > As an example, let's say that you have multiple variants of serverips
> > - each interface has its own, and each interface has a property as say
> > interface no (eth_no).
> > It is logical to address a serverip for an interface as
> > %serverip%eth_no%%, and in the end get the contents of variable
> > serverip0 (say for eth_no containing value 0).
> > In fact, Linux bash allows doing that easily.
> > We are doing most of our upgrade/configuration stuff via Shell scripts
> > which is very flexible, easy and efficient.
> > To support boot redundancy, we support multiple slots for kernel/dtbs,
> > etc, which are addressed as %update_type_%upd_type%% where
> upd_type
> > can be dtb, kernel, etc.
> > The spec says " Environment variables can be used on the command-line
> > by using %variable-name% where variable-name is the environment
> > variable's name. Variable substitution is not recursive."
> > I treat it as an extension to %var% case; %var case is not touched.
> > What do you think?
>
> "Variable substitution is not recursive" means the enviornment variable
> doesn't support nested. If you want to push the change, you should change
> the spec first. And there is one variable substitution flow figure, it
> should be
> update as well.
OK, thank you, I understand.
There is actually an ugly workaround. Vars substitution depends on the var
order. Thus, there is a way to
redeclare a variable (after deletion), so it changes the order in a way that
in %serverip%eth_no%% eth_no comes first,
and %serverip...% comes next. Then this nesting sort of working.
Another ugly (but possible) way would be do something like this:
set serverip%eth_no% >v tmp
This gives " serverip0 = nnn.nnn.nnn.nnn", and then have a string
find/supporting app which would give anything after "= ".
Still not clear why this limitation in the specs.... It is easy to do with
Bash script, and for a Shell script it is just a small change...
May I suggest to review the spec regarding Shell variables substitution?

Thank you,
Vladimir
>
> Thanks,
> Zhichao
>
> >
> > Regarding unit tests.
> > I verified variables substitutions using %var% and %var%var1%% and
> > %%var1%var% to make sure there would be no regression.
> > Our scripts use variables extensively, and no issues were encountered.
> > Please let me know if you need the scripts we are using, and I will
> > send them to you.
> >
> > Which unit tests are available?
> >
> > Thank you,
> > Vladimir
> >
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Thursday, January 9, 2020 10:03 PM
> > To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com; Gao,
> > Zhichao <zhichao.gao@intel.com>
> > Subject: RE: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support
> > nested variables substitution from cmd line
> >
> > Vladimir,
> > Is this enhancement an extension to Shell spec or violation of Shell
> > spec?
> > If it's an extension, I am happy to have such feature implemented.
> > Otherwise, we may need to discuss in uefi.org.
> >
> > Can you list the unit tests you have done?
> >
> > Thanks,
> > Ray
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Vladimir Olovyannikov via Groups.Io
> > > Sent: Friday, January 10, 2020 6:07 AM
> > > To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Gao, Zhichao
> > > <zhichao.gao@intel.com>
> > > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > > Subject: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support
> > > nested variables substitution from cmd line
> > >
> > > The current implementation of ShellSubstituteVariables() does not
> > > allow substitution of variables names containing another variable
> > > name(s) between %...%
> > >
> > > Example: %text%var_name%% where var_name variable contains 0.
> > > Here the variable value which should be returned on substitution
> > > would be contents of text0 variable.
> > > The current implementation would return nothing as %text0% would be
> > > eliminated by StripUnreplacedEnvironmentVariables().
> > >
> > > Modify the code so that StripUnreplacedEnvironmentVariables checks
> > > if variable between %...% really exists. If it does not, delete %...%.
> > > If it exists, preserve it and tell the caller that another run of
> > > ShellConvertVariables() is needed. This way, any nested variable
> > > between %...% can be easily retrieved.
> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2452
> > >
> > > Signed-off-by: Vladimir Olovyannikov
> > > <vladimir.olovyannikov@broadcom.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > ---
> > >  ShellPkg/Application/Shell/Shell.c | 71
> > > ++++++++++++++++++++++++------
> > >  1 file changed, 57 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/ShellPkg/Application/Shell/Shell.c
> > > b/ShellPkg/Application/Shell/Shell.c
> > > index d16adae0ea30..3756a71794d1 100644
> > > --- a/ShellPkg/Application/Shell/Shell.c
> > > +++ b/ShellPkg/Application/Shell/Shell.c
> > > @@ -1510,12 +1510,16 @@ ShellConvertAlias(
> > >
> > >  /**
> > >    This function will eliminate unreplaced (and therefore non-found)
> > environment variables.
> > > -
> > > +  If a variable exists between %...%, it will be preserved, and
> > > + VarExists  would be TRUE.
> > >    @param[in,out] CmdLine   The command line to update.
> > > +  @param[out] VarExists    A pointer to the BOOLEAN which is set if
> > > +                           a Shell variable between %...% exists.
> > >  **/
> > >  EFI_STATUS
> > >  StripUnreplacedEnvironmentVariables(
> > > -  IN OUT CHAR16 *CmdLine
> > > +  IN OUT CHAR16 *CmdLine,
> > > +  OUT BOOLEAN   *VarExists
> > >    )
> > >  {
> > >    CHAR16 *FirstPercent;
> > > @@ -1558,12 +1562,38 @@ StripUnreplacedEnvironmentVariables(
> > >      if (FirstQuote == NULL || SecondPercent < FirstQuote) {
> > >        if (IsValidEnvironmentVariableName(FirstPercent,
> > > SecondPercent))
> > {
> > >          //
> > > -        // We need to remove from FirstPercent to SecondPercent
> > > -        //
> > > -        CopyMem(FirstPercent, SecondPercent + 1,
> > > StrSize(SecondPercent
> > + 1));
> > > -        //
> > > -        // don't need to update the locator.  both % characters are
> > gone.
> > > +        // If this Shell variable exists, consider that another run
> > > + is
> > needed.
> > > +        // For example, consider a variable %var%var2%% when var2 is
> > > 0.
> > > +        // After the first run, it becomes %var0%, and after the
> > > + second
> > run
> > > +        // it contains the value of var0 variable.
> > > +        // Eliminate variable if it does not exist.
> > >          //
> > > +        CHAR16 *VarName;
> > > +
> > > +        // Consider NULL terminator as well
> > > +        VarName = (CHAR16 *)AllocateZeroPool((SecondPercent -
> > FirstPercent) *
> > > +                                             sizeof(CHAR16));
> > > +        if (VarName) {
> > > +          CopyMem(VarName, FirstPercent + 1,
> > > +                  (SecondPercent - FirstPercent - 1) *
> > > sizeof(CHAR16));
> > > +        }
> > > +
> > > +        if (!VarName || !ShellGetEnvironmentVariable(VarName)) {
> > > +          //
> > > +          // We need to remove from FirstPercent to SecondPercent.
> > > +          //
> > > +          CopyMem(FirstPercent, SecondPercent + 1,
> > StrSize(SecondPercent + 1));
> > > +          //
> > > +          // don't need to update the locator.  both % characters
> > > + are
> > gone.
> > > +          //
> > > +        } else {
> > > +          *VarExists = TRUE;
> > > +          //
> > > +          // In this case, locator needs to be updated as %
> > > + characters
> > present.
> > > +          //
> > > +          CurrentLocator = SecondPercent + 1;
> > > +        }
> > > +        SHELL_FREE_NON_NULL(VarName);
> > >        } else {
> > >          CurrentLocator = SecondPercent + 1;
> > >        }
> > > @@ -1581,13 +1611,18 @@ StripUnreplacedEnvironmentVariables(
> > >    If the return value is not NULL the memory must be caller freed.
> > >
> > >    @param[in] OriginalCommandLine    The original command line
> > > +  @param[out] NeedExtraRun          Indication that the caller needs
> > > to
> > repeat
> > > +                                    this call to convert variables
> > > + as
> > there are
> > > +                                    existing variable(s) between %..%
> > > +                                    present after the call.
> > >
> > >    @retval NULL                      An error occurred.
> > >    @return                           The new command line with no
> > environment variables present.
> > >  **/
> > >  CHAR16*
> > >  ShellConvertVariables (
> > > -  IN CONST CHAR16 *OriginalCommandLine
> > > +  IN CONST CHAR16 *OriginalCommandLine,
> > > +  OUT BOOLEAN     *NeedExtraRun
> > >    )
> > >  {
> > >    CONST CHAR16        *MasterEnvList;
> > > @@ -1601,11 +1636,13 @@ ShellConvertVariables (
> > >    ALIAS_LIST          *AliasListNode;
> > >
> > >    ASSERT(OriginalCommandLine != NULL);
> > > +  ASSERT(NeedExtraRun != NULL);
> > >
> > >    ItemSize          = 0;
> > >    NewSize           = StrSize(OriginalCommandLine);
> > >    CurrentScriptFile = ShellCommandGetCurrentScriptFile();
> > >    Temp              = NULL;
> > > +  *NeedExtraRun     = FALSE;
> > >
> > >    ///@todo update this to handle the %0 - %9 for scripting only
> > > (borrow
> > from line 1256 area) ? ? ?
> > >
> > > @@ -1698,7 +1735,7 @@ ShellConvertVariables (
> > >    //
> > >    // Remove non-existent environment variables
> > >    //
> > > -  StripUnreplacedEnvironmentVariables(NewCommandLine1);
> > > +  StripUnreplacedEnvironmentVariables(NewCommandLine1,
> > > + NeedExtraRun);
> > >
> > >    //
> > >    // Now cleanup any straggler intentionally ignored "%" characters
> > > @@ -1845,12 +1882,18 @@ ShellSubstituteVariables(
> > >    )
> > >  {
> > >    CHAR16      *NewCmdLine;
> > > -  NewCmdLine = ShellConvertVariables(*CmdLine);
> > > -  SHELL_FREE_NON_NULL(*CmdLine);
> > > -  if (NewCmdLine == NULL) {
> > > -    return (EFI_OUT_OF_RESOURCES);
> > > +  BOOLEAN     NeedExtraRun;
> > > +
> > > +  NeedExtraRun = TRUE;
> > > +  while (NeedExtraRun) {
> > > +    NewCmdLine = ShellConvertVariables(*CmdLine, &NeedExtraRun);
> > > +    SHELL_FREE_NON_NULL(*CmdLine);
> > > +    if (NewCmdLine == NULL) {
> > > +      return (EFI_OUT_OF_RESOURCES);
> > > +    }
> > > +    *CmdLine = NewCmdLine;
> > >    }
> > > -  *CmdLine = NewCmdLine;
> > > +
> > >    return (EFI_SUCCESS);
> > >  }
> > >
> > > --
> > > 2.17.1
> > >
> > >
> > >