Hi Gao, > Hi Vladimir, > > > -----Original Message----- > > From: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> > > Sent: Saturday, January 11, 2020 12:33 AM > > To: Ni, Ray <ray...@intel.com>; devel@edk2.groups.io; Gao, Zhichao > > <zhichao....@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...@intel.com> > > Sent: Thursday, January 9, 2020 10:03 PM > > To: devel@edk2.groups.io; vladimir.olovyanni...@broadcom.com; Gao, > > Zhichao <zhichao....@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...@intel.com>; Gao, Zhichao > > > <zhichao....@intel.com> > > > Cc: Vladimir Olovyannikov <vladimir.olovyanni...@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.olovyanni...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
