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

["Vladimir Olovyannikov" <vladimir.olovyannikov@broadcom.com>]

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
>
>
>