[edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line
Vladimir Olovyannikov via Groups.Io
vladimir.olovyannikov=broadcom.com at groups.io
Tue Jan 14 17:33:38 UTC 2020
Hi Gao,
> Hi Vladimir,
>
> > -----Original Message-----
> > From: Vladimir Olovyannikov <vladimir.olovyannikov at broadcom.com>
> > Sent: Saturday, January 11, 2020 12:33 AM
> > To: Ni, Ray <ray.ni at intel.com>; devel at edk2.groups.io; Gao, Zhichao
> > <zhichao.gao at 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 at intel.com>
> > Sent: Thursday, January 9, 2020 10:03 PM
> > To: devel at edk2.groups.io; vladimir.olovyannikov at broadcom.com; Gao,
> > Zhichao <zhichao.gao at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
> > > Vladimir Olovyannikov via Groups.Io
> > > Sent: Friday, January 10, 2020 6:07 AM
> > > To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Gao, Zhichao
> > > <zhichao.gao at intel.com>
> > > Cc: Vladimir Olovyannikov <vladimir.olovyannikov at 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 at 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
> > >
> > >
> > >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53237): https://edk2.groups.io/g/devel/message/53237
Mute This Topic: https://groups.io/mt/69589246/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list