-
Notifications
You must be signed in to change notification settings - Fork 4
Description
The eval_cmd
feature is questionable to begin with as it encourages mixing config and behavioural logic. Having good separation between logic and config is generally considered to result in a more robust and maintainable long term solution. At some point it might be good to consider removing that feature and replacing it with a plug-in system that gives the same functionality without the issues associated with blindly executing arbitrary strings in configuration when loading the config file.
There are not many uses of this in OT, so it's feasible to remove, with some effort.
Implementation issues
In the short term, the main issue with the implementation of this feature is that eval_cmd
only has a start delimiter. Which means that if the wildcards are substituted in a different order then arbitrary text will be executed. This is a slightly contrived example, as DVSim has direct support for using environment variables as wildcards. However conceptually this demonstration shows real a real bug that I've seen.
{
"a": "{eval_cmd} env | grep PWD",
"b": "{a}/sub/dir"
}
If a
is resolved before b
, then b
might contain what you expect:
~/opentitan/sub/dir
However if for some reason b
is resolved before a
, then we get:
{
"a": "{eval_cmd} env | grep PWD",
"b": "{eval_cmd} env | grep PWD/sub/dir"
}
Which in a second pass becomes:
{
"a": "~/opentitan",
"b": ""
}
There are two issues here:
- The operations are not commutative - the order matters
- The eval command should only be run once and the result then propagated. Right now if the wildcard is substituted before it's evaluated, then the shell commands are run multiple times for no reason.
Proposal
Short term, fix the wildcard substitution so that it is commutative. Potential solutions are:
- Do not use the value of a field anywhere else until it's been evaluated? This may cause issues in terms of dependency loops, and complicate the algorithm?
- Add an end delimiter before substituting
The delimiter idea could be implemented by moving the arguments within the enclosing brackets. For example use the format {eval_cmd arguments ...}
as a replacement for {eval_cmd} arguments ...
. As part of the migration step the old form could be automatically converted. Which means that if the string contains {eval_cmd}
then attempt to evaluate it, if that's not possible right now as it contains unresolved wildcards, then convert it to the new format.
Long term, ideally we find a way of implementing the required functionality without the {eval_cmd}
feature.