From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oi1-f169.google.com (mail-oi1-f169.google.com [209.85.167.169]) by mx.groups.io with SMTP id smtpd.web11.444.1579023221966549982 for ; Tue, 14 Jan 2020 09:33:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@broadcom.com header.s=google header.b=EnWHU6RP; spf=pass (domain: broadcom.com, ip: 209.85.167.169, mailfrom: vladimir.olovyannikov@broadcom.com) Received: by mail-oi1-f169.google.com with SMTP id v140so12625209oie.0 for ; Tue, 14 Jan 2020 09:33: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=3PDRLZfWwuVCw0N1H+50ThiIClbP1UKC/d4mQcCGhrw=; b=EnWHU6RPWcdoVs3011Mu9qoaFxvT/DU5MVBFBBB904aWYFbOnAOVJ6Kc23eGzhPhwb phYEV0fAbGsN/C1bWirSzdsDPfxq3pNLz3O3EnMKxzINu1rcb+/tAtPbzeVrDWyasf3b g4+6gRr85zMQ/aIaTiFMc8AqeaCiIaalwtD48= 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=3PDRLZfWwuVCw0N1H+50ThiIClbP1UKC/d4mQcCGhrw=; b=NCEtvJi1jVGYSsvc0INxU6B4/2ECOz7MaGf/PxZFqCQgP9IG6yJM0oHq2LWEPIjFEl TjQdcokOPMx17G3T0nDkqL9CjSYbB5VHpJ5pucw+RZU169MwxhWUykUCqu4hSezbMfI7 edpm+1hO3grQJ3BsOfigwrcyfsV3liYLg5n2Qz8fLaa3bvQjQTTZD9WF8SejOYI2Bsvm B7TYZgxULCPB4CTfAaIkJ1ZqZ9S/Voqemugd7xKRCmJE9WWQx/nwcoc5YAmzp/SaYCo3 x90Aqg7julIs0rsnLnrKg5mwvR4v0vEIKatF3hC8vew3kW7lteGQlVuBatkm7c1zyGbM PXGQ== X-Gm-Message-State: APjAAAWv7rqVmtxXcldypySZDUJWZ1b8N5ijbTzT92hpPmM4IoDR968h Qxg59PbrmDX/xUpmT88z0lHriufTTts+nECmhcxHSQ== X-Google-Smtp-Source: APXvYqwOw8ae2Qh6r8aHqYKIf30abSv/zjxG/rnjWAfdZ8P1wvgktwmEGdz60b/tcYkau+H24TLHDhfRRrB9Tj7x0/4= X-Received: by 2002:a05:6808:643:: with SMTP id z3mr17872240oih.19.1579023221090; Tue, 14 Jan 2020 09:33:41 -0800 (PST) From: "Vladimir Olovyannikov" References: <20200109220637.28691-1-vladimir.olovyannikov@broadcom.com> <734D49CCEBEEF84792F5B80ED585239D5C3E6CF4@SHSMSX104.ccr.corp.intel.com> <2d6b2d376dfef281c2ec66b35d0735f8@mail.gmail.com> In-Reply-To: MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQHY2wJ5U8LEjQSCQHyjrgeEx4qUwAIZreV9AquEUdgCJ3SK2aes4eQw Date: Tue, 14 Jan 2020 09:33:38 -0800 Message-ID: <3687630d6993fe580fde044d2a29b9c8@mail.gmail.com> Subject: Re: [edk2-devel] [PATCH 1/1] ShellPkg/Application: Support nested variables substitution from cmd line To: "Gao, Zhichao" , "Ni, Ray" , devel@edk2.groups.io Content-Type: text/plain; charset="UTF-8" Hi Gao, > Hi Vladimir, > > > -----Original Message----- > > From: Vladimir Olovyannikov > > Sent: Saturday, January 11, 2020 12:33 AM > > To: Ni, Ray ; devel@edk2.groups.io; Gao, Zhichao > > > > 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 > > 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 > > > > > > > > >