From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f53.google.com (mail-ot1-f53.google.com [209.85.210.53]) by mx.groups.io with SMTP id smtpd.web10.12588.1578673961350623691 for ; Fri, 10 Jan 2020 08:32:41 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@broadcom.com header.s=google header.b=Tv4cc8cN; spf=pass (domain: broadcom.com, ip: 209.85.210.53, mailfrom: vladimir.olovyannikov@broadcom.com) Received: by mail-ot1-f53.google.com with SMTP id 59so2482028otp.12 for ; Fri, 10 Jan 2020 08:32:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=from:references:in-reply-to:mime-version:thread-index:date :message-id:subject:to; bh=g0PerXbaUdyeCVqpX/dcx+wl9qr+Gw1cv8FpiKJABx8=; b=Tv4cc8cNZwyIZ6WCIVmPPRQkuZ1gJwixDi8fB7NP79IfV6mI+zC7wdpM/nNzjLFBI+ m/UobI77eWMmxIfHmS+gzoN8qeZVepQGBQuXWoKm4dISGlZdioQYVvfyzjjShFIWFsJT RaDeZzyYaY1iQiA9v6tQTnxG2EHn3UWnsUiP4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:references:in-reply-to:mime-version :thread-index:date:message-id:subject:to; bh=g0PerXbaUdyeCVqpX/dcx+wl9qr+Gw1cv8FpiKJABx8=; b=Lg9Q6XzWPBjHNighlgU2TtCsBQprIiPtOKHfoTUWh926hcf1GQT6avYFNkwD6OUb2K ulpEF48zYUjNqNticlvVVzAmXTiHQNI4ngsq0QYKhaT7Jm96LGCjMPtilcREIRjthFes MizoxB9ZeoqwAsr5YLvD259XSGUBiraFCWeh3sjeIw1P//8D0K8gtJKpaG0/RPAKS6dZ FZyPeSMtjdBXdChNkxppbhTQlX0mk/hpgrkgRtrbxtIemaX9kfdQv50LEEit+f9wu1K/ UhFavQSH/WSiMovqxCwEapBwNLg9XjTbAVAZotTU3VD43Gmx5IiS/972rkwMBY3jucgU hrGQ== X-Gm-Message-State: APjAAAU154la1xxvy3Q/fI5UNqa/6GSvN5quiB/VEVufPr24ZW1JQPGu fLSxxD0JRTYnLH5WijQKr4hnNblj4hvc/cnCov4mZTQCUjH4zQ== X-Google-Smtp-Source: APXvYqyo06Ub1dA9AYgXp+Pos1OFIh3Q91O+Av3D3dcnNC/p8dt6f0uvls7Sluy//SxlmvW2SWbluy8C9vvnYpXE3TE= X-Received: by 2002:a9d:7c90:: with SMTP id q16mr3340389otn.191.1578673960349; Fri, 10 Jan 2020 08:32:40 -0800 (PST) From: "Vladimir Olovyannikov" References: <20200109220637.28691-1-vladimir.olovyannikov@broadcom.com> <734D49CCEBEEF84792F5B80ED585239D5C3E6CF4@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C3E6CF4@SHSMSX104.ccr.corp.intel.com> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHY2wJ5U8LEjQSCQHyjrgeEx4qUwAIZreV9p80bpZA= Date: Fri, 10 Jan 2020 08:32:37 -0800 Message-ID: <2d6b2d376dfef281c2ec66b35d0735f8@mail.gmail.com> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line To: "Ni, Ray" , devel@edk2.groups.io, "Gao, Zhichao" Content-Type: text/plain; charset="UTF-8" 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 Sent: Thursday, January 9, 2020 10:03 PM To: devel@edk2.groups.io; vladimir.olovyannikov@broadcom.com; Gao, Zhichao 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 On Behalf Of > Vladimir Olovyannikov via Groups.Io > Sent: Friday, January 10, 2020 6:07 AM > To: devel@edk2.groups.io; Ni, Ray ; Gao, Zhichao > > Cc: Vladimir Olovyannikov > 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 > > 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 > > >