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

["Gao, Zhichao" <zhichao.gao@intel.com>]

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