[igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag
Mauro Carvalho Chehab
mauro.chehab at linux.intel.com
Tue Jul 5 19:58:00 UTC 2022
On Thu, 30 Jun 2022 13:40:47 +0000
"Knop, Ryszard" <ryszard.knop at intel.com> wrote:
> On Wed, 2022-06-29 at 18:13 +0200, Mauro Carvalho Chehab wrote:
> > On Tue, 28 Jun 2022 11:44:32 +0200
> > Ryszard Knop <ryszard.knop at intel.com> wrote:
> >
> > > Allows specifying additional environment variables to be set for
> > > the
> > > launched test processes. This commit introduces the argument
> > > parsing.
> > >
> > > Signed-off-by: Ryszard Knop <ryszard.knop at intel.com>
> > > ---
> > > runner/settings.c | 116
> > > +++++++++++++++++++++++++++++++++++++++++++++-
> > > runner/settings.h | 9 ++++
> > > 2 files changed, 124 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/runner/settings.c b/runner/settings.c
> > > index d153954a..de339484 100644
> > > --- a/runner/settings.c
> > > +++ b/runner/settings.c
> > > @@ -35,6 +35,7 @@ enum {
> > > OPT_DRY_RUN = 'd',
> > > OPT_INCLUDE = 't',
> > > OPT_EXCLUDE = 'x',
> > > + OPT_ENVIRONMENT = 'e',
> > > OPT_SYNC = 's',
> > > OPT_LOG_LEVEL = 'l',
> > > OPT_OVERWRITE = 'o',
> > > @@ -389,6 +390,113 @@ static void free_regexes(struct regex_list
> > > *regexes)
> > > free(regexes->regexes);
> > > }
> > >
> > > +/**
> > > + * string_trim_and_duplicate() - returns a duplicated, trimmed
> > > string
> > > + * @string: string to trim and duplicate
> > > + *
> > > + * If the provided string is NULL, a NULL is returned. In any
> > > other case, a
> > > + * newly-allocated string of length up to strlen(string) is
> > > returned. The
> > > + * returned string has its whitespace removed (as detected by
> > > isspace), while
> > > + * the original string is left unmodified.
> > > + */
> > > +static char *string_trim_and_duplicate(const char *string) {
> > > + size_t length;
> > > +
> > > + if (string == NULL)
> > > + return NULL;
> > > +
> > > + length = strlen(string);
> > > +
> > > + while (length > 0 && isspace(string[0])) {
> > > + string++;
> > > + length--;
> > > + }
> > > +
> > > + while (length > 0 && isspace(string[length - 1])) {
> > > + length--;
> > > + }
> > > +
> > > + return strndup(string, length);
> > > +}
> > > +
> > > +/**
> > > + * add_env_var() - Adds a new environment variable to the runner
> > > settings.
> > > + * @env_vars: Pointer to the env var list head from the settings.
> > > + * @env_kv: Environment variable key-value pair string to add.
> > > + *
> > > + * env_kv must be a string like "KEY=VALUE" or just "KEY" if the
> > > value is to
> > > + * be loaded from the runner's environment variables. In the
> > > latter case, if
> > > + * the requested variable is not set, the operation will fail.
> > > + *
> > > + * An empty variable may be set by providing an env_kv of "KEY="
> > > or setting
> > > + * an empty variable for the runner process, then providing just
> > > "KEY".
> > > + */
> > > +static bool add_env_var(struct igt_list_head *env_vars, char
> > > *env_kv) {
> > > + char *current_env_value, *kv_split;
> > > + struct environment_variable *var = NULL;
> > > +
> > > + if (env_vars == NULL || env_kv == NULL || strlen(env_kv) ==
> > > 0)
> > > + goto error;
> >
> > As you had already allocated env_vars at this point, as otherwise
> > env_vars
> > would be NULL, the best is to just call IGT_INIT_LIST_HEAD(env_vars)
> > when
> > it was allocated.
> >
> > > +
> > > + env_kv = strdup(env_kv);
> > > + kv_split = strchr(env_kv, '=');
> >
> > Better to use strtok().
> >
> > > + if (kv_split == env_kv) {
> > > + fprintf(stderr, "Missing key for --environment
> > > \"%s\"\n", env_kv);
> > > + goto error;
> > > + }
> > > +
> > > + var = malloc(sizeof(struct environment_variable));
> > > + if (kv_split != NULL) {
> > > + /* value provided - copy string contents after '='
> > > */
> > > + kv_split[0] = NULL;
> > > +
> > > + var->key = string_trim_and_duplicate(env_kv);
> > > + var->value = strdup(kv_split + 1); /* Can be empty,
> > > that's okay */
> > > + } else {
> > > + /* use the runner's environment, if set */
> > > + current_env_value = getenv(env_kv);
> > > + if (current_env_value == NULL) {
> > > + fprintf(stderr, "No value provided for --
> > > environment \"%s\" and "
> > > + "variable is not set for
> > > igt_runner\n", env_kv);
> > > + goto error;
> > > + }
> > > +
> > > + var->key = string_trim_and_duplicate(env_kv);
> > > + var->value = strdup(current_env_value);
> > > + }
> >
> >
> > It sounds weird to accept things like:
> >
> > "ENV= var"
> > or even:
> > " ENV=var"
> >
> > So, I would code the above with something based on this:
> >
> > key = strtok(env_kv, ", \n\t=");
> > if (!key || !*key)
> > goto err;
> > value = strtok(NULL, ", \n\t");
> >
> > Simpler, easier to understand and will give errors if spaces are
> > found where they shouldn't be. It can even be used inside a loop,
> > if someone would want to use it as:
> >
> > --environment "VAR1=foo, VAR2=bar"
>
> Using strtok on the value does not handle cases where, for example, you
> can have spaces or equals sign in the value itself:
>
> -e "DATA_PATH=/mnt/Pendrive 2GB/build/..."
> -e "SUBPROCESS_ARGS=--arg1=123 --another-arg=456"
You could then use strpbrk(), which should still simplify the parser.
p = strpbrk(key, " \n\t=");
if (*p == '=')
value = p++;
else
value = NULL;
*p = '\0';
Although, in this case, one -e per parameter would be needed.
>
> An alternative solution would be to enable shell-like parsing with
> quotes, escape chars, etc, but that sounds like a lot more work.
Agreed.
> As for the key, I thought about a case where you might have a script
> that just applies env vars from a file or script which can be aligned:
>
> BINARIES=/opt/igt/...
> DATA=/mnt/stuff/...
>
> Most software would convert " DATA" into "DATA" automatically.
> Whenever it's good practice or not is another discussion, but I'd like
> this arg to do the sane thing by default. But it looks like strtok will
> simplify key lookups, so I'll try to do that here.
Ok.
Regards,
Mauro
More information about the igt-dev
mailing list