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

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

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