[igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags

[Mauro Carvalho Chehab]

On Thu, 30 Jun 2022 13:57:16 +0000
"Knop, Ryszard" <ryszard.knop at intel.com> wrote:


> > >  
> > > +static bool should_skip_env_line(char *line) {
> > > +       char *trimmed_line;
> > > +       bool is_comment, missing_split;
> > > +
> > > +       if (line == NULL)
> > > +               return true;
> > > +
> > > +       trimmed_line = string_trim_and_duplicate(line);  
> > 
> > Why do you need to trim whitespaces here?  
> 
> We check if a line is a comment if it's empty, contains only whitespace
> or starts with a '#' below. If it contains just whitespace, trimming it
> gets you an empty string. If there's whitespace before '#', I'd like to
> treat it as a comment too.
> 
> This can be useful if you're manually editing the environment file for
> quick iteration.

If the idea is to get env vars from the file, I would implement it on
a different way:

	--environment <filename>

or

	-- env-file <filename>

The way your current code implements, the file seems to be used only
at recover time, so it doesn´t make much sense, IMHO, to allow it to
be edited.

Yet, I don't see much sense on having a function to skip. I mean, the file
parser could just be something similar to:

	while(fgets(line, sizeof(line), fp)) {
		p = line;
		while (*p == ' '||*p == '\t')
			p++;
		if (*p == "#")
			continue;

		key = p;
		p = strpbrk(key, "\n\t =");
		if (*value != '=')
			value = NULL;
		else
			value = p + 1;
		*p = '\0';

		add_key_value(key, value);
	}

(or something similar, using strtok)

> 
> > > +       if (trimmed_line == NULL)
> > > +               return true;
> > > +
> > > +       is_comment = strlen(trimmed_line) == 0 || trimmed_line[0]
> > > == '#';
> > > +       missing_split = strchr(trimmed_line, '=') == NULL;
> > > +
> > > +       free(trimmed_line);
> > > +       return is_comment || missing_split;
> > > +}
> > > +
> > > +/**
> > > + * read_env_vars_from_file() - load env vars from a file
> > > + * @env_vars: list head to add env var entries to
> > > + * @dirfd: file descriptor of the directory containing env vars
> > > + * 
> > > + * Loads the environment.txt file and adds each line-separated
> > > KEY=VALUE pair
> > > + * into the provided env_vars list. Lines not containing the '='
> > > K-V separator
> > > + * or starting with a '#' are ignored.
> > > + * 
> > > + * The function returns true if the env vars were read
> > > successfully, or there
> > > + * was nothing to be read.
> > > + */
> > > +static bool read_env_vars_from_file(struct igt_list_head
> > > *env_vars, FILE *f)
> > > +{
> > > +       char *line = NULL;
> > > +       ssize_t line_length;
> > > +       size_t line_buffer_length = 0;
> > > +
> > > +       while ((line_length = getline(&line, &line_buffer_length,
> > > f)) != -1) {
> > > +               if (should_skip_env_line(line))
> > > +                       continue;
> > > +
> > > +               /* Strip the line feed, but keep trailing
> > > whitespace */
> > > +               if (line_length > 0 && line[line_length - 2] ==
> > > '\n')
> > > +                       line[line_length - 2] = '\0';
> > > +
> > > +               add_env_var(env_vars, line);
> > > +       }
> > > +
> > > +       /* input file can be empty */
> > > +       if (line != NULL)
> > > +               free(line);
> > > +
> > > +       return true;
> > > +}
> > > +
> > >  bool read_settings_from_dir(struct settings *settings, int dirfd)
> > >  {
> > > -       int fd;
> > >         FILE *f;
> > >  
> > >         clear_settings(settings);
> > >  
> > > -       if ((fd = openat(dirfd, settings_filename, O_RDONLY)) < 0)
> > > -               return false;
> > > +       /* settings are always there */
> > > +       {  
> > 
> > Why are you adding a '{' here?  
> 
> The block below (which checks for env_filename existence) does almost
> exactly the same thing, with different args and read_env_vars instead
> of read_settings. They neatly align visually this way. I can revert
> this if you'd like.

I don't see the need of using it, and this makes it harder to review, IMO.

Keep it simple.

Regards,
Mauro

> 
> Thanks, Ryszard
> 
> > > +               if ((f = fopenat_read(dirfd, settings_filename)) ==
> > > NULL)
> > > +                       return false;
> > >  
> > > -       f = fdopen(fd, "r");
> > > -       if (!f) {
> > > -               close(fd);
> > > -               return false;
> > > -       }
> > > +               if (!read_settings_from_file(settings, f)) {
> > > +                       fclose(f);
> > > +                       return false;
> > > +               }
> > >  
> > > -       if (!read_settings_from_file(settings, f)) {
> > >                 fclose(f);
> > > -               return false;
> > >         }
> > >  
> > > -       fclose(f);
> > > +       /* env file may not exist if no --environment was set */
> > > +       if (file_exists_at(dirfd, env_filename)) {
> > > +               if ((f = fopenat_read(dirfd, env_filename)) ==
> > > NULL)
> > > +                       return false;
> > > +
> > > +               if (!read_env_vars_from_file(&settings->env_vars,
> > > f)) {
> > > +                       fclose(f);
> > > +                       return false;
> > > +               }
> > > +
> > > +               fclose(f);
> > > +       }
> > >  
> > >         return true;
> > >  }  
>