All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag
@ 2022-06-28  9:44 Ryszard Knop
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 1/7] runner: rename free_settings to clear_settings Ryszard Knop
                   ` (9 more replies)
  0 siblings, 10 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

This series adds a new flag/argument to the igt_runner, --environment.

It sets the provided KEY=VALUE pair as the environment variable for
executed test processes. If just the KEY is provided, the currently-set
value is reused (and the run fails if it's not set for the runner).

The most important thing about this feature is that all the variables
set this way are saved for resumed runs as well. This lets developers
easily try something out from the terminal without having to remember
to set the same vars/reuse a wrapper to continue a run.

Ryszard Knop (7):
  runner: rename free_settings to clear_settings
  runner: make the usage function variadic
  igt_list: Add the igt_list_empty_or_null helper
  runner: Add the --environment flag
  runner: Serialize the provided environment flags
  runner: Set requested env vars during execution
  runner: Add help for the --environment flag

 lib/igt_list.c        |   5 +
 lib/igt_list.h        |   1 +
 runner/executor.c     |  10 +-
 runner/resultgen.c    |   2 +-
 runner/runner_tests.c |  12 +-
 runner/settings.c     | 385 ++++++++++++++++++++++++++++++++++--------
 runner/settings.h     |  15 +-
 7 files changed, 350 insertions(+), 80 deletions(-)

-- 
2.36.1

^ permalink raw reply	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t 1/7] runner: rename free_settings to clear_settings
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
@ 2022-06-28  9:44 ` Ryszard Knop
  2022-06-29 15:48   ` Mauro Carvalho Chehab
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic Ryszard Knop
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

That function does not actually free the settings pointer - rename it
for clarity.

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/executor.c     |  2 +-
 runner/resultgen.c    |  2 +-
 runner/runner_tests.c | 12 ++++++------
 runner/settings.c     |  8 ++++----
 runner/settings.h     |  6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/runner/executor.c b/runner/executor.c
index 6e6ca9cc..9b89cc09 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1547,7 +1547,7 @@ bool initialize_execute_state_from_resume(int dirfd,
 	struct job_list_entry *entry;
 	int resdirfd, fd, i;
 
-	free_settings(settings);
+	clear_settings(settings);
 	free_job_list(list);
 	memset(state, 0, sizeof(*state));
 	state->resuming = true;
diff --git a/runner/resultgen.c b/runner/resultgen.c
index 6ecf325b..8e11b2b1 100644
--- a/runner/resultgen.c
+++ b/runner/resultgen.c
@@ -1702,7 +1702,7 @@ struct json_object *generate_results_json(int dirfd)
 		close(fd);
 	}
 
-	free_settings(&settings);
+	clear_settings(&settings);
 	free_job_list(&job_list);
 
 	return obj;
diff --git a/runner/runner_tests.c b/runner/runner_tests.c
index 8fe86978..aca031fc 100644
--- a/runner/runner_tests.c
+++ b/runner/runner_tests.c
@@ -139,7 +139,7 @@ static void job_list_filter_test(const char *name, const char *filterarg1, const
 	}
 
 	igt_fixture {
-		free_settings(settings);
+		clear_settings(settings);
 		free(settings);
 	}
 }
@@ -953,7 +953,7 @@ igt_main
 			close(fd);
 			close(dirfd);
 			clear_directory(dirname);
-			free_settings(cmp_settings);
+			clear_settings(cmp_settings);
 			free(cmp_settings);
 		}
 	}
@@ -1176,7 +1176,7 @@ igt_main
 			igt_assert(write(fd, journaltext, strlen(journaltext)) == strlen(journaltext));
 
 			free_job_list(list);
-			free_settings(settings);
+			clear_settings(settings);
 			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
 
 			igt_assert_eq(state.next, 0);
@@ -1233,7 +1233,7 @@ igt_main
 			igt_assert(write(fd, journaltext, strlen(journaltext)) == strlen(journaltext));
 
 			free_job_list(list);
-			free_settings(settings);
+			clear_settings(settings);
 			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
 
 			/* All subtests are in journal, the entry should be considered completed */
@@ -1294,7 +1294,7 @@ igt_main
 			igt_assert_eq(write(fd, journaltext, sizeof(journaltext)), sizeof(journaltext));
 
 			free_job_list(list);
-			free_settings(settings);
+			clear_settings(settings);
 			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
 
 			igt_assert_eq(state.next, 1);
@@ -1942,7 +1942,7 @@ igt_main
 	}
 
 	igt_fixture {
-		free_settings(settings);
+		clear_settings(settings);
 		free(settings);
 	}
 }
diff --git a/runner/settings.c b/runner/settings.c
index bb63a9f4..06947c91 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -486,7 +486,7 @@ void init_settings(struct settings *settings)
 	memset(settings, 0, sizeof(*settings));
 }
 
-void free_settings(struct settings *settings)
+void clear_settings(struct settings *settings)
 {
 	free(settings->test_list);
 	free(settings->name);
@@ -536,7 +536,7 @@ bool parse_options(int argc, char **argv,
 		{ 0, 0, 0, 0},
 	};
 
-	free_settings(settings);
+	clear_settings(settings);
 
 	optind = 1;
 
@@ -705,7 +705,7 @@ bool parse_options(int argc, char **argv,
 	return true;
 
  error:
-	free_settings(settings);
+	clear_settings(settings);
 	return false;
 }
 
@@ -939,7 +939,7 @@ bool read_settings_from_dir(struct settings *settings, int dirfd)
 	int fd;
 	FILE *f;
 
-	free_settings(settings);
+	clear_settings(settings);
 
 	if ((fd = openat(dirfd, settings_filename, O_RDONLY)) < 0)
 		return false;
diff --git a/runner/settings.h b/runner/settings.h
index 6ae64695..6d444d01 100644
--- a/runner/settings.h
+++ b/runner/settings.h
@@ -76,14 +76,14 @@ struct settings {
 void init_settings(struct settings *settings);
 
 /**
- * free_settings:
+ * clear_settings:
  *
  * Releases all allocated resources for a settings object and
  * initializes it to an empty state (see #init_settings).
  *
- * @settings: Object to release and initialize.
+ * @settings: Object to release and reinitialize.
  */
-void free_settings(struct settings *settings);
+void clear_settings(struct settings *settings);
 
 /**
  * parse_options:
-- 
2.36.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 1/7] runner: rename free_settings to clear_settings Ryszard Knop
@ 2022-06-28  9:44 ` Ryszard Knop
  2022-06-28 23:51   ` [igt-dev] [i-g-t,2/7] " Maíra Canal
                     ` (2 more replies)
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 3/7] igt_list: Add the igt_list_empty_or_null helper Ryszard Knop
                   ` (7 subsequent siblings)
  9 siblings, 3 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

For easier error logging, make the usage() function variadic and pass
its arguments to vfprintf. Additionally, reorder its arguments so that
it visually matches all the printf functions.

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/settings.c | 63 ++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/runner/settings.c b/runner/settings.c
index 06947c91..d153954a 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -295,10 +295,16 @@ static const char *usage_str =
 	"                        this option if given.\n"
 	;
 
-static void usage(const char *extra_message, FILE *f)
+static void usage(FILE *f, const char *extra_message, ...)
 {
-	if (extra_message)
-		fprintf(f, "%s\n\n", extra_message);
+	if (extra_message) {
+		va_list args;
+
+		va_start(args, extra_message);
+		vfprintf(f, extra_message, args);
+		fputs("\n\n", f);
+		va_end(args);
+	}
 
 	fputs(usage_str, f);
 }
@@ -310,12 +316,7 @@ static bool add_regex(struct regex_list *list, char *new)
 
 	regex = g_regex_new(new, G_REGEX_OPTIMIZE, 0, &error);
 	if (error) {
-		char *buf = malloc(snprintf(NULL, 0, "Invalid regex '%s': %s", new, error->message) + 1);
-
-		sprintf(buf, "Invalid regex '%s': %s", new, error->message);
-		usage(buf, stderr);
-
-		free(buf);
+		usage(stderr, "Invalid regex '%s': %s", new, error->message);
 		g_error_free(error);
 		return false;
 	}
@@ -549,7 +550,7 @@ bool parse_options(int argc, char **argv,
 			print_version();
 			goto error;
 		case OPT_HELP:
-			usage(NULL, stdout);
+			usage(stdout, NULL);
 			goto error;
 		case OPT_NAME:
 			settings->name = strdup(optarg);
@@ -574,7 +575,7 @@ bool parse_options(int argc, char **argv,
 			break;
 		case OPT_DISK_USAGE_LIMIT:
 			if (!parse_usage_limit(settings, optarg)) {
-				usage("Cannot parse disk usage limit", stderr);
+				usage(stderr, "Cannot parse disk usage limit");
 				goto error;
 			}
 			break;
@@ -583,7 +584,7 @@ bool parse_options(int argc, char **argv,
 			break;
 		case OPT_LOG_LEVEL:
 			if (!set_log_level(settings, optarg)) {
-				usage("Cannot parse log level", stderr);
+				usage(stderr, "Cannot parse log level");
 				goto error;
 			}
 			break;
@@ -631,7 +632,7 @@ bool parse_options(int argc, char **argv,
 			break;
 		case OPT_PRUNE_MODE:
 			if (!set_prune_mode(settings, optarg)) {
-				usage("Cannot parse prune mode", stderr);
+				usage(stderr, "Cannot parse prune mode");
 				goto error;
 			}
 			break;
@@ -644,10 +645,10 @@ bool parse_options(int argc, char **argv,
 			settings->list_all = true;
 			break;
 		case '?':
-			usage(NULL, stderr);
+			usage(stderr, NULL);
 			goto error;
 		default:
-			usage("Cannot parse options", stderr);
+			usage(stderr, "Cannot parse options");
 			goto error;
 		}
 	}
@@ -664,7 +665,7 @@ bool parse_options(int argc, char **argv,
 		case 0:
 			break;
 		default:
-			usage("Too many arguments for --list-all", stderr);
+			usage(stderr, "Too many arguments for --list-all");
 			goto error;
 		}
 	} else {
@@ -677,10 +678,10 @@ bool parse_options(int argc, char **argv,
 			settings->results_path = absolute_path(argv[optind]);
 			break;
 		case 0:
-			usage("Results-path missing", stderr);
+			usage(stderr, "Results-path missing");
 			goto error;
 		default:
-			usage("Extra arguments after results-path", stderr);
+			usage(stderr, "Extra arguments after results-path");
 			goto error;
 		}
 		if (!settings->name) {
@@ -697,7 +698,7 @@ bool parse_options(int argc, char **argv,
 	}
 
 	if (!settings->test_root) {
-		usage("Test root not set", stderr);
+		usage(stderr, "Test root not set");
 		goto error;
 	}
 
@@ -714,17 +715,17 @@ bool validate_settings(struct settings *settings)
 	int dirfd, fd;
 
 	if (settings->test_list && !readable_file(settings->test_list)) {
-		usage("Cannot open test-list file", stderr);
+		usage(stderr, "Cannot open test-list file");
 		return false;
 	}
 
 	if (!settings->results_path) {
-		usage("No results-path set; this shouldn't happen", stderr);
+		usage(stderr, "No results-path set; this shouldn't happen");
 		return false;
 	}
 
 	if (!settings->test_root) {
-		usage("No test root set; this shouldn't happen", stderr);
+		usage(stderr, "No test root set; this shouldn't happen");
 		return false;
 	}
 
@@ -780,23 +781,24 @@ bool serialize_settings(struct settings *settings)
 	FILE *f;
 
 	if (!settings->results_path) {
-		usage("No results-path set; this shouldn't happen", stderr);
+		usage(stderr, "No results-path set; this shouldn't happen");
 		return false;
 	}
 
 	if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
 		mkdir(settings->results_path, 0755);
 		if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
-			usage("Creating results-path failed", stderr);
+			usage(stderr, "Creating results-path failed");
 			return false;
 		}
 	}
+
 	if (settings->enable_code_coverage) {
 		strcpy(path, settings->results_path);
 		strcat(path, CODE_COV_RESULTS_PATH);
 		if ((covfd = open(path, O_DIRECTORY | O_RDONLY)) < 0) {
 			if (mkdir(path, 0755)) {
-				usage("Creating code coverage path failed", stderr);
+				usage(stderr, "Creating code coverage path failed");
 				return false;
 			}
 		} else {
@@ -806,24 +808,19 @@ bool serialize_settings(struct settings *settings)
 
 	if (!settings->overwrite &&
 	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
-		usage("Settings metadata already exists and not overwriting", stderr);
+		usage(stderr, "Settings metadata already exists and not overwriting");
 		return false;
 	}
 
 	if (settings->overwrite &&
 	    unlinkat(dirfd, settings_filename, 0) != 0 &&
 	    errno != ENOENT) {
-		usage("Error removing old settings metadata", stderr);
+		usage(stderr, "Error removing old settings metadata");
 		return false;
 	}
 
 	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
-		char *msg;
-
-		asprintf(&msg, "Creating settings serialization file failed: %s", strerror(errno));
-		usage(msg, stderr);
-
-		free(msg);
+		usage(msg, "Creating settings serialization file failed: %s", strerror(errno));
 		close(dirfd);
 		return false;
 	}
-- 
2.36.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t 3/7] igt_list: Add the igt_list_empty_or_null helper
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 1/7] runner: rename free_settings to clear_settings Ryszard Knop
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic Ryszard Knop
@ 2022-06-28  9:44 ` Ryszard Knop
  2022-06-29 16:00   ` Mauro Carvalho Chehab
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag Ryszard Knop
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Checks if the list is empty or uninitialized.

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 lib/igt_list.c | 5 +++++
 lib/igt_list.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/lib/igt_list.c b/lib/igt_list.c
index 37ae139c..dc94fad8 100644
--- a/lib/igt_list.c
+++ b/lib/igt_list.c
@@ -81,3 +81,8 @@ bool igt_list_empty(const struct igt_list_head *head)
 {
 	return head->next == head;
 }
+
+bool igt_list_empty_or_null(const struct igt_list_head *head)
+{
+	return head->next == NULL || head->prev == NULL || igt_list_empty(head);
+}
diff --git a/lib/igt_list.h b/lib/igt_list.h
index be63fd80..4efb7a22 100644
--- a/lib/igt_list.h
+++ b/lib/igt_list.h
@@ -80,6 +80,7 @@ void igt_list_move(struct igt_list_head *elem, struct igt_list_head *list);
 void igt_list_move_tail(struct igt_list_head *elem, struct igt_list_head *list);
 int igt_list_length(const struct igt_list_head *head);
 bool igt_list_empty(const struct igt_list_head *head);
+bool igt_list_empty_or_null(const struct igt_list_head *head);
 
 #define igt_container_of(ptr, sample, member)				\
 	(__typeof__(sample))((char *)(ptr) -				\
-- 
2.36.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
                   ` (2 preceding siblings ...)
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 3/7] igt_list: Add the igt_list_empty_or_null helper Ryszard Knop
@ 2022-06-28  9:44 ` Ryszard Knop
  2022-06-29 16:13   ` Mauro Carvalho Chehab
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags Ryszard Knop
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

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@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;
+
+	env_kv = strdup(env_kv);
+	kv_split = strchr(env_kv, '=');
+	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);
+	}
+
+	if (igt_list_empty_or_null(env_vars))
+		IGT_INIT_LIST_HEAD(env_vars);
+
+	igt_list_add_tail(var, env_vars);
+
+	free(env_kv);
+	return true;
+
+error:
+	if (var != NULL)
+		free(var);
+
+	if (env_kv != NULL)
+		free(env_kv);
+
+	return false;
+}
+
+static void free_env_vars(struct igt_list_head *env_vars) {
+	struct environment_variable *iter;
+
+	while (!igt_list_empty_or_null(env_vars)) {
+		iter = igt_list_first_entry(env_vars, iter, link);
+
+		free(iter->key);
+		free(iter->value);
+
+		igt_list_del(iter);
+		free(iter);
+	}
+}
+
 static bool readable_file(const char *filename)
 {
 	return !access(filename, R_OK);
@@ -496,6 +604,7 @@ void clear_settings(struct settings *settings)
 
 	free_regexes(&settings->include_regexes);
 	free_regexes(&settings->exclude_regexes);
+	free_env_vars(&settings->env_vars);
 
 	init_settings(settings);
 }
@@ -514,6 +623,7 @@ bool parse_options(int argc, char **argv,
 		{"allow-non-root", no_argument, NULL, OPT_ALLOW_NON_ROOT},
 		{"include-tests", required_argument, NULL, OPT_INCLUDE},
 		{"exclude-tests", required_argument, NULL, OPT_EXCLUDE},
+		{"environment", required_argument, NULL, OPT_ENVIRONMENT},
 		{"abort-on-monitored-error", optional_argument, NULL, OPT_ABORT_ON_ERROR},
 		{"disk-usage-limit", required_argument, NULL, OPT_DISK_USAGE_LIMIT},
 		{"sync", no_argument, NULL, OPT_SYNC},
@@ -543,7 +653,7 @@ bool parse_options(int argc, char **argv,
 
 	settings->dmesg_warn_level = -1;
 
-	while ((c = getopt_long(argc, argv, "hn:dt:x:sl:omb:L",
+	while ((c = getopt_long(argc, argv, "hn:dt:x:e:sl:omb:L",
 				long_options, NULL)) != -1) {
 		switch (c) {
 		case OPT_VERSION:
@@ -569,6 +679,10 @@ bool parse_options(int argc, char **argv,
 			if (!add_regex(&settings->exclude_regexes, strdup(optarg)))
 				goto error;
 			break;
+		case OPT_ENVIRONMENT:
+			if (!add_env_var(&settings->env_vars, optarg))
+				goto error;
+			break;
 		case OPT_ABORT_ON_ERROR:
 			if (!parse_abort_conditions(settings, optarg))
 				goto error;
diff --git a/runner/settings.h b/runner/settings.h
index 6d444d01..819c3460 100644
--- a/runner/settings.h
+++ b/runner/settings.h
@@ -7,6 +7,8 @@
 #include <stdio.h>
 #include <glib.h>
 
+#include "igt_list.h"
+
 enum {
 	LOG_LEVEL_NORMAL = 0,
 	LOG_LEVEL_QUIET = -1,
@@ -37,6 +39,12 @@ struct regex_list {
 	size_t size;
 };
 
+struct environment_variable {
+	struct igt_list_head link;
+	char * key;
+	char * value;
+};
+
 struct settings {
 	int abort_mask;
 	size_t disk_usage_limit;
@@ -46,6 +54,7 @@ struct settings {
 	bool allow_non_root;
 	struct regex_list include_regexes;
 	struct regex_list exclude_regexes;
+	struct igt_list_head env_vars;
 	bool sync;
 	int log_level;
 	bool overwrite;
-- 
2.36.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
                   ` (3 preceding siblings ...)
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag Ryszard Knop
@ 2022-06-28  9:44 ` Ryszard Knop
  2022-06-29 16:36   ` Mauro Carvalho Chehab
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 6/7] runner: Set requested env vars during execution Ryszard Knop
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Settings serialization now creates a separate file, environment.txt,
which contains KEY=VALUE pairs of environment variables to use for test
process spawning. Only vars created with --environment are serialized.
Also add misc warning fixes.

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/settings.c | 206 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 169 insertions(+), 37 deletions(-)

diff --git a/runner/settings.c b/runner/settings.c
index de339484..1926e197 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -79,6 +79,9 @@ static struct {
 	{ 0, 0 },
 };
 
+static char settings_filename[] = "metadata.txt";
+static char env_filename[] = "environment.txt";
+
 static bool set_log_level(struct settings* settings, const char *level)
 {
 	typeof(*log_levels) *it;
@@ -296,6 +299,7 @@ static const char *usage_str =
 	"                        this option if given.\n"
 	;
 
+__attribute__ ((format(printf, 2, 3)))
 static void usage(FILE *f, const char *extra_message, ...)
 {
 	if (extra_message) {
@@ -448,7 +452,7 @@ static bool add_env_var(struct igt_list_head *env_vars, char *env_kv) {
 	var = malloc(sizeof(struct environment_variable));
 	if (kv_split != NULL) {
 		/* value provided - copy string contents after '=' */
-		kv_split[0] = NULL;
+		kv_split[0] = '\0';
 
 		var->key = string_trim_and_duplicate(env_kv);
 		var->value = strdup(kv_split + 1); /* Can be empty, that's okay */
@@ -468,7 +472,7 @@ static bool add_env_var(struct igt_list_head *env_vars, char *env_kv) {
 	if (igt_list_empty_or_null(env_vars))
 		IGT_INIT_LIST_HEAD(env_vars);
 
-	igt_list_add_tail(var, env_vars);
+	igt_list_add_tail(&var->link, env_vars);
 
 	free(env_kv);
 	return true;
@@ -492,11 +496,16 @@ static void free_env_vars(struct igt_list_head *env_vars) {
 		free(iter->key);
 		free(iter->value);
 
-		igt_list_del(iter);
+		igt_list_del(&iter->link);
 		free(iter);
 	}
 }
 
+static bool file_exists_at(int dirfd, const char *filename)
+{
+	return faccessat(dirfd, filename, F_OK, 0) == 0;
+}
+
 static bool readable_file(const char *filename)
 {
 	return !access(filename, R_OK);
@@ -885,14 +894,78 @@ bool validate_settings(struct settings *settings)
 	return true;
 }
 
-static char settings_filename[] = "metadata.txt";
+/**
+ * fopenat() - wrapper for fdopen(openat(dirfd, filename))
+ * @dirfd: directory fd to pass to openat
+ * @filename: file name to open with openat
+ * @flags: flags to pass to openat
+ * @openat_mode: (octal) mode to pass to openat
+ * @fdopen_mode: (text) mode to pass to fdopen
+ */
+static FILE *fopenat(int dirfd, char *filename, int flags, mode_t openat_mode, const char *fdopen_mode)
+{
+	int fd;
+	FILE *f;
+
+	if ((fd = openat(dirfd, filename, flags, openat_mode)) < 0) {
+		usage(stderr, "fopenat(%s, %d) failed: %s",
+		      filename, flags, strerror(errno));
+		return NULL;
+	}
+
+	f = fdopen(fd, fdopen_mode);
+	if (!f) {
+		close(fd);
+		return NULL;
+	}
+
+	return f;
+}
+
+static FILE *fopenat_read(int dirfd, char *filename)
+{
+	return fopenat(dirfd, filename, O_RDONLY, 0000, "r");
+}
+
+static FILE *fopenat_create(int dirfd, char *filename, bool overwrite)
+{
+	mode_t mode_if_exists = overwrite ? O_TRUNC : O_EXCL;
+	return fopenat(dirfd, filename, O_CREAT | O_RDWR | mode_if_exists, 0666, "w");
+}
+
+static bool serialize_environment(struct settings *settings, int dirfd)
+{
+	struct environment_variable *iter;
+	FILE *f;
+
+	if (file_exists_at(dirfd, env_filename) && !settings->overwrite) {
+		usage(stderr, "%s already exists, not overwriting", env_filename);
+		return false;
+	}
+
+	if ((f = fopenat_create(dirfd, env_filename, settings->overwrite)) == NULL)
+		return false;
+
+	igt_list_for_each_entry(iter, &settings->env_vars, link) {
+		fprintf(f, "%s=%s\n", iter->key, iter->value);
+	}
+
+	if (settings->sync) {
+		fflush(f);
+		fsync(fileno(f));
+	}
+
+	fclose(f);
+	return true;
+}
+
 bool serialize_settings(struct settings *settings)
 {
 #define SERIALIZE_LINE(f, s, name, format) fprintf(f, "%s : " format "\n", #name, s->name)
 
-	int dirfd, covfd, fd;
-	char path[PATH_MAX];
 	FILE *f;
+	int dirfd, covfd;
+	char path[PATH_MAX];
 
 	if (!settings->results_path) {
 		usage(stderr, "No results-path set; this shouldn't happen");
@@ -920,28 +993,13 @@ bool serialize_settings(struct settings *settings)
 		}
 	}
 
-	if (!settings->overwrite &&
-	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
-		usage(stderr, "Settings metadata already exists and not overwriting");
-		return false;
-	}
-
-	if (settings->overwrite &&
-	    unlinkat(dirfd, settings_filename, 0) != 0 &&
-	    errno != ENOENT) {
-		usage(stderr, "Error removing old settings metadata");
-		return false;
-	}
-
-	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
-		usage(msg, "Creating settings serialization file failed: %s", strerror(errno));
+	if (file_exists_at(dirfd, settings_filename) && !settings->overwrite) {
+		usage(stderr, "%s already exists, not overwriting", settings_filename);
 		close(dirfd);
 		return false;
 	}
 
-	f = fdopen(fd, "w");
-	if (!f) {
-		close(fd);
+	if ((f = fopenat_create(dirfd, settings_filename, settings->overwrite)) == NULL) {
 		close(dirfd);
 		return false;
 	}
@@ -972,11 +1030,22 @@ bool serialize_settings(struct settings *settings)
 	SERIALIZE_LINE(f, settings, code_coverage_script, "%s");
 
 	if (settings->sync) {
-		fsync(fd);
-		fsync(dirfd);
+		fflush(f);
+		fsync(fileno(f));
 	}
 
 	fclose(f);
+
+	if (!igt_list_empty_or_null(&settings->env_vars)) {
+		if (!serialize_environment(settings, dirfd)) {
+			close(dirfd);
+			return false;
+		}
+	}
+
+	if (settings->sync)
+		fsync(dirfd);
+
 	close(dirfd);
 	return true;
 
@@ -1045,28 +1114,91 @@ bool read_settings_from_file(struct settings *settings, FILE *f)
 #undef PARSE_LINE
 }
 
+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);
+	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 */
+	{
+		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;
 }
-- 
2.36.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t 6/7] runner: Set requested env vars during execution
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
                   ` (4 preceding siblings ...)
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags Ryszard Knop
@ 2022-06-28  9:44 ` Ryszard Knop
  2022-06-29 16:28   ` Mauro Carvalho Chehab
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 7/7] runner: Add help for the --environment flag Ryszard Knop
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/executor.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/runner/executor.c b/runner/executor.c
index 9b89cc09..21c41715 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1872,6 +1872,14 @@ bool execute(struct execute_state *state,
 		return true;
 	}
 
+	if (!igt_list_empty_or_null(&settings->env_vars)) {
+		struct environment_variable *iter;
+
+		igt_list_for_each_entry(iter, &settings->env_vars, link) {
+			setenv(iter->key, iter->value, 1);
+		}
+	}
+
 	if ((resdirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
 		/* Initialize state should have done this */
 		errf("Error: Failure opening results path %s\n",
-- 
2.36.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t 7/7] runner: Add help for the --environment flag
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
                   ` (5 preceding siblings ...)
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 6/7] runner: Set requested env vars during execution Ryszard Knop
@ 2022-06-28  9:44 ` Ryszard Knop
  2022-06-29 16:30   ` Mauro Carvalho Chehab
  2022-06-28 10:36 ` [igt-dev] ✓ Fi.CI.BAT: success for Add " Patchwork
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Ryszard Knop @ 2022-06-28  9:44 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/settings.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/runner/settings.c b/runner/settings.c
index 1926e197..383e641e 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -286,6 +286,10 @@ static const char *usage_str =
 	"  -b, --blacklist FILENAME\n"
 	"                        Exclude all test matching to regexes from FILENAME\n"
 	"                        (can be used more than once)\n"
+	"  -e, --environment <KEY or KEY=VALUE>\n"
+	"                        Set an environment variable for the test process.\n"
+	"                        If only the key is provided, the current value is read\n"
+	"                        from the runner's environment (and saved for resumes).\n"
 	"  -L, --list-all        List all matching subtests instead of running\n"
 	"  --collect-code-cov    Enables gcov-based collect of code coverage for tests.\n"
 	"                        Requires --collect-script FILENAME\n"
-- 
2.36.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for Add the --environment flag
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
                   ` (6 preceding siblings ...)
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 7/7] runner: Add help for the --environment flag Ryszard Knop
@ 2022-06-28 10:36 ` Patchwork
  2022-06-29  0:03 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
  9 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2022-06-28 10:36 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: igt-dev

[-- Attachment #1: Type: text/plain, Size: 8065 bytes --]

== Series Details ==

Series: Add the --environment flag
URL   : https://patchwork.freedesktop.org/series/105721/
State : success

== Summary ==

CI Bug Log - changes from IGT_6545 -> IGTPW_7417
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/index.html

Participating hosts (41 -> 40)
------------------------------

  Additional (1): bat-dg2-8 
  Missing    (2): fi-icl-u2 fi-bdw-samus 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_7417:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@gem:
    - {bat-adln-1}:       NOTRUN -> [DMESG-FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/bat-adln-1/igt@i915_selftest@live@gem.html

  
Known issues
------------

  Here are the changes found in IGTPW_7417 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@gem:
    - fi-pnv-d510:        NOTRUN -> [DMESG-FAIL][2] ([i915#4528])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/fi-pnv-d510/igt@i915_selftest@live@gem.html

  * igt@i915_selftest@live@gt_engines:
    - bat-dg1-5:          [PASS][3] -> [INCOMPLETE][4] ([i915#4418])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/bat-dg1-5/igt@i915_selftest@live@gt_engines.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/bat-dg1-5/igt@i915_selftest@live@gt_engines.html

  * igt@kms_busy@basic@flip:
    - bat-adlp-4:         [PASS][5] -> [DMESG-WARN][6] ([i915#3576])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/bat-adlp-4/igt@kms_busy@basic@flip.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/bat-adlp-4/igt@kms_busy@basic@flip.html

  * igt@kms_flip@basic-flip-vs-modeset@a-edp1:
    - fi-tgl-u2:          [PASS][7] -> [DMESG-WARN][8] ([i915#402])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/fi-tgl-u2/igt@kms_flip@basic-flip-vs-modeset@a-edp1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/fi-tgl-u2/igt@kms_flip@basic-flip-vs-modeset@a-edp1.html

  * igt@prime_vgem@basic-userptr:
    - fi-tgl-u2:          NOTRUN -> [SKIP][9] ([fdo#109295] / [i915#3301])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/fi-tgl-u2/igt@prime_vgem@basic-userptr.html

  
#### Possible fixes ####

  * igt@core_hotunplug@unbind-rebind:
    - {bat-adln-1}:       [DMESG-WARN][10] ([i915#6299]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/bat-adln-1/igt@core_hotunplug@unbind-rebind.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/bat-adln-1/igt@core_hotunplug@unbind-rebind.html

  * igt@gem_sync@basic-each:
    - fi-hsw-g3258:       [FAIL][12] -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/fi-hsw-g3258/igt@gem_sync@basic-each.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/fi-hsw-g3258/igt@gem_sync@basic-each.html

  * igt@i915_selftest@live@hangcheck:
    - {fi-jsl-1}:         [INCOMPLETE][14] ([i915#6106]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/fi-jsl-1/igt@i915_selftest@live@hangcheck.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/fi-jsl-1/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@migrate:
    - {bat-dg2-9}:        [DMESG-WARN][16] ([i915#5763]) -> [PASS][17] +9 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/bat-dg2-9/igt@i915_selftest@live@migrate.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/bat-dg2-9/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - fi-pnv-d510:        [DMESG-FAIL][18] ([i915#4528]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/fi-pnv-d510/igt@i915_selftest@live@requests.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/fi-pnv-d510/igt@i915_selftest@live@requests.html

  * igt@kms_busy@basic@modeset:
    - {bat-adlp-6}:       [DMESG-WARN][20] ([i915#3576]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/bat-adlp-6/igt@kms_busy@basic@modeset.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/bat-adlp-6/igt@kms_busy@basic@modeset.html

  * igt@kms_flip@basic-flip-vs-modeset@b-edp1:
    - bat-adlp-4:         [DMESG-WARN][22] ([i915#3576]) -> [PASS][23] +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/bat-adlp-4/igt@kms_flip@basic-flip-vs-modeset@b-edp1.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/bat-adlp-4/igt@kms_flip@basic-flip-vs-modeset@b-edp1.html

  * igt@kms_flip@basic-plain-flip@a-edp1:
    - fi-tgl-u2:          [DMESG-WARN][24] ([i915#402]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/fi-tgl-u2/igt@kms_flip@basic-plain-flip@a-edp1.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/fi-tgl-u2/igt@kms_flip@basic-plain-flip@a-edp1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4418]: https://gitlab.freedesktop.org/drm/intel/issues/4418
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5174]: https://gitlab.freedesktop.org/drm/intel/issues/5174
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5763]: https://gitlab.freedesktop.org/drm/intel/issues/5763
  [i915#5903]: https://gitlab.freedesktop.org/drm/intel/issues/5903
  [i915#6106]: https://gitlab.freedesktop.org/drm/intel/issues/6106
  [i915#6299]: https://gitlab.freedesktop.org/drm/intel/issues/6299


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_6545 -> IGTPW_7417

  CI-20190529: 20190529
  CI_DRM_11817: 001d43f0d51677c2f8a45e95dcaf63a95c3934e9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_7417: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/index.html
  IGT_6545: dff2d8a8f0b395cdf006e82ad1ec623400ab7443 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/index.html

[-- Attachment #2: Type: text/html, Size: 7665 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [i-g-t,2/7] runner: make the usage function variadic
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic Ryszard Knop
@ 2022-06-28 23:51   ` Maíra Canal
  2022-06-30 13:16     ` Knop, Ryszard
  2022-06-29 15:51   ` [igt-dev] [PATCH i-g-t 2/7] " Mauro Carvalho Chehab
  2022-07-25 11:53   ` Petri Latvala
  2 siblings, 1 reply; 32+ messages in thread
From: Maíra Canal @ 2022-06-28 23:51 UTC (permalink / raw)
  To: igt-dev, ryszard.knop; +Cc: Maíra Canal

On 6/28/22 11:44, Ryszard Knop wrote:
> For easier error logging, make the usage() function variadic and pass
> its arguments to vfprintf. Additionally, reorder its arguments so that
> it visually matches all the printf functions.
> 
> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> ---

Hi,

Overall this looks good, just a small typo.

> @@ -806,24 +808,19 @@ bool serialize_settings(struct settings *settings)
>  
>  	if (!settings->overwrite &&
>  	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
> -		usage("Settings metadata already exists and not overwriting", stderr);
> +		usage(stderr, "Settings metadata already exists and not overwriting");
>  		return false;
>  	}
>  
>  	if (settings->overwrite &&
>  	    unlinkat(dirfd, settings_filename, 0) != 0 &&
>  	    errno != ENOENT) {
> -		usage("Error removing old settings metadata", stderr);
> +		usage(stderr, "Error removing old settings metadata");
>  		return false;
>  	}
>  
>  	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
> -		char *msg;
> -
> -		asprintf(&msg, "Creating settings serialization file failed: %s", strerror(errno));
> -		usage(msg, stderr);
> -
> -		free(msg);
> +		usage(msg, "Creating settings serialization file failed: %s", strerror(errno));

I believe this should be:
    usage(stderr, "Creating settings serialization file failed: %s", strerror(errno));

Reviewed-by: Maíra Canal <maira.canal@usp.br>

Best Regards,
- Maíra Canal

>  		close(dirfd);
>  		return false;
>   }

^ permalink raw reply	[flat|nested] 32+ messages in thread
* [igt-dev] ✗ Fi.CI.IGT: failure for Add the --environment flag
  2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
                   ` (7 preceding siblings ...)
  2022-06-28 10:36 ` [igt-dev] ✓ Fi.CI.BAT: success for Add " Patchwork
@ 2022-06-29  0:03 ` Patchwork
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
  9 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2022-06-29  0:03 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: igt-dev

[-- Attachment #1: Type: text/plain, Size: 45380 bytes --]

== Series Details ==

Series: Add the --environment flag
URL   : https://patchwork.freedesktop.org/series/105721/
State : failure

== Summary ==

CI Bug Log - changes from IGT_6545_full -> IGTPW_7417_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_7417_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_7417_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/index.html

Participating hosts (7 -> 10)
------------------------------

  Additional (3): shard-rkl shard-dg1 shard-tglu 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_7417_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_ctx_persistence@smoketest:
    - shard-apl:          [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl7/igt@gem_ctx_persistence@smoketest.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl4/igt@gem_ctx_persistence@smoketest.html

  
#### Warnings ####

  * igt@i915_pm_rc6_residency@rc6-idle@bcs0:
    - shard-iclb:         [WARN][3] -> [FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb3/igt@i915_pm_rc6_residency@rc6-idle@bcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb6/igt@i915_pm_rc6_residency@rc6-idle@bcs0.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_pm_rc6_residency@rc6-idle@vecs0:
    - {shard-tglu}:       NOTRUN -> [WARN][5] +2 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglu-5/igt@i915_pm_rc6_residency@rc6-idle@vecs0.html

  
New tests
---------

  New tests have been introduced between IGT_6545_full and IGTPW_7417_full:

### New IGT tests (8) ###

  * igt@kms_async_flips@alternate-sync-async-flip@pipe-a-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [2.34] s

  * igt@kms_async_flips@alternate-sync-async-flip@pipe-b-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [2.15] s

  * igt@kms_async_flips@alternate-sync-async-flip@pipe-c-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [2.17] s

  * igt@kms_async_flips@alternate-sync-async-flip@pipe-d-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [2.15] s

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-a-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [0.61] s

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-b-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [0.50] s

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [0.50] s

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-hdmi-a-3:
    - Statuses : 1 pass(s)
    - Exec time: [0.51] s

  

Known issues
------------

  Here are the changes found in IGTPW_7417_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_busy@close-race:
    - shard-snb:          [PASS][6] -> [TIMEOUT][7] ([i915#5748])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-snb4/igt@gem_busy@close-race.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-snb7/igt@gem_busy@close-race.html

  * igt@gem_ctx_persistence@hostile:
    - shard-tglb:         [PASS][8] -> [FAIL][9] ([i915#2410])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-tglb2/igt@gem_ctx_persistence@hostile.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb7/igt@gem_ctx_persistence@hostile.html

  * igt@gem_eio@kms:
    - shard-tglb:         [PASS][10] -> [FAIL][11] ([i915#5784]) +1 similar issue
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-tglb7/igt@gem_eio@kms.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb3/igt@gem_eio@kms.html

  * igt@gem_exec_balancer@parallel-bb-first:
    - shard-iclb:         [PASS][12] -> [SKIP][13] ([i915#4525]) +2 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb2/igt@gem_exec_balancer@parallel-bb-first.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb5/igt@gem_exec_balancer@parallel-bb-first.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-glk:          [PASS][14] -> [FAIL][15] ([i915#2842])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-glk2/igt@gem_exec_fair@basic-none-share@rcs0.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk9/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [PASS][16] -> [FAIL][17] ([i915#2842])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl2/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl2/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-none-vip@rcs0:
    - shard-tglb:         [PASS][18] -> [FAIL][19] ([i915#2842]) +1 similar issue
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-tglb6/igt@gem_exec_fair@basic-none-vip@rcs0.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb6/igt@gem_exec_fair@basic-none-vip@rcs0.html

  * igt@gem_exec_fair@basic-none@bcs0:
    - shard-tglb:         NOTRUN -> [FAIL][20] ([i915#2842]) +4 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb2/igt@gem_exec_fair@basic-none@bcs0.html

  * igt@gem_exec_fair@basic-none@vcs1:
    - shard-iclb:         NOTRUN -> [FAIL][21] ([i915#2842]) +4 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb1/igt@gem_exec_fair@basic-none@vcs1.html
    - shard-kbl:          NOTRUN -> [FAIL][22] ([i915#2842])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl6/igt@gem_exec_fair@basic-none@vcs1.html

  * igt@gem_exec_fair@basic-pace@vcs0:
    - shard-iclb:         [PASS][23] -> [FAIL][24] ([i915#2842]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb8/igt@gem_exec_fair@basic-pace@vcs0.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb8/igt@gem_exec_fair@basic-pace@vcs0.html

  * igt@gem_lmem_swapping@heavy-verify-random-ccs:
    - shard-apl:          NOTRUN -> [SKIP][25] ([fdo#109271] / [i915#4613])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl3/igt@gem_lmem_swapping@heavy-verify-random-ccs.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - shard-glk:          NOTRUN -> [SKIP][26] ([fdo#109271] / [i915#4613])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk7/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_ppgtt@flink-and-close-vma-leak:
    - shard-glk:          [PASS][27] -> [FAIL][28] ([i915#644])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-glk2/igt@gem_ppgtt@flink-and-close-vma-leak.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk2/igt@gem_ppgtt@flink-and-close-vma-leak.html

  * igt@gem_render_copy@yf-tiled-mc-ccs-to-vebox-y-tiled:
    - shard-iclb:         NOTRUN -> [SKIP][29] ([i915#768])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb8/igt@gem_render_copy@yf-tiled-mc-ccs-to-vebox-y-tiled.html

  * igt@gem_softpin@evict-single-offset:
    - shard-apl:          NOTRUN -> [FAIL][30] ([i915#4171])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl4/igt@gem_softpin@evict-single-offset.html

  * igt@gem_userptr_blits@coherency-unsync:
    - shard-iclb:         NOTRUN -> [SKIP][31] ([i915#3297])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb8/igt@gem_userptr_blits@coherency-unsync.html
    - shard-tglb:         NOTRUN -> [SKIP][32] ([i915#3297])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb3/igt@gem_userptr_blits@coherency-unsync.html

  * igt@gen9_exec_parse@basic-rejected-ctx-param:
    - shard-snb:          NOTRUN -> [SKIP][33] ([fdo#109271]) +38 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-snb5/igt@gen9_exec_parse@basic-rejected-ctx-param.html
    - shard-tglb:         NOTRUN -> [SKIP][34] ([i915#2527] / [i915#2856]) +1 similar issue
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb2/igt@gen9_exec_parse@basic-rejected-ctx-param.html
    - shard-iclb:         NOTRUN -> [SKIP][35] ([i915#2856]) +1 similar issue
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@gen9_exec_parse@basic-rejected-ctx-param.html

  * igt@i915_module_load@reload-with-fault-injection:
    - shard-snb:          [PASS][36] -> [DMESG-WARN][37] ([i915#6201])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-snb4/igt@i915_module_load@reload-with-fault-injection.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-snb4/igt@i915_module_load@reload-with-fault-injection.html

  * igt@i915_pm_dc@dc6-dpms:
    - shard-iclb:         [PASS][38] -> [FAIL][39] ([i915#454]) +1 similar issue
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb2/igt@i915_pm_dc@dc6-dpms.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb3/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_dc@dc9-dpms:
    - shard-apl:          [PASS][40] -> [SKIP][41] ([fdo#109271])
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl4/igt@i915_pm_dc@dc9-dpms.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl3/igt@i915_pm_dc@dc9-dpms.html

  * igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-0-hflip:
    - shard-kbl:          NOTRUN -> [SKIP][42] ([fdo#109271]) +28 similar issues
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl1/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-0-hflip.html
    - shard-tglb:         NOTRUN -> [SKIP][43] ([fdo#111615])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb7/igt@kms_big_fb@yf-tiled-max-hw-stride-32bpp-rotate-0-hflip.html

  * igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc:
    - shard-glk:          NOTRUN -> [SKIP][44] ([fdo#109271] / [i915#3886])
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk2/igt@kms_ccs@pipe-a-crc-primary-rotation-180-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_ccs@pipe-a-missing-ccs-buffer-y_tiled_gen12_rc_ccs_cc:
    - shard-apl:          NOTRUN -> [SKIP][45] ([fdo#109271] / [i915#3886]) +5 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl4/igt@kms_ccs@pipe-a-missing-ccs-buffer-y_tiled_gen12_rc_ccs_cc.html

  * igt@kms_ccs@pipe-c-bad-pixel-format-4_tiled_dg2_rc_ccs:
    - shard-tglb:         NOTRUN -> [SKIP][46] ([i915#6095]) +1 similar issue
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb3/igt@kms_ccs@pipe-c-bad-pixel-format-4_tiled_dg2_rc_ccs.html

  * igt@kms_ccs@pipe-c-bad-pixel-format-yf_tiled_ccs:
    - shard-tglb:         NOTRUN -> [SKIP][47] ([fdo#111615] / [i915#3689])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb1/igt@kms_ccs@pipe-c-bad-pixel-format-yf_tiled_ccs.html

  * igt@kms_ccs@pipe-d-crc-primary-basic-4_tiled_dg2_rc_ccs:
    - shard-iclb:         NOTRUN -> [SKIP][48] ([fdo#109278]) +4 similar issues
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@kms_ccs@pipe-d-crc-primary-basic-4_tiled_dg2_rc_ccs.html

  * igt@kms_ccs@pipe-d-crc-primary-rotation-180-y_tiled_ccs:
    - shard-tglb:         NOTRUN -> [SKIP][49] ([i915#3689]) +2 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb6/igt@kms_ccs@pipe-d-crc-primary-rotation-180-y_tiled_ccs.html

  * igt@kms_cdclk@plane-scaling:
    - shard-iclb:         NOTRUN -> [SKIP][50] ([i915#3742])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb7/igt@kms_cdclk@plane-scaling.html
    - shard-tglb:         NOTRUN -> [SKIP][51] ([i915#3742])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb7/igt@kms_cdclk@plane-scaling.html

  * igt@kms_chamelium@hdmi-hpd-storm:
    - shard-apl:          NOTRUN -> [SKIP][52] ([fdo#109271] / [fdo#111827]) +11 similar issues
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl3/igt@kms_chamelium@hdmi-hpd-storm.html

  * igt@kms_chamelium@vga-edid-read:
    - shard-iclb:         NOTRUN -> [SKIP][53] ([fdo#109284] / [fdo#111827]) +1 similar issue
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb3/igt@kms_chamelium@vga-edid-read.html

  * igt@kms_color_chamelium@pipe-a-ctm-0-5:
    - shard-glk:          NOTRUN -> [SKIP][54] ([fdo#109271] / [fdo#111827]) +4 similar issues
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk6/igt@kms_color_chamelium@pipe-a-ctm-0-5.html

  * igt@kms_color_chamelium@pipe-c-ctm-0-75:
    - shard-kbl:          NOTRUN -> [SKIP][55] ([fdo#109271] / [fdo#111827]) +1 similar issue
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl6/igt@kms_color_chamelium@pipe-c-ctm-0-75.html
    - shard-snb:          NOTRUN -> [SKIP][56] ([fdo#109271] / [fdo#111827]) +1 similar issue
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-snb5/igt@kms_color_chamelium@pipe-c-ctm-0-75.html
    - shard-tglb:         NOTRUN -> [SKIP][57] ([fdo#109284] / [fdo#111827]) +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb3/igt@kms_color_chamelium@pipe-c-ctm-0-75.html

  * igt@kms_cursor_crc@cursor-rapid-movement@pipe-c-dp-1-512x512:
    - shard-apl:          NOTRUN -> [SKIP][58] ([fdo#109271]) +158 similar issues
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl3/igt@kms_cursor_crc@cursor-rapid-movement@pipe-c-dp-1-512x512.html

  * igt@kms_flip@2x-wf_vblank-ts-check:
    - shard-iclb:         NOTRUN -> [SKIP][59] ([fdo#109274]) +7 similar issues
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb3/igt@kms_flip@2x-wf_vblank-ts-check.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-dp1:
    - shard-apl:          [PASS][60] -> [FAIL][61] ([i915#2122])
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl2/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-dp1.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl4/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-dp1.html

  * igt@kms_flip@flip-vs-suspend-interruptible@c-dp1:
    - shard-kbl:          [PASS][62] -> [INCOMPLETE][63] ([i915#3614] / [i915#794])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-kbl4/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl3/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html

  * igt@kms_flip@flip-vs-suspend@b-dp1:
    - shard-apl:          [PASS][64] -> [DMESG-WARN][65] ([i915#180]) +1 similar issue
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl3/igt@kms_flip@flip-vs-suspend@b-dp1.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl8/igt@kms_flip@flip-vs-suspend@b-dp1.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-downscaling:
    - shard-iclb:         [PASS][66] -> [SKIP][67] ([i915#3701])
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb3/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-downscaling.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-downscaling.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-upscaling:
    - shard-glk:          [PASS][68] -> [FAIL][69] ([i915#4911])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-glk9/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-upscaling.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk8/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-64bpp-ytile-upscaling.html

  * igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs-upscaling:
    - shard-iclb:         NOTRUN -> [SKIP][70] ([i915#2587])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb3/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs-upscaling.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-move:
    - shard-tglb:         NOTRUN -> [SKIP][71] ([fdo#109280] / [fdo#111825]) +3 similar issues
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb1/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-move.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-shrfb-pgflip-blt:
    - shard-iclb:         NOTRUN -> [SKIP][72] ([fdo#109280]) +2 similar issues
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-shrfb-pgflip-blt.html

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          NOTRUN -> [SKIP][73] ([fdo#109271]) +52 similar issues
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk1/igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-mmap-cpu.html

  * igt@kms_hdr@bpc-switch@pipe-a-dp-1:
    - shard-kbl:          [PASS][74] -> [FAIL][75] ([i915#1188])
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-kbl1/igt@kms_hdr@bpc-switch@pipe-a-dp-1.html
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl1/igt@kms_hdr@bpc-switch@pipe-a-dp-1.html

  * igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb:
    - shard-apl:          NOTRUN -> [FAIL][76] ([fdo#108145] / [i915#265]) +2 similar issues
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl8/igt@kms_plane_alpha_blend@pipe-b-alpha-opaque-fb.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
    - shard-glk:          NOTRUN -> [FAIL][77] ([fdo#108145] / [i915#265]) +1 similar issue
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk5/igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb.html
    - shard-kbl:          NOTRUN -> [FAIL][78] ([fdo#108145] / [i915#265]) +1 similar issue
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl6/igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb.html

  * igt@kms_plane_scaling@2x-scaler-multi-pipe:
    - shard-tglb:         NOTRUN -> [SKIP][79] ([fdo#109274] / [fdo#111825]) +7 similar issues
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb6/igt@kms_plane_scaling@2x-scaler-multi-pipe.html

  * igt@kms_psr2_sf@cursor-plane-update-sf:
    - shard-apl:          NOTRUN -> [SKIP][80] ([fdo#109271] / [i915#658]) +2 similar issues
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl2/igt@kms_psr2_sf@cursor-plane-update-sf.html

  * igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf:
    - shard-iclb:         NOTRUN -> [SKIP][81] ([i915#658])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb5/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf.html
    - shard-kbl:          NOTRUN -> [SKIP][82] ([fdo#109271] / [i915#658])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl6/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf.html
    - shard-tglb:         NOTRUN -> [SKIP][83] ([i915#2920])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb2/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf.html
    - shard-glk:          NOTRUN -> [SKIP][84] ([fdo#109271] / [i915#658])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk8/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-sf.html

  * igt@kms_psr_stress_test@invalidate-primary-flip-overlay:
    - shard-tglb:         [PASS][85] -> [SKIP][86] ([i915#5519])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-tglb1/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb2/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html

  * igt@kms_writeback@writeback-check-output:
    - shard-apl:          NOTRUN -> [SKIP][87] ([fdo#109271] / [i915#2437])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl4/igt@kms_writeback@writeback-check-output.html

  * igt@perf_pmu@event-wait@rcs0:
    - shard-iclb:         NOTRUN -> [SKIP][88] ([fdo#112283])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@perf_pmu@event-wait@rcs0.html
    - shard-tglb:         NOTRUN -> [SKIP][89] ([fdo#112283])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb6/igt@perf_pmu@event-wait@rcs0.html

  * igt@sw_sync@sync_multi_timeline_wait:
    - shard-apl:          NOTRUN -> [FAIL][90] ([i915#6140])
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl7/igt@sw_sync@sync_multi_timeline_wait.html

  * igt@sysfs_clients@recycle:
    - shard-glk:          NOTRUN -> [SKIP][91] ([fdo#109271] / [i915#2994])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk6/igt@sysfs_clients@recycle.html

  * igt@sysfs_clients@split-10:
    - shard-apl:          NOTRUN -> [SKIP][92] ([fdo#109271] / [i915#2994])
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl7/igt@sysfs_clients@split-10.html

  
#### Possible fixes ####

  * igt@feature_discovery@psr2:
    - shard-iclb:         [SKIP][93] ([i915#658]) -> [PASS][94]
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb8/igt@feature_discovery@psr2.html
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@feature_discovery@psr2.html

  * igt@gem_eio@in-flight-contexts-10ms:
    - shard-apl:          [TIMEOUT][95] ([i915#3063]) -> [PASS][96]
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl8/igt@gem_eio@in-flight-contexts-10ms.html
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl1/igt@gem_eio@in-flight-contexts-10ms.html

  * igt@gem_exec_balancer@parallel-out-fence:
    - shard-iclb:         [SKIP][97] ([i915#4525]) -> [PASS][98] +2 similar issues
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb6/igt@gem_exec_balancer@parallel-out-fence.html
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@gem_exec_balancer@parallel-out-fence.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-tglb:         [FAIL][99] ([i915#2842]) -> [PASS][100] +1 similar issue
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-tglb1/igt@gem_exec_fair@basic-none-share@rcs0.html
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb5/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_fair@basic-pace@vcs0:
    - shard-kbl:          [FAIL][101] ([i915#2842]) -> [PASS][102]
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-kbl3/igt@gem_exec_fair@basic-pace@vcs0.html
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl3/igt@gem_exec_fair@basic-pace@vcs0.html

  * igt@gem_exec_whisper@basic-fds-priority:
    - shard-glk:          [DMESG-WARN][103] ([i915#118]) -> [PASS][104]
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-glk8/igt@gem_exec_whisper@basic-fds-priority.html
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk6/igt@gem_exec_whisper@basic-fds-priority.html

  * igt@gem_huc_copy@huc-copy:
    - shard-tglb:         [SKIP][105] ([i915#2190]) -> [PASS][106]
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-tglb6/igt@gem_huc_copy@huc-copy.html
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb5/igt@gem_huc_copy@huc-copy.html

  * igt@gem_render_copy_redux@flink-interruptible:
    - shard-glk:          [INCOMPLETE][107] -> [PASS][108]
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-glk7/igt@gem_render_copy_redux@flink-interruptible.html
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk8/igt@gem_render_copy_redux@flink-interruptible.html

  * igt@gem_userptr_blits@huge-split:
    - shard-glk:          [FAIL][109] ([i915#3376]) -> [PASS][110]
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-glk5/igt@gem_userptr_blits@huge-split.html
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk9/igt@gem_userptr_blits@huge-split.html

  * igt@kms_cursor_legacy@cursor-vs-flip@legacy:
    - shard-iclb:         [FAIL][111] -> [PASS][112]
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb7/igt@kms_cursor_legacy@cursor-vs-flip@legacy.html
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb8/igt@kms_cursor_legacy@cursor-vs-flip@legacy.html

  * igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size:
    - shard-glk:          [FAIL][113] ([i915#2346]) -> [PASS][114] +1 similar issue
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-glk2/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-glk8/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html

  * igt@kms_hdr@bpc-switch-suspend@pipe-a-dp-1:
    - shard-kbl:          [FAIL][115] ([i915#1188]) -> [PASS][116]
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-kbl7/igt@kms_hdr@bpc-switch-suspend@pipe-a-dp-1.html
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-kbl4/igt@kms_hdr@bpc-switch-suspend@pipe-a-dp-1.html

  * igt@kms_plane_scaling@planes-downscale-factor-0-5@pipe-a-edp-1:
    - shard-iclb:         [SKIP][117] ([i915#5235]) -> [PASS][118] +2 similar issues
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb2/igt@kms_plane_scaling@planes-downscale-factor-0-5@pipe-a-edp-1.html
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb7/igt@kms_plane_scaling@planes-downscale-factor-0-5@pipe-a-edp-1.html

  * igt@kms_psr@psr2_sprite_blt:
    - shard-iclb:         [SKIP][119] ([fdo#109441]) -> [PASS][120] +1 similar issue
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb8/igt@kms_psr@psr2_sprite_blt.html
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@kms_psr@psr2_sprite_blt.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-apl:          [DMESG-WARN][121] ([i915#180]) -> [PASS][122] +2 similar issues
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl2/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  
#### Warnings ####

  * igt@i915_pm_dc@dc3co-vpb-simulation:
    - shard-iclb:         [SKIP][123] ([i915#588]) -> [SKIP][124] ([i915#658])
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb1/igt@i915_pm_dc@dc3co-vpb-simulation.html

  * igt@kms_content_protection@mei_interface:
    - shard-tglb:         [SKIP][125] ([fdo#109300]) -> [SKIP][126] ([i915#1063])
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-tglb3/igt@kms_content_protection@mei_interface.html
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-tglb5/igt@kms_content_protection@mei_interface.html
    - shard-iclb:         [SKIP][127] ([fdo#109300]) -> [SKIP][128] ([fdo#109300] / [fdo#111066])
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb8/igt@kms_content_protection@mei_interface.html
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb5/igt@kms_content_protection@mei_interface.html

  * igt@kms_psr2_sf@cursor-plane-move-continuous-exceed-fully-sf:
    - shard-iclb:         [SKIP][129] ([i915#2920]) -> [SKIP][130] ([i915#658])
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb2/igt@kms_psr2_sf@cursor-plane-move-continuous-exceed-fully-sf.html
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb1/igt@kms_psr2_sf@cursor-plane-move-continuous-exceed-fully-sf.html

  * igt@kms_psr2_sf@cursor-plane-update-sf:
    - shard-iclb:         [SKIP][131] ([i915#2920]) -> [SKIP][132] ([fdo#111068] / [i915#658])
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb2/igt@kms_psr2_sf@cursor-plane-update-sf.html
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb7/igt@kms_psr2_sf@cursor-plane-update-sf.html

  * igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf:
    - shard-iclb:         [SKIP][133] ([i915#658]) -> [SKIP][134] ([i915#2920])
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-iclb3/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf.html
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-iclb2/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf.html

  * igt@runner@aborted:
    - shard-apl:          ([FAIL][135], [FAIL][136], [FAIL][137], [FAIL][138], [FAIL][139], [FAIL][140]) ([fdo#109271] / [i915#180] / [i915#3002] / [i915#4312] / [i915#5257]) -> ([FAIL][141], [FAIL][142], [FAIL][143], [FAIL][144]) ([fdo#109271] / [i915#180] / [i915#3002] / [i915#337] / [i915#4312] / [i915#5257])
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl7/igt@runner@aborted.html
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl1/igt@runner@aborted.html
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl7/igt@runner@aborted.html
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl2/igt@runner@aborted.html
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl3/igt@runner@aborted.html
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6545/shard-apl3/igt@runner@aborted.html
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl4/igt@runner@aborted.html
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl8/igt@runner@aborted.html
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl4/igt@runner@aborted.html
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/shard-apl7/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [IGT#2]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/2
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109300]: https://bugs.freedesktop.org/show_bug.cgi?id=109300
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110254]: https://bugs.freedesktop.org/show_bug.cgi?id=110254
  [fdo#110542]: https://bugs.freedesktop.org/show_bug.cgi?id=110542
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [fdo#111066]: https://bugs.freedesktop.org/show_bug.cgi?id=111066
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111314]: https://bugs.freedesktop.org/show_bug.cgi?id=111314
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644
  [fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1063]: https://gitlab.freedesktop.org/drm/intel/issues/1063
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1149]: https://gitlab.freedesktop.org/drm/intel/issues/1149
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#1257]: https://gitlab.freedesktop.org/drm/intel/issues/1257
  [i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1722]: https://gitlab.freedesktop.org/drm/intel/issues/1722
  [i915#1755]: https://gitlab.freedesktop.org/drm/intel/issues/1755
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2410]: https://gitlab.freedesktop.org/drm/intel/issues/2410
  [i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2530]: https://gitlab.freedesktop.org/drm/intel/issues/2530
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#284]: https://gitlab.freedesktop.org/drm/intel/issues/284
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
  [i915#2994]: https://gitlab.freedesktop.org/drm/intel/issues/2994
  [i915#3002]: https://gitlab.freedesktop.org/drm/intel/issues/3002
  [i915#3012]: https://gitlab.freedesktop.org/drm/intel/issues/3012
  [i915#3063]: https://gitlab.freedesktop.org/drm/intel/issues/3063
  [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3323]: https://gitlab.freedesktop.org/drm/intel/issues/3323
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361
  [i915#337]: https://gitlab.freedesktop.org/drm/intel/issues/337
  [i915#3376]: https://gitlab.freedesktop.org/drm/intel/issues/3376
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3558]: https://gitlab.freedesktop.org/drm/intel/issues/3558
  [i915#3614]: https://gitlab.freedesktop.org/drm/intel/issues/3614
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3701]: https://gitlab.freedesktop.org/drm/intel/issues/3701
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
  [i915#3736]: https://gitlab.freedesktop.org/drm/intel/issues/3736
  [i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
  [i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804
  [i915#3825]: https://gitlab.freedesktop.org/drm/intel/issues/3825
  [i915#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
  [i915#3828]: https://gitlab.freedesktop.org/drm/intel/issues/3828
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3936]: https://gitlab.freedesktop.org/drm/intel/issues/3936
  [i915#3938]: https://gitlab.freedesktop.org/drm/intel/issues/3938
  [i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#3966]: https://gitlab.freedesktop.org/drm/intel/issues/3966
  [i915#4016]: https://gitlab.freedesktop.org/drm/intel/issues/4016
  [i915#4036]: https://gitlab.freedesktop.org/drm/intel/issues/4036
  [i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4171]: https://gitlab.freedesktop.org/drm/intel/issues/4171
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4278]: https://gitlab.freedesktop.org/drm/intel/issues/4278
  [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#433]: https://gitlab.freedesktop.org/drm/intel/issues/433
  [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
  [i915#4369]: https://gitlab.freedesktop.org/drm/intel/issues/4369
  [i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4462]: https://gitlab.freedesktop.org/drm/intel/issues/4462
  [i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
  [i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4842]: https://gitlab.freedesktop.org/drm/intel/issues/4842
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4853]: https://gitlab.freedesktop.org/drm/intel/issues/4853
  [i915#4854]: https://gitlab.freedesktop.org/drm/intel/issues/4854
  [i915#4855]: https://gitlab.freedesktop.org/drm/intel/issues/4855
  [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4877]: https://gitlab.freedesktop.org/drm/intel/issues/4877
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#4883]: https://gitlab.freedesktop.org/drm/intel/issues/4883
  [i915#4884]: https://gitlab.freedesktop.org/drm/intel/issues/4884
  [i915#4885]: https://gitlab.freedesktop.org/drm/intel/issues/4885
  [i915#4893]: https://gitlab.freedesktop.org/drm/intel/issues/4893
  [i915#4911]: https://gitlab.freedesktop.org/drm/intel/issues/4911
  [i915#4941]: https://gitlab.freedesktop.org/drm/intel/issues/4941
  [i915#4958]: https://gitlab.freedesktop.org/drm/intel/issues/4958
  [i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
  [i915#5115]: https://gitlab.freedesktop.org/drm/intel/issues/5115
  [i915#5174]: https://gitlab.freedesktop.org/drm/intel/issues/5174
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5257]: https://gitlab.freedesktop.org/drm/intel/issues/5257
  [i915#5264]: https://gitlab.freedesktop.org/drm/intel/issues/5264
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5287]: https://gitlab.freedesktop.org/drm/intel/issues/5287
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5303]: https://gitlab.freedesktop.org/drm/intel/issues/5303
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5327]: https://gitlab.freedesktop.org/drm/intel/issues/5327
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5519]: https://gitlab.freedesktop.org/drm/intel/issues/5519
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5723]: https://gitlab.freedesktop.org/drm/intel/issues/5723
  [i915#5748]: https://gitlab.freedesktop.org/drm/intel/issues/5748
  [i915#5784]: https://gitlab.freedesktop.org/drm/intel/issues/5784
  [i915#588]: https://gitlab.freedesktop.org/drm/intel/issues/588
  [i915#5903]: https://gitlab.freedesktop.org/drm/intel/issues/5903
  [i915#6011]: https://gitlab.freedesktop.org/drm/intel/issues/6011
  [i915#6076]: https://gitlab.freedesktop.org/drm/intel/issues/6076
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6117]: https://gitlab.freedesktop.org/drm/intel/issues/6117
  [i915#6140]: https://gitlab.freedesktop.org/drm/intel/issues/6140
  [i915#6201]: https://gitlab.freedesktop.org/drm/intel/issues/6201
  [i915#6230]: https://gitlab.freedesktop.org/drm/intel/issues/6230
  [i915#6245]: https://gitlab.freedesktop.org/drm/intel/issues/6245
  [i915#6247]: https://gitlab.freedesktop.org/drm/intel/issues/6247
  [i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
  [i915#6258]: https://gitlab.freedesktop.org/drm/intel/issues/6258
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
  [i915#644]: https://gitlab.freedesktop.org/drm/intel/issues/644
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#768]: https://gitlab.freedesktop.org/drm/intel/issues/768
  [i915#794]: https://gitlab.freedesktop.org/drm/intel/issues/794


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_6545 -> IGTPW_7417

  CI-20190529: 20190529
  CI_DRM_11817: 001d43f0d51677c2f8a45e95dcaf63a95c3934e9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_7417: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/index.html
  IGT_6545: dff2d8a8f0b395cdf006e82ad1ec623400ab7443 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_7417/index.html

[-- Attachment #2: Type: text/html, Size: 43683 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/7] runner: rename free_settings to clear_settings
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 1/7] runner: rename free_settings to clear_settings Ryszard Knop
@ 2022-06-29 15:48   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-06-29 15:48 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, 28 Jun 2022 11:44:29 +0200
Ryszard Knop <ryszard.knop@intel.com> wrote:

> That function does not actually free the settings pointer - rename it
> for clarity.

Yeah, despite freeing several settings-> functions, as it doesn't free
setting itself, I guess it is fair to rename it to clear_settings().

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> 
> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> ---
>  runner/executor.c     |  2 +-
>  runner/resultgen.c    |  2 +-
>  runner/runner_tests.c | 12 ++++++------
>  runner/settings.c     |  8 ++++----
>  runner/settings.h     |  6 +++---
>  5 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index 6e6ca9cc..9b89cc09 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -1547,7 +1547,7 @@ bool initialize_execute_state_from_resume(int dirfd,
>  	struct job_list_entry *entry;
>  	int resdirfd, fd, i;
>  
> -	free_settings(settings);
> +	clear_settings(settings);
>  	free_job_list(list);
>  	memset(state, 0, sizeof(*state));
>  	state->resuming = true;
> diff --git a/runner/resultgen.c b/runner/resultgen.c
> index 6ecf325b..8e11b2b1 100644
> --- a/runner/resultgen.c
> +++ b/runner/resultgen.c
> @@ -1702,7 +1702,7 @@ struct json_object *generate_results_json(int dirfd)
>  		close(fd);
>  	}
>  
> -	free_settings(&settings);
> +	clear_settings(&settings);
>  	free_job_list(&job_list);
>  
>  	return obj;
> diff --git a/runner/runner_tests.c b/runner/runner_tests.c
> index 8fe86978..aca031fc 100644
> --- a/runner/runner_tests.c
> +++ b/runner/runner_tests.c
> @@ -139,7 +139,7 @@ static void job_list_filter_test(const char *name, const char *filterarg1, const
>  	}
>  
>  	igt_fixture {
> -		free_settings(settings);
> +		clear_settings(settings);
>  		free(settings);
>  	}
>  }
> @@ -953,7 +953,7 @@ igt_main
>  			close(fd);
>  			close(dirfd);
>  			clear_directory(dirname);
> -			free_settings(cmp_settings);
> +			clear_settings(cmp_settings);
>  			free(cmp_settings);
>  		}
>  	}
> @@ -1176,7 +1176,7 @@ igt_main
>  			igt_assert(write(fd, journaltext, strlen(journaltext)) == strlen(journaltext));
>  
>  			free_job_list(list);
> -			free_settings(settings);
> +			clear_settings(settings);
>  			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
>  
>  			igt_assert_eq(state.next, 0);
> @@ -1233,7 +1233,7 @@ igt_main
>  			igt_assert(write(fd, journaltext, strlen(journaltext)) == strlen(journaltext));
>  
>  			free_job_list(list);
> -			free_settings(settings);
> +			clear_settings(settings);
>  			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
>  
>  			/* All subtests are in journal, the entry should be considered completed */
> @@ -1294,7 +1294,7 @@ igt_main
>  			igt_assert_eq(write(fd, journaltext, sizeof(journaltext)), sizeof(journaltext));
>  
>  			free_job_list(list);
> -			free_settings(settings);
> +			clear_settings(settings);
>  			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
>  
>  			igt_assert_eq(state.next, 1);
> @@ -1942,7 +1942,7 @@ igt_main
>  	}
>  
>  	igt_fixture {
> -		free_settings(settings);
> +		clear_settings(settings);
>  		free(settings);
>  	}
>  }
> diff --git a/runner/settings.c b/runner/settings.c
> index bb63a9f4..06947c91 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -486,7 +486,7 @@ void init_settings(struct settings *settings)
>  	memset(settings, 0, sizeof(*settings));
>  }
>  
> -void free_settings(struct settings *settings)
> +void clear_settings(struct settings *settings)
>  {
>  	free(settings->test_list);
>  	free(settings->name);
> @@ -536,7 +536,7 @@ bool parse_options(int argc, char **argv,
>  		{ 0, 0, 0, 0},
>  	};
>  
> -	free_settings(settings);
> +	clear_settings(settings);
>  
>  	optind = 1;
>  
> @@ -705,7 +705,7 @@ bool parse_options(int argc, char **argv,
>  	return true;
>  
>   error:
> -	free_settings(settings);
> +	clear_settings(settings);
>  	return false;
>  }
>  
> @@ -939,7 +939,7 @@ bool read_settings_from_dir(struct settings *settings, int dirfd)
>  	int fd;
>  	FILE *f;
>  
> -	free_settings(settings);
> +	clear_settings(settings);
>  
>  	if ((fd = openat(dirfd, settings_filename, O_RDONLY)) < 0)
>  		return false;
> diff --git a/runner/settings.h b/runner/settings.h
> index 6ae64695..6d444d01 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -76,14 +76,14 @@ struct settings {
>  void init_settings(struct settings *settings);
>  
>  /**
> - * free_settings:
> + * clear_settings:
>   *
>   * Releases all allocated resources for a settings object and
>   * initializes it to an empty state (see #init_settings).
>   *
> - * @settings: Object to release and initialize.
> + * @settings: Object to release and reinitialize.
>   */
> -void free_settings(struct settings *settings);
> +void clear_settings(struct settings *settings);
>  
>  /**
>   * parse_options:

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic Ryszard Knop
  2022-06-28 23:51   ` [igt-dev] [i-g-t,2/7] " Maíra Canal
@ 2022-06-29 15:51   ` Mauro Carvalho Chehab
  2022-07-25 11:53   ` Petri Latvala
  2 siblings, 0 replies; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-06-29 15:51 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, 28 Jun 2022 11:44:30 +0200
Ryszard Knop <ryszard.knop@intel.com> wrote:

> For easier error logging, make the usage() function variadic and pass
> its arguments to vfprintf. Additionally, reorder its arguments so that
> it visually matches all the printf functions.
> 
> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  runner/settings.c | 63 ++++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/runner/settings.c b/runner/settings.c
> index 06947c91..d153954a 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -295,10 +295,16 @@ static const char *usage_str =
>  	"                        this option if given.\n"
>  	;
>  
> -static void usage(const char *extra_message, FILE *f)
> +static void usage(FILE *f, const char *extra_message, ...)
>  {
> -	if (extra_message)
> -		fprintf(f, "%s\n\n", extra_message);
> +	if (extra_message) {
> +		va_list args;
> +
> +		va_start(args, extra_message);
> +		vfprintf(f, extra_message, args);
> +		fputs("\n\n", f);
> +		va_end(args);
> +	}
>  
>  	fputs(usage_str, f);
>  }
> @@ -310,12 +316,7 @@ static bool add_regex(struct regex_list *list, char *new)
>  
>  	regex = g_regex_new(new, G_REGEX_OPTIMIZE, 0, &error);
>  	if (error) {
> -		char *buf = malloc(snprintf(NULL, 0, "Invalid regex '%s': %s", new, error->message) + 1);
> -
> -		sprintf(buf, "Invalid regex '%s': %s", new, error->message);
> -		usage(buf, stderr);
> -
> -		free(buf);
> +		usage(stderr, "Invalid regex '%s': %s", new, error->message);
>  		g_error_free(error);
>  		return false;
>  	}
> @@ -549,7 +550,7 @@ bool parse_options(int argc, char **argv,
>  			print_version();
>  			goto error;
>  		case OPT_HELP:
> -			usage(NULL, stdout);
> +			usage(stdout, NULL);
>  			goto error;
>  		case OPT_NAME:
>  			settings->name = strdup(optarg);
> @@ -574,7 +575,7 @@ bool parse_options(int argc, char **argv,
>  			break;
>  		case OPT_DISK_USAGE_LIMIT:
>  			if (!parse_usage_limit(settings, optarg)) {
> -				usage("Cannot parse disk usage limit", stderr);
> +				usage(stderr, "Cannot parse disk usage limit");
>  				goto error;
>  			}
>  			break;
> @@ -583,7 +584,7 @@ bool parse_options(int argc, char **argv,
>  			break;
>  		case OPT_LOG_LEVEL:
>  			if (!set_log_level(settings, optarg)) {
> -				usage("Cannot parse log level", stderr);
> +				usage(stderr, "Cannot parse log level");
>  				goto error;
>  			}
>  			break;
> @@ -631,7 +632,7 @@ bool parse_options(int argc, char **argv,
>  			break;
>  		case OPT_PRUNE_MODE:
>  			if (!set_prune_mode(settings, optarg)) {
> -				usage("Cannot parse prune mode", stderr);
> +				usage(stderr, "Cannot parse prune mode");
>  				goto error;
>  			}
>  			break;
> @@ -644,10 +645,10 @@ bool parse_options(int argc, char **argv,
>  			settings->list_all = true;
>  			break;
>  		case '?':
> -			usage(NULL, stderr);
> +			usage(stderr, NULL);
>  			goto error;
>  		default:
> -			usage("Cannot parse options", stderr);
> +			usage(stderr, "Cannot parse options");
>  			goto error;
>  		}
>  	}
> @@ -664,7 +665,7 @@ bool parse_options(int argc, char **argv,
>  		case 0:
>  			break;
>  		default:
> -			usage("Too many arguments for --list-all", stderr);
> +			usage(stderr, "Too many arguments for --list-all");
>  			goto error;
>  		}
>  	} else {
> @@ -677,10 +678,10 @@ bool parse_options(int argc, char **argv,
>  			settings->results_path = absolute_path(argv[optind]);
>  			break;
>  		case 0:
> -			usage("Results-path missing", stderr);
> +			usage(stderr, "Results-path missing");
>  			goto error;
>  		default:
> -			usage("Extra arguments after results-path", stderr);
> +			usage(stderr, "Extra arguments after results-path");
>  			goto error;
>  		}
>  		if (!settings->name) {
> @@ -697,7 +698,7 @@ bool parse_options(int argc, char **argv,
>  	}
>  
>  	if (!settings->test_root) {
> -		usage("Test root not set", stderr);
> +		usage(stderr, "Test root not set");
>  		goto error;
>  	}
>  
> @@ -714,17 +715,17 @@ bool validate_settings(struct settings *settings)
>  	int dirfd, fd;
>  
>  	if (settings->test_list && !readable_file(settings->test_list)) {
> -		usage("Cannot open test-list file", stderr);
> +		usage(stderr, "Cannot open test-list file");
>  		return false;
>  	}
>  
>  	if (!settings->results_path) {
> -		usage("No results-path set; this shouldn't happen", stderr);
> +		usage(stderr, "No results-path set; this shouldn't happen");
>  		return false;
>  	}
>  
>  	if (!settings->test_root) {
> -		usage("No test root set; this shouldn't happen", stderr);
> +		usage(stderr, "No test root set; this shouldn't happen");
>  		return false;
>  	}
>  
> @@ -780,23 +781,24 @@ bool serialize_settings(struct settings *settings)
>  	FILE *f;
>  
>  	if (!settings->results_path) {
> -		usage("No results-path set; this shouldn't happen", stderr);
> +		usage(stderr, "No results-path set; this shouldn't happen");
>  		return false;
>  	}
>  
>  	if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
>  		mkdir(settings->results_path, 0755);
>  		if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
> -			usage("Creating results-path failed", stderr);
> +			usage(stderr, "Creating results-path failed");
>  			return false;
>  		}
>  	}
> +
>  	if (settings->enable_code_coverage) {
>  		strcpy(path, settings->results_path);
>  		strcat(path, CODE_COV_RESULTS_PATH);
>  		if ((covfd = open(path, O_DIRECTORY | O_RDONLY)) < 0) {
>  			if (mkdir(path, 0755)) {
> -				usage("Creating code coverage path failed", stderr);
> +				usage(stderr, "Creating code coverage path failed");
>  				return false;
>  			}
>  		} else {
> @@ -806,24 +808,19 @@ bool serialize_settings(struct settings *settings)
>  
>  	if (!settings->overwrite &&
>  	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
> -		usage("Settings metadata already exists and not overwriting", stderr);
> +		usage(stderr, "Settings metadata already exists and not overwriting");
>  		return false;
>  	}
>  
>  	if (settings->overwrite &&
>  	    unlinkat(dirfd, settings_filename, 0) != 0 &&
>  	    errno != ENOENT) {
> -		usage("Error removing old settings metadata", stderr);
> +		usage(stderr, "Error removing old settings metadata");
>  		return false;
>  	}
>  
>  	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
> -		char *msg;
> -
> -		asprintf(&msg, "Creating settings serialization file failed: %s", strerror(errno));
> -		usage(msg, stderr);
> -
> -		free(msg);
> +		usage(msg, "Creating settings serialization file failed: %s", strerror(errno));
>  		close(dirfd);
>  		return false;
>  	}

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 3/7] igt_list: Add the igt_list_empty_or_null helper
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 3/7] igt_list: Add the igt_list_empty_or_null helper Ryszard Knop
@ 2022-06-29 16:00   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-06-29 16:00 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, 28 Jun 2022 11:44:31 +0200
Ryszard Knop <ryszard.knop@intel.com> wrote:

> Checks if the list is empty or uninitialized.

Looking at patch 4/7, I understand why you want this function:

	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 (igt_list_empty_or_null(env_vars))
			IGT_INIT_LIST_HEAD(env_vars);

Yet, I don't like the idea of conditionally initializing env_vars.

IMO, the best would be to always call IGT_INIT_LIST_HEAD(env_vars),
dropping the conditional check at add_env_var().

Regards,
Mauro

> 
> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> ---
>  lib/igt_list.c | 5 +++++
>  lib/igt_list.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/lib/igt_list.c b/lib/igt_list.c
> index 37ae139c..dc94fad8 100644
> --- a/lib/igt_list.c
> +++ b/lib/igt_list.c
> @@ -81,3 +81,8 @@ bool igt_list_empty(const struct igt_list_head *head)
>  {
>  	return head->next == head;
>  }
> +
> +bool igt_list_empty_or_null(const struct igt_list_head *head)
> +{
> +	return head->next == NULL || head->prev == NULL || igt_list_empty(head);
> +}
> diff --git a/lib/igt_list.h b/lib/igt_list.h
> index be63fd80..4efb7a22 100644
> --- a/lib/igt_list.h
> +++ b/lib/igt_list.h
> @@ -80,6 +80,7 @@ void igt_list_move(struct igt_list_head *elem, struct igt_list_head *list);
>  void igt_list_move_tail(struct igt_list_head *elem, struct igt_list_head *list);
>  int igt_list_length(const struct igt_list_head *head);
>  bool igt_list_empty(const struct igt_list_head *head);
> +bool igt_list_empty_or_null(const struct igt_list_head *head);
>  
>  #define igt_container_of(ptr, sample, member)				\
>  	(__typeof__(sample))((char *)(ptr) -				\

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag Ryszard Knop
@ 2022-06-29 16:13   ` Mauro Carvalho Chehab
       [not found]     ` <b1e8327b02fbbda0e180b217735b460c6f307f6b.camel@intel.com>
  0 siblings, 1 reply; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-06-29 16:13 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, 28 Jun 2022 11:44:32 +0200
Ryszard Knop <ryszard.knop@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@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"

> +
> +	if (igt_list_empty_or_null(env_vars))
> +		IGT_INIT_LIST_HEAD(env_vars);
> +
> +	igt_list_add_tail(var, env_vars);
> +
> +	free(env_kv);
> +	return true;
> +
> +error:
> +	if (var != NULL)
> +		free(var);
> +
> +	if (env_kv != NULL)
> +		free(env_kv);
> +
> +	return false;
> +}
> +
> +static void free_env_vars(struct igt_list_head *env_vars) {
> +	struct environment_variable *iter;
> +
> +	while (!igt_list_empty_or_null(env_vars)) {
> +		iter = igt_list_first_entry(env_vars, iter, link);
> +
> +		free(iter->key);
> +		free(iter->value);
> +
> +		igt_list_del(iter);
> +		free(iter);
> +	}
> +}
> +
>  static bool readable_file(const char *filename)
>  {
>  	return !access(filename, R_OK);
> @@ -496,6 +604,7 @@ void clear_settings(struct settings *settings)
>  
>  	free_regexes(&settings->include_regexes);
>  	free_regexes(&settings->exclude_regexes);
> +	free_env_vars(&settings->env_vars);
>  
>  	init_settings(settings);
>  }
> @@ -514,6 +623,7 @@ bool parse_options(int argc, char **argv,
>  		{"allow-non-root", no_argument, NULL, OPT_ALLOW_NON_ROOT},
>  		{"include-tests", required_argument, NULL, OPT_INCLUDE},
>  		{"exclude-tests", required_argument, NULL, OPT_EXCLUDE},
> +		{"environment", required_argument, NULL, OPT_ENVIRONMENT},
>  		{"abort-on-monitored-error", optional_argument, NULL, OPT_ABORT_ON_ERROR},
>  		{"disk-usage-limit", required_argument, NULL, OPT_DISK_USAGE_LIMIT},
>  		{"sync", no_argument, NULL, OPT_SYNC},
> @@ -543,7 +653,7 @@ bool parse_options(int argc, char **argv,
>  
>  	settings->dmesg_warn_level = -1;
>  
> -	while ((c = getopt_long(argc, argv, "hn:dt:x:sl:omb:L",
> +	while ((c = getopt_long(argc, argv, "hn:dt:x:e:sl:omb:L",
>  				long_options, NULL)) != -1) {
>  		switch (c) {
>  		case OPT_VERSION:
> @@ -569,6 +679,10 @@ bool parse_options(int argc, char **argv,
>  			if (!add_regex(&settings->exclude_regexes, strdup(optarg)))
>  				goto error;
>  			break;
> +		case OPT_ENVIRONMENT:
> +			if (!add_env_var(&settings->env_vars, optarg))
> +				goto error;
> +			break;
>  		case OPT_ABORT_ON_ERROR:
>  			if (!parse_abort_conditions(settings, optarg))
>  				goto error;
> diff --git a/runner/settings.h b/runner/settings.h
> index 6d444d01..819c3460 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -7,6 +7,8 @@
>  #include <stdio.h>
>  #include <glib.h>
>  
> +#include "igt_list.h"
> +
>  enum {
>  	LOG_LEVEL_NORMAL = 0,
>  	LOG_LEVEL_QUIET = -1,
> @@ -37,6 +39,12 @@ struct regex_list {
>  	size_t size;
>  };
>  
> +struct environment_variable {
> +	struct igt_list_head link;
> +	char * key;
> +	char * value;
> +};
> +
>  struct settings {
>  	int abort_mask;
>  	size_t disk_usage_limit;
> @@ -46,6 +54,7 @@ struct settings {
>  	bool allow_non_root;
>  	struct regex_list include_regexes;
>  	struct regex_list exclude_regexes;
> +	struct igt_list_head env_vars;
>  	bool sync;
>  	int log_level;
>  	bool overwrite;

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 6/7] runner: Set requested env vars during execution
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 6/7] runner: Set requested env vars during execution Ryszard Knop
@ 2022-06-29 16:28   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-06-29 16:28 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, 28 Jun 2022 11:44:34 +0200
Ryszard Knop <ryszard.knop@intel.com> wrote:

> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> ---
>  runner/executor.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/runner/executor.c b/runner/executor.c
> index 9b89cc09..21c41715 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -1872,6 +1872,14 @@ bool execute(struct execute_state *state,
>  		return true;
>  	}
>  
> +	if (!igt_list_empty_or_null(&settings->env_vars)) {

Just always initialize the list. This would avoid tests like this.

> +		struct environment_variable *iter;
> +
> +		igt_list_for_each_entry(iter, &settings->env_vars, link) {
> +			setenv(iter->key, iter->value, 1);
> +		}
> +	}
> +

The rest looks ok on my eyes.

>  	if ((resdirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
>  		/* Initialize state should have done this */
>  		errf("Error: Failure opening results path %s\n",

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 7/7] runner: Add help for the --environment flag
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 7/7] runner: Add help for the --environment flag Ryszard Knop
@ 2022-06-29 16:30   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-06-29 16:30 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, 28 Jun 2022 11:44:35 +0200
Ryszard Knop <ryszard.knop@intel.com> wrote:

> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> ---
>  runner/settings.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/runner/settings.c b/runner/settings.c
> index 1926e197..383e641e 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -286,6 +286,10 @@ static const char *usage_str =
>  	"  -b, --blacklist FILENAME\n"
>  	"                        Exclude all test matching to regexes from FILENAME\n"
>  	"                        (can be used more than once)\n"
> +	"  -e, --environment <KEY or KEY=VALUE>\n"
> +	"                        Set an environment variable for the test process.\n"
> +	"                        If only the key is provided, the current value is read\n"
> +	"                        from the runner's environment (and saved for resumes).\n"
>  	"  -L, --list-all        List all matching subtests instead of running\n"
>  	"  --collect-code-cov    Enables gcov-based collect of code coverage for tests.\n"
>  	"                        Requires --collect-script FILENAME\n"

Looks good to me, but I would also add support for:

		-e 'VAR1=foo, VAR2=bar"

(see my previous comments)

Besides adding help, you also need to place unit tests in order to
check that the new command line parameter is working fine.
See runner/runner_tests.c.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags Ryszard Knop
@ 2022-06-29 16:36   ` Mauro Carvalho Chehab
       [not found]     ` <6622adedd2931cbd6d4e0c3eb46a2d3f72343351.camel@intel.com>
  0 siblings, 1 reply; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-06-29 16:36 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, 28 Jun 2022 11:44:33 +0200
Ryszard Knop <ryszard.knop@intel.com> wrote:

> Settings serialization now creates a separate file, environment.txt,
> which contains KEY=VALUE pairs of environment variables to use for test
> process spawning. Only vars created with --environment are serialized.
> Also add misc warning fixes.
> 
> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> ---
>  runner/settings.c | 206 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 169 insertions(+), 37 deletions(-)
> 
> diff --git a/runner/settings.c b/runner/settings.c
> index de339484..1926e197 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -79,6 +79,9 @@ static struct {
>  	{ 0, 0 },
>  };
>  
> +static char settings_filename[] = "metadata.txt";
> +static char env_filename[] = "environment.txt";
> +

are the above const? If so, add such attribute.

>  static bool set_log_level(struct settings* settings, const char *level)
>  {
>  	typeof(*log_levels) *it;
> @@ -296,6 +299,7 @@ static const char *usage_str =
>  	"                        this option if given.\n"
>  	;
>  
> +__attribute__ ((format(printf, 2, 3)))
>  static void usage(FILE *f, const char *extra_message, ...)
>  {
>  	if (extra_message) {
> @@ -448,7 +452,7 @@ static bool add_env_var(struct igt_list_head *env_vars, char *env_kv) {
>  	var = malloc(sizeof(struct environment_variable));
>  	if (kv_split != NULL) {
>  		/* value provided - copy string contents after '=' */
> -		kv_split[0] = NULL;
> +		kv_split[0] = '\0';

Why is this here? Should be at the patch that added it.

>  
>  		var->key = string_trim_and_duplicate(env_kv);
>  		var->value = strdup(kv_split + 1); /* Can be empty, that's okay */
> @@ -468,7 +472,7 @@ static bool add_env_var(struct igt_list_head *env_vars, char *env_kv) {
>  	if (igt_list_empty_or_null(env_vars))
>  		IGT_INIT_LIST_HEAD(env_vars);
>  
> -	igt_list_add_tail(var, env_vars);
> +	igt_list_add_tail(&var->link, env_vars);
>  
>  	free(env_kv);
>  	return true;
> @@ -492,11 +496,16 @@ static void free_env_vars(struct igt_list_head *env_vars) {
>  		free(iter->key);
>  		free(iter->value);
>  
> -		igt_list_del(iter);
> +		igt_list_del(&iter->link);
>  		free(iter);
>  	}
>  }
>  
> +static bool file_exists_at(int dirfd, const char *filename)
> +{
> +	return faccessat(dirfd, filename, F_OK, 0) == 0;
> +}
> +
>  static bool readable_file(const char *filename)
>  {
>  	return !access(filename, R_OK);
> @@ -885,14 +894,78 @@ bool validate_settings(struct settings *settings)
>  	return true;
>  }
>  
> -static char settings_filename[] = "metadata.txt";
> +/**
> + * fopenat() - wrapper for fdopen(openat(dirfd, filename))
> + * @dirfd: directory fd to pass to openat
> + * @filename: file name to open with openat
> + * @flags: flags to pass to openat
> + * @openat_mode: (octal) mode to pass to openat
> + * @fdopen_mode: (text) mode to pass to fdopen
> + */
> +static FILE *fopenat(int dirfd, char *filename, int flags, mode_t openat_mode, const char *fdopen_mode)
> +{
> +	int fd;
> +	FILE *f;
> +
> +	if ((fd = openat(dirfd, filename, flags, openat_mode)) < 0) {
> +		usage(stderr, "fopenat(%s, %d) failed: %s",
> +		      filename, flags, strerror(errno));
> +		return NULL;
> +	}
> +
> +	f = fdopen(fd, fdopen_mode);
> +	if (!f) {
> +		close(fd);
> +		return NULL;
> +	}
> +
> +	return f;
> +}
> +
> +static FILE *fopenat_read(int dirfd, char *filename)
> +{
> +	return fopenat(dirfd, filename, O_RDONLY, 0000, "r");
> +}
> +
> +static FILE *fopenat_create(int dirfd, char *filename, bool overwrite)
> +{
> +	mode_t mode_if_exists = overwrite ? O_TRUNC : O_EXCL;
> +	return fopenat(dirfd, filename, O_CREAT | O_RDWR | mode_if_exists, 0666, "w");
> +}
> +
> +static bool serialize_environment(struct settings *settings, int dirfd)
> +{
> +	struct environment_variable *iter;
> +	FILE *f;
> +
> +	if (file_exists_at(dirfd, env_filename) && !settings->overwrite) {
> +		usage(stderr, "%s already exists, not overwriting", env_filename);
> +		return false;
> +	}
> +
> +	if ((f = fopenat_create(dirfd, env_filename, settings->overwrite)) == NULL)
> +		return false;
> +
> +	igt_list_for_each_entry(iter, &settings->env_vars, link) {
> +		fprintf(f, "%s=%s\n", iter->key, iter->value);
> +	}
> +
> +	if (settings->sync) {
> +		fflush(f);
> +		fsync(fileno(f));
> +	}
> +
> +	fclose(f);
> +	return true;
> +}
> +
>  bool serialize_settings(struct settings *settings)
>  {
>  #define SERIALIZE_LINE(f, s, name, format) fprintf(f, "%s : " format "\n", #name, s->name)
>  
> -	int dirfd, covfd, fd;
> -	char path[PATH_MAX];
>  	FILE *f;
> +	int dirfd, covfd;
> +	char path[PATH_MAX];
>  
>  	if (!settings->results_path) {
>  		usage(stderr, "No results-path set; this shouldn't happen");
> @@ -920,28 +993,13 @@ bool serialize_settings(struct settings *settings)
>  		}
>  	}
>  
> -	if (!settings->overwrite &&
> -	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
> -		usage(stderr, "Settings metadata already exists and not overwriting");
> -		return false;
> -	}
> -
> -	if (settings->overwrite &&
> -	    unlinkat(dirfd, settings_filename, 0) != 0 &&
> -	    errno != ENOENT) {
> -		usage(stderr, "Error removing old settings metadata");
> -		return false;
> -	}
> -
> -	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
> -		usage(msg, "Creating settings serialization file failed: %s", strerror(errno));
> +	if (file_exists_at(dirfd, settings_filename) && !settings->overwrite) {
> +		usage(stderr, "%s already exists, not overwriting", settings_filename);
>  		close(dirfd);
>  		return false;
>  	}
>  
> -	f = fdopen(fd, "w");
> -	if (!f) {
> -		close(fd);
> +	if ((f = fopenat_create(dirfd, settings_filename, settings->overwrite)) == NULL) {
>  		close(dirfd);
>  		return false;
>  	}
> @@ -972,11 +1030,22 @@ bool serialize_settings(struct settings *settings)
>  	SERIALIZE_LINE(f, settings, code_coverage_script, "%s");
>  
>  	if (settings->sync) {
> -		fsync(fd);
> -		fsync(dirfd);
> +		fflush(f);
> +		fsync(fileno(f));
>  	}
>  
>  	fclose(f);
> +
> +	if (!igt_list_empty_or_null(&settings->env_vars)) {

See my past comments.

> +		if (!serialize_environment(settings, dirfd)) {
> +			close(dirfd);
> +			return false;
> +		}
> +	}
> +
> +	if (settings->sync)
> +		fsync(dirfd);
> +
>  	close(dirfd);
>  	return true;
>  
> @@ -1045,28 +1114,91 @@ bool read_settings_from_file(struct settings *settings, FILE *f)
>  #undef PARSE_LINE
>  }
>  
> +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?

> +	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?

> +		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;
>  }

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [i-g-t,2/7] runner: make the usage function variadic
  2022-06-28 23:51   ` [igt-dev] [i-g-t,2/7] " Maíra Canal
@ 2022-06-30 13:16     ` Knop, Ryszard
  0 siblings, 0 replies; 32+ messages in thread
From: Knop, Ryszard @ 2022-06-30 13:16 UTC (permalink / raw)
  To: igt-dev, maira.canal

On Tue, 2022-06-28 at 20:51 -0300, Maíra Canal wrote:
> On 6/28/22 11:44, Ryszard Knop wrote:
> > For easier error logging, make the usage() function variadic and
> > pass
> > its arguments to vfprintf. Additionally, reorder its arguments so
> > that
> > it visually matches all the printf functions.
> > 
> > Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> > ---
> 
> Hi,
> 
> Overall this looks good, just a small typo.
> 
> > @@ -806,24 +808,19 @@ bool serialize_settings(struct settings
> > *settings)
> >  
> >         if (!settings->overwrite &&
> >             faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
> > -               usage("Settings metadata already exists and not
> > overwriting", stderr);
> > +               usage(stderr, "Settings metadata already exists and
> > not overwriting");
> >                 return false;
> >         }
> >  
> >         if (settings->overwrite &&
> >             unlinkat(dirfd, settings_filename, 0) != 0 &&
> >             errno != ENOENT) {
> > -               usage("Error removing old settings metadata",
> > stderr);
> > +               usage(stderr, "Error removing old settings
> > metadata");
> >                 return false;
> >         }
> >  
> >         if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL
> > | O_WRONLY, 0666)) < 0) {
> > -               char *msg;
> > -
> > -               asprintf(&msg, "Creating settings serialization
> > file failed: %s", strerror(errno));
> > -               usage(msg, stderr);
> > -
> > -               free(msg);
> > +               usage(msg, "Creating settings serialization file
> > failed: %s", strerror(errno));
> 
> I believe this should be:
>     usage(stderr, "Creating settings serialization file failed: %s",
> strerror(errno));

That's right, sorry, missed this one while splitting the patch.

Thanks, Ryszard

> 
> Reviewed-by: Maíra Canal <maira.canal@usp.br>
> 
> Best Regards,
> - Maíra Canal
> 
> >                 close(dirfd);
> >                 return false;
> >   }


^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag
       [not found]     ` <b1e8327b02fbbda0e180b217735b460c6f307f6b.camel@intel.com>
@ 2022-07-05 19:58       ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-07-05 19:58 UTC (permalink / raw)
  To: Knop, Ryszard; +Cc: igt-dev

On Thu, 30 Jun 2022 13:40:47 +0000
"Knop, Ryszard" <ryszard.knop@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@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@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

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags
       [not found]     ` <6622adedd2931cbd6d4e0c3eb46a2d3f72343351.camel@intel.com>
@ 2022-07-05 20:12       ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 32+ messages in thread
From: Mauro Carvalho Chehab @ 2022-07-05 20:12 UTC (permalink / raw)
  To: Knop, Ryszard; +Cc: igt-dev

On Thu, 30 Jun 2022 13:57:16 +0000
"Knop, Ryszard" <ryszard.knop@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;
> > >  }  
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic
  2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic Ryszard Knop
  2022-06-28 23:51   ` [igt-dev] [i-g-t,2/7] " Maíra Canal
  2022-06-29 15:51   ` [igt-dev] [PATCH i-g-t 2/7] " Mauro Carvalho Chehab
@ 2022-07-25 11:53   ` Petri Latvala
  2 siblings, 0 replies; 32+ messages in thread
From: Petri Latvala @ 2022-07-25 11:53 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Tue, Jun 28, 2022 at 11:44:30AM +0200, Ryszard Knop wrote:
> For easier error logging, make the usage() function variadic and pass
> its arguments to vfprintf. Additionally, reorder its arguments so that
> it visually matches all the printf functions.
> 
> Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
> ---
>  runner/settings.c | 63 ++++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/runner/settings.c b/runner/settings.c
> index 06947c91..d153954a 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -295,10 +295,16 @@ static const char *usage_str =
>  	"                        this option if given.\n"
>  	;
>  
> -static void usage(const char *extra_message, FILE *f)
> +static void usage(FILE *f, const char *extra_message, ...)

Could use a little bit of __attribute__((format(printf

-- 
Petri Latvala


>  {
> -	if (extra_message)
> -		fprintf(f, "%s\n\n", extra_message);
> +	if (extra_message) {
> +		va_list args;
> +
> +		va_start(args, extra_message);
> +		vfprintf(f, extra_message, args);
> +		fputs("\n\n", f);
> +		va_end(args);
> +	}
>  
>  	fputs(usage_str, f);
>  }
> @@ -310,12 +316,7 @@ static bool add_regex(struct regex_list *list, char *new)
>  
>  	regex = g_regex_new(new, G_REGEX_OPTIMIZE, 0, &error);
>  	if (error) {
> -		char *buf = malloc(snprintf(NULL, 0, "Invalid regex '%s': %s", new, error->message) + 1);
> -
> -		sprintf(buf, "Invalid regex '%s': %s", new, error->message);
> -		usage(buf, stderr);
> -
> -		free(buf);
> +		usage(stderr, "Invalid regex '%s': %s", new, error->message);
>  		g_error_free(error);
>  		return false;
>  	}
> @@ -549,7 +550,7 @@ bool parse_options(int argc, char **argv,
>  			print_version();
>  			goto error;
>  		case OPT_HELP:
> -			usage(NULL, stdout);
> +			usage(stdout, NULL);
>  			goto error;
>  		case OPT_NAME:
>  			settings->name = strdup(optarg);
> @@ -574,7 +575,7 @@ bool parse_options(int argc, char **argv,
>  			break;
>  		case OPT_DISK_USAGE_LIMIT:
>  			if (!parse_usage_limit(settings, optarg)) {
> -				usage("Cannot parse disk usage limit", stderr);
> +				usage(stderr, "Cannot parse disk usage limit");
>  				goto error;
>  			}
>  			break;
> @@ -583,7 +584,7 @@ bool parse_options(int argc, char **argv,
>  			break;
>  		case OPT_LOG_LEVEL:
>  			if (!set_log_level(settings, optarg)) {
> -				usage("Cannot parse log level", stderr);
> +				usage(stderr, "Cannot parse log level");
>  				goto error;
>  			}
>  			break;
> @@ -631,7 +632,7 @@ bool parse_options(int argc, char **argv,
>  			break;
>  		case OPT_PRUNE_MODE:
>  			if (!set_prune_mode(settings, optarg)) {
> -				usage("Cannot parse prune mode", stderr);
> +				usage(stderr, "Cannot parse prune mode");
>  				goto error;
>  			}
>  			break;
> @@ -644,10 +645,10 @@ bool parse_options(int argc, char **argv,
>  			settings->list_all = true;
>  			break;
>  		case '?':
> -			usage(NULL, stderr);
> +			usage(stderr, NULL);
>  			goto error;
>  		default:
> -			usage("Cannot parse options", stderr);
> +			usage(stderr, "Cannot parse options");
>  			goto error;
>  		}
>  	}
> @@ -664,7 +665,7 @@ bool parse_options(int argc, char **argv,
>  		case 0:
>  			break;
>  		default:
> -			usage("Too many arguments for --list-all", stderr);
> +			usage(stderr, "Too many arguments for --list-all");
>  			goto error;
>  		}
>  	} else {
> @@ -677,10 +678,10 @@ bool parse_options(int argc, char **argv,
>  			settings->results_path = absolute_path(argv[optind]);
>  			break;
>  		case 0:
> -			usage("Results-path missing", stderr);
> +			usage(stderr, "Results-path missing");
>  			goto error;
>  		default:
> -			usage("Extra arguments after results-path", stderr);
> +			usage(stderr, "Extra arguments after results-path");
>  			goto error;
>  		}
>  		if (!settings->name) {
> @@ -697,7 +698,7 @@ bool parse_options(int argc, char **argv,
>  	}
>  
>  	if (!settings->test_root) {
> -		usage("Test root not set", stderr);
> +		usage(stderr, "Test root not set");
>  		goto error;
>  	}
>  
> @@ -714,17 +715,17 @@ bool validate_settings(struct settings *settings)
>  	int dirfd, fd;
>  
>  	if (settings->test_list && !readable_file(settings->test_list)) {
> -		usage("Cannot open test-list file", stderr);
> +		usage(stderr, "Cannot open test-list file");
>  		return false;
>  	}
>  
>  	if (!settings->results_path) {
> -		usage("No results-path set; this shouldn't happen", stderr);
> +		usage(stderr, "No results-path set; this shouldn't happen");
>  		return false;
>  	}
>  
>  	if (!settings->test_root) {
> -		usage("No test root set; this shouldn't happen", stderr);
> +		usage(stderr, "No test root set; this shouldn't happen");
>  		return false;
>  	}
>  
> @@ -780,23 +781,24 @@ bool serialize_settings(struct settings *settings)
>  	FILE *f;
>  
>  	if (!settings->results_path) {
> -		usage("No results-path set; this shouldn't happen", stderr);
> +		usage(stderr, "No results-path set; this shouldn't happen");
>  		return false;
>  	}
>  
>  	if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
>  		mkdir(settings->results_path, 0755);
>  		if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
> -			usage("Creating results-path failed", stderr);
> +			usage(stderr, "Creating results-path failed");
>  			return false;
>  		}
>  	}
> +
>  	if (settings->enable_code_coverage) {
>  		strcpy(path, settings->results_path);
>  		strcat(path, CODE_COV_RESULTS_PATH);
>  		if ((covfd = open(path, O_DIRECTORY | O_RDONLY)) < 0) {
>  			if (mkdir(path, 0755)) {
> -				usage("Creating code coverage path failed", stderr);
> +				usage(stderr, "Creating code coverage path failed");
>  				return false;
>  			}
>  		} else {
> @@ -806,24 +808,19 @@ bool serialize_settings(struct settings *settings)
>  
>  	if (!settings->overwrite &&
>  	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
> -		usage("Settings metadata already exists and not overwriting", stderr);
> +		usage(stderr, "Settings metadata already exists and not overwriting");
>  		return false;
>  	}
>  
>  	if (settings->overwrite &&
>  	    unlinkat(dirfd, settings_filename, 0) != 0 &&
>  	    errno != ENOENT) {
> -		usage("Error removing old settings metadata", stderr);
> +		usage(stderr, "Error removing old settings metadata");
>  		return false;
>  	}
>  
>  	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
> -		char *msg;
> -
> -		asprintf(&msg, "Creating settings serialization file failed: %s", strerror(errno));
> -		usage(msg, stderr);
> -
> -		free(msg);
> +		usage(msg, "Creating settings serialization file failed: %s", strerror(errno));
>  		close(dirfd);
>  		return false;
>  	}
> -- 
> 2.36.1
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 0/7] Add the --environment flag
@ 2022-08-08 16:18 ` Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 1/7] runner: rename free_settings to clear_settings Ryszard Knop
                     ` (7 more replies)
  0 siblings, 8 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

It sets the provided KEY=VALUE pair as the environment variable for
executed test processes. If just the KEY is provided, the currently-set
value is reused (and the run fails if it's not set for the runner).

The most important thing about this feature is that all the variables
set this way are saved for resumed runs as well. This lets developers
easily try something out from the terminal without having to remember
to set the same vars/reuse a wrapper to continue a run.

v2:
- [2/7] usage(msg, ...) -> usage(stderr) (Maira)
- [2/7] __attribute__ ((format (...))) (Petri)
- [3/7] Moved warning fixes from later commits here
- [3/7] igt_list_empty_or_null -> igt_list_empty (Mauro)
- [3/7] Optimized the parser, more error handling (Mauro)
- [4/7] Simplify env var reading from files (Mauro)
- [4/7] Don't align settings loading block (Mauro)
- [4/7] Make filenames static const (Mauro)
- [5/7] Removed unnecessary checks, misc cleanup (Mauro)
- [7/7] Added requested tests (Mauro)


^ permalink raw reply	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 1/7] runner: rename free_settings to clear_settings
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
@ 2022-08-08 16:18   ` Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 2/7] runner: make the usage function variadic Ryszard Knop
                     ` (6 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

That function does not actually free the settings pointer - rename it
for clarity.

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/executor.c     |  2 +-
 runner/resultgen.c    |  2 +-
 runner/runner_tests.c | 12 ++++++------
 runner/settings.c     |  8 ++++----
 runner/settings.h     |  6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/runner/executor.c b/runner/executor.c
index 6e6ca9cc..9b89cc09 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1547,7 +1547,7 @@ bool initialize_execute_state_from_resume(int dirfd,
 	struct job_list_entry *entry;
 	int resdirfd, fd, i;
 
-	free_settings(settings);
+	clear_settings(settings);
 	free_job_list(list);
 	memset(state, 0, sizeof(*state));
 	state->resuming = true;
diff --git a/runner/resultgen.c b/runner/resultgen.c
index 6ecf325b..8e11b2b1 100644
--- a/runner/resultgen.c
+++ b/runner/resultgen.c
@@ -1702,7 +1702,7 @@ struct json_object *generate_results_json(int dirfd)
 		close(fd);
 	}
 
-	free_settings(&settings);
+	clear_settings(&settings);
 	free_job_list(&job_list);
 
 	return obj;
diff --git a/runner/runner_tests.c b/runner/runner_tests.c
index 8fe86978..aca031fc 100644
--- a/runner/runner_tests.c
+++ b/runner/runner_tests.c
@@ -139,7 +139,7 @@ static void job_list_filter_test(const char *name, const char *filterarg1, const
 	}
 
 	igt_fixture {
-		free_settings(settings);
+		clear_settings(settings);
 		free(settings);
 	}
 }
@@ -953,7 +953,7 @@ igt_main
 			close(fd);
 			close(dirfd);
 			clear_directory(dirname);
-			free_settings(cmp_settings);
+			clear_settings(cmp_settings);
 			free(cmp_settings);
 		}
 	}
@@ -1176,7 +1176,7 @@ igt_main
 			igt_assert(write(fd, journaltext, strlen(journaltext)) == strlen(journaltext));
 
 			free_job_list(list);
-			free_settings(settings);
+			clear_settings(settings);
 			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
 
 			igt_assert_eq(state.next, 0);
@@ -1233,7 +1233,7 @@ igt_main
 			igt_assert(write(fd, journaltext, strlen(journaltext)) == strlen(journaltext));
 
 			free_job_list(list);
-			free_settings(settings);
+			clear_settings(settings);
 			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
 
 			/* All subtests are in journal, the entry should be considered completed */
@@ -1294,7 +1294,7 @@ igt_main
 			igt_assert_eq(write(fd, journaltext, sizeof(journaltext)), sizeof(journaltext));
 
 			free_job_list(list);
-			free_settings(settings);
+			clear_settings(settings);
 			igt_assert(initialize_execute_state_from_resume(dirfd, &state, settings, list));
 
 			igt_assert_eq(state.next, 1);
@@ -1942,7 +1942,7 @@ igt_main
 	}
 
 	igt_fixture {
-		free_settings(settings);
+		clear_settings(settings);
 		free(settings);
 	}
 }
diff --git a/runner/settings.c b/runner/settings.c
index bb63a9f4..06947c91 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -486,7 +486,7 @@ void init_settings(struct settings *settings)
 	memset(settings, 0, sizeof(*settings));
 }
 
-void free_settings(struct settings *settings)
+void clear_settings(struct settings *settings)
 {
 	free(settings->test_list);
 	free(settings->name);
@@ -536,7 +536,7 @@ bool parse_options(int argc, char **argv,
 		{ 0, 0, 0, 0},
 	};
 
-	free_settings(settings);
+	clear_settings(settings);
 
 	optind = 1;
 
@@ -705,7 +705,7 @@ bool parse_options(int argc, char **argv,
 	return true;
 
  error:
-	free_settings(settings);
+	clear_settings(settings);
 	return false;
 }
 
@@ -939,7 +939,7 @@ bool read_settings_from_dir(struct settings *settings, int dirfd)
 	int fd;
 	FILE *f;
 
-	free_settings(settings);
+	clear_settings(settings);
 
 	if ((fd = openat(dirfd, settings_filename, O_RDONLY)) < 0)
 		return false;
diff --git a/runner/settings.h b/runner/settings.h
index 6ae64695..6d444d01 100644
--- a/runner/settings.h
+++ b/runner/settings.h
@@ -76,14 +76,14 @@ struct settings {
 void init_settings(struct settings *settings);
 
 /**
- * free_settings:
+ * clear_settings:
  *
  * Releases all allocated resources for a settings object and
  * initializes it to an empty state (see #init_settings).
  *
- * @settings: Object to release and initialize.
+ * @settings: Object to release and reinitialize.
  */
-void free_settings(struct settings *settings);
+void clear_settings(struct settings *settings);
 
 /**
  * parse_options:
-- 
2.37.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 2/7] runner: make the usage function variadic
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 1/7] runner: rename free_settings to clear_settings Ryszard Knop
@ 2022-08-08 16:18   ` Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 3/7] runner: Add the --environment flag Ryszard Knop
                     ` (5 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

For easier error logging, make the usage() function variadic and pass
its arguments to vfprintf. Additionally, reorder its arguments so that
it visually matches all the printf functions.

v2:
- usage(msg, ...) -> usage(stderr) (Maira)
- __attribute__ ((format (...))) (Petri)

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/settings.c | 64 +++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 33 deletions(-)

diff --git a/runner/settings.c b/runner/settings.c
index 06947c91..ba288043 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -295,10 +295,17 @@ static const char *usage_str =
 	"                        this option if given.\n"
 	;
 
-static void usage(const char *extra_message, FILE *f)
+__attribute__ ((format (printf, 2, 3)))
+static void usage(FILE *f, const char *extra_message, ...)
 {
-	if (extra_message)
-		fprintf(f, "%s\n\n", extra_message);
+	if (extra_message) {
+		va_list args;
+
+		va_start(args, extra_message);
+		vfprintf(f, extra_message, args);
+		fputs("\n\n", f);
+		va_end(args);
+	}
 
 	fputs(usage_str, f);
 }
@@ -310,12 +317,7 @@ static bool add_regex(struct regex_list *list, char *new)
 
 	regex = g_regex_new(new, G_REGEX_OPTIMIZE, 0, &error);
 	if (error) {
-		char *buf = malloc(snprintf(NULL, 0, "Invalid regex '%s': %s", new, error->message) + 1);
-
-		sprintf(buf, "Invalid regex '%s': %s", new, error->message);
-		usage(buf, stderr);
-
-		free(buf);
+		usage(stderr, "Invalid regex '%s': %s", new, error->message);
 		g_error_free(error);
 		return false;
 	}
@@ -549,7 +551,7 @@ bool parse_options(int argc, char **argv,
 			print_version();
 			goto error;
 		case OPT_HELP:
-			usage(NULL, stdout);
+			usage(stdout, NULL);
 			goto error;
 		case OPT_NAME:
 			settings->name = strdup(optarg);
@@ -574,7 +576,7 @@ bool parse_options(int argc, char **argv,
 			break;
 		case OPT_DISK_USAGE_LIMIT:
 			if (!parse_usage_limit(settings, optarg)) {
-				usage("Cannot parse disk usage limit", stderr);
+				usage(stderr, "Cannot parse disk usage limit");
 				goto error;
 			}
 			break;
@@ -583,7 +585,7 @@ bool parse_options(int argc, char **argv,
 			break;
 		case OPT_LOG_LEVEL:
 			if (!set_log_level(settings, optarg)) {
-				usage("Cannot parse log level", stderr);
+				usage(stderr, "Cannot parse log level");
 				goto error;
 			}
 			break;
@@ -631,7 +633,7 @@ bool parse_options(int argc, char **argv,
 			break;
 		case OPT_PRUNE_MODE:
 			if (!set_prune_mode(settings, optarg)) {
-				usage("Cannot parse prune mode", stderr);
+				usage(stderr, "Cannot parse prune mode");
 				goto error;
 			}
 			break;
@@ -644,10 +646,10 @@ bool parse_options(int argc, char **argv,
 			settings->list_all = true;
 			break;
 		case '?':
-			usage(NULL, stderr);
+			usage(stderr, NULL);
 			goto error;
 		default:
-			usage("Cannot parse options", stderr);
+			usage(stderr, "Cannot parse options");
 			goto error;
 		}
 	}
@@ -664,7 +666,7 @@ bool parse_options(int argc, char **argv,
 		case 0:
 			break;
 		default:
-			usage("Too many arguments for --list-all", stderr);
+			usage(stderr, "Too many arguments for --list-all");
 			goto error;
 		}
 	} else {
@@ -677,10 +679,10 @@ bool parse_options(int argc, char **argv,
 			settings->results_path = absolute_path(argv[optind]);
 			break;
 		case 0:
-			usage("Results-path missing", stderr);
+			usage(stderr, "Results-path missing");
 			goto error;
 		default:
-			usage("Extra arguments after results-path", stderr);
+			usage(stderr, "Extra arguments after results-path");
 			goto error;
 		}
 		if (!settings->name) {
@@ -697,7 +699,7 @@ bool parse_options(int argc, char **argv,
 	}
 
 	if (!settings->test_root) {
-		usage("Test root not set", stderr);
+		usage(stderr, "Test root not set");
 		goto error;
 	}
 
@@ -714,17 +716,17 @@ bool validate_settings(struct settings *settings)
 	int dirfd, fd;
 
 	if (settings->test_list && !readable_file(settings->test_list)) {
-		usage("Cannot open test-list file", stderr);
+		usage(stderr, "Cannot open test-list file");
 		return false;
 	}
 
 	if (!settings->results_path) {
-		usage("No results-path set; this shouldn't happen", stderr);
+		usage(stderr, "No results-path set; this shouldn't happen");
 		return false;
 	}
 
 	if (!settings->test_root) {
-		usage("No test root set; this shouldn't happen", stderr);
+		usage(stderr, "No test root set; this shouldn't happen");
 		return false;
 	}
 
@@ -780,23 +782,24 @@ bool serialize_settings(struct settings *settings)
 	FILE *f;
 
 	if (!settings->results_path) {
-		usage("No results-path set; this shouldn't happen", stderr);
+		usage(stderr, "No results-path set; this shouldn't happen");
 		return false;
 	}
 
 	if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
 		mkdir(settings->results_path, 0755);
 		if ((dirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
-			usage("Creating results-path failed", stderr);
+			usage(stderr, "Creating results-path failed");
 			return false;
 		}
 	}
+
 	if (settings->enable_code_coverage) {
 		strcpy(path, settings->results_path);
 		strcat(path, CODE_COV_RESULTS_PATH);
 		if ((covfd = open(path, O_DIRECTORY | O_RDONLY)) < 0) {
 			if (mkdir(path, 0755)) {
-				usage("Creating code coverage path failed", stderr);
+				usage(stderr, "Creating code coverage path failed");
 				return false;
 			}
 		} else {
@@ -806,24 +809,19 @@ bool serialize_settings(struct settings *settings)
 
 	if (!settings->overwrite &&
 	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
-		usage("Settings metadata already exists and not overwriting", stderr);
+		usage(stderr, "Settings metadata already exists and not overwriting");
 		return false;
 	}
 
 	if (settings->overwrite &&
 	    unlinkat(dirfd, settings_filename, 0) != 0 &&
 	    errno != ENOENT) {
-		usage("Error removing old settings metadata", stderr);
+		usage(stderr, "Error removing old settings metadata");
 		return false;
 	}
 
 	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
-		char *msg;
-
-		asprintf(&msg, "Creating settings serialization file failed: %s", strerror(errno));
-		usage(msg, stderr);
-
-		free(msg);
+		usage(stderr, "Creating settings serialization file failed: %s", strerror(errno));
 		close(dirfd);
 		return false;
 	}
-- 
2.37.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 3/7] runner: Add the --environment flag
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 1/7] runner: rename free_settings to clear_settings Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 2/7] runner: make the usage function variadic Ryszard Knop
@ 2022-08-08 16:18   ` Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 4/7] runner: Serialize the provided environment flags Ryszard Knop
                     ` (4 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Allows specifying additional environment variables to be set for the
launched test processes. This commit introduces the argument parsing.

v2:
- Moved warning fixes from later commits here
- igt_list_empty_or_null -> igt_list_empty (Mauro)
- Optimized the parser, more error handling (Mauro)

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/settings.c | 123 +++++++++++++++++++++++++++++++++++++++++++++-
 runner/settings.h |   9 ++++
 2 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/runner/settings.c b/runner/settings.c
index ba288043..d0f642a4 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',
@@ -390,6 +391,119 @@ 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.
+ * @key_value: Environment variable key-value pair string to add.
+ *
+ * key_value 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 key_value of "KEY=" or setting
+ * an empty variable for the runner process, then providing just the "KEY".
+ */
+static bool add_env_var(struct igt_list_head *env_vars, const char *key_value) {
+	char *env_kv, *value_str;
+	struct environment_variable *var = NULL;
+
+	if (env_vars == NULL || key_value == NULL || strlen(key_value) == 0)
+		goto error;
+
+	env_kv = strdup(key_value);
+	value_str = strpbrk(env_kv, "\n=");
+
+	if (value_str == env_kv) {
+		fprintf(stderr, "Missing key for --environment \"%s\"\n", key_value);
+		goto error;
+	}
+
+	if (value_str != NULL && value_str[0] != '=') {
+		fprintf(stderr, "Invalid characters in key for --environment \"%s\"\n", key_value);
+		goto error;
+	}
+
+	if (value_str != NULL) {
+		/* value provided - copy string contents after '=' */
+		value_str[0] = '\0';
+		value_str++;
+	} else {
+		/* env_kv is the key - use the runner's environment, if set */
+		value_str = getenv(env_kv);
+		if (value_str == NULL) {
+			fprintf(stderr, "No value provided for --environment \"%s\" and "
+			                "variable is not set for igt_runner\n", env_kv);
+			goto error;
+		}
+	}
+
+	var = malloc(sizeof(struct environment_variable));
+
+	var->key = string_trim_and_duplicate(env_kv);
+	if (strlen(var->key) == 0) {
+		fprintf(stderr, "Environment variable key is empty for \"%s\"\n", key_value);
+		goto error;
+	}
+
+	var->value = strdup(value_str); /* Can be empty, that's okay */
+
+	igt_list_add_tail(&var->link, env_vars);
+	free(env_kv);
+	return true;
+
+error:
+	if (var != NULL)
+		free(var);
+
+	if (env_kv != NULL)
+		free(env_kv);
+
+	return false;
+}
+
+static void free_env_vars(struct igt_list_head *env_vars) {
+	struct environment_variable *iter;
+
+	while (!igt_list_empty(env_vars)) {
+		iter = igt_list_first_entry(env_vars, iter, link);
+
+		free(iter->key);
+		free(iter->value);
+
+		igt_list_del(&iter->link);
+		free(iter);
+	}
+}
+
 static bool readable_file(const char *filename)
 {
 	return !access(filename, R_OK);
@@ -486,6 +600,7 @@ static void print_version(void)
 void init_settings(struct settings *settings)
 {
 	memset(settings, 0, sizeof(*settings));
+	IGT_INIT_LIST_HEAD(&settings->env_vars);
 }
 
 void clear_settings(struct settings *settings)
@@ -497,6 +612,7 @@ void clear_settings(struct settings *settings)
 
 	free_regexes(&settings->include_regexes);
 	free_regexes(&settings->exclude_regexes);
+	free_env_vars(&settings->env_vars);
 
 	init_settings(settings);
 }
@@ -515,6 +631,7 @@ bool parse_options(int argc, char **argv,
 		{"allow-non-root", no_argument, NULL, OPT_ALLOW_NON_ROOT},
 		{"include-tests", required_argument, NULL, OPT_INCLUDE},
 		{"exclude-tests", required_argument, NULL, OPT_EXCLUDE},
+		{"environment", required_argument, NULL, OPT_ENVIRONMENT},
 		{"abort-on-monitored-error", optional_argument, NULL, OPT_ABORT_ON_ERROR},
 		{"disk-usage-limit", required_argument, NULL, OPT_DISK_USAGE_LIMIT},
 		{"sync", no_argument, NULL, OPT_SYNC},
@@ -544,7 +661,7 @@ bool parse_options(int argc, char **argv,
 
 	settings->dmesg_warn_level = -1;
 
-	while ((c = getopt_long(argc, argv, "hn:dt:x:sl:omb:L",
+	while ((c = getopt_long(argc, argv, "hn:dt:x:e:sl:omb:L",
 				long_options, NULL)) != -1) {
 		switch (c) {
 		case OPT_VERSION:
@@ -570,6 +687,10 @@ bool parse_options(int argc, char **argv,
 			if (!add_regex(&settings->exclude_regexes, strdup(optarg)))
 				goto error;
 			break;
+		case OPT_ENVIRONMENT:
+			if (!add_env_var(&settings->env_vars, optarg))
+				goto error;
+			break;
 		case OPT_ABORT_ON_ERROR:
 			if (!parse_abort_conditions(settings, optarg))
 				goto error;
diff --git a/runner/settings.h b/runner/settings.h
index 6d444d01..819c3460 100644
--- a/runner/settings.h
+++ b/runner/settings.h
@@ -7,6 +7,8 @@
 #include <stdio.h>
 #include <glib.h>
 
+#include "igt_list.h"
+
 enum {
 	LOG_LEVEL_NORMAL = 0,
 	LOG_LEVEL_QUIET = -1,
@@ -37,6 +39,12 @@ struct regex_list {
 	size_t size;
 };
 
+struct environment_variable {
+	struct igt_list_head link;
+	char * key;
+	char * value;
+};
+
 struct settings {
 	int abort_mask;
 	size_t disk_usage_limit;
@@ -46,6 +54,7 @@ struct settings {
 	bool allow_non_root;
 	struct regex_list include_regexes;
 	struct regex_list exclude_regexes;
+	struct igt_list_head env_vars;
 	bool sync;
 	int log_level;
 	bool overwrite;
-- 
2.37.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 4/7] runner: Serialize the provided environment flags
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
                     ` (2 preceding siblings ...)
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 3/7] runner: Add the --environment flag Ryszard Knop
@ 2022-08-08 16:18   ` Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 5/7] runner: Set requested env vars during execution Ryszard Knop
                     ` (3 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Settings serialization now creates a separate file, environment.txt,
which contains KEY=VALUE pairs of environment variables to use for test
process spawning. Only vars created with --environment are serialized.
Also add misc warning fixes.

v2:
- Simplify env var reading from files (Mauro)
- Don't align settings loading block (Mauro)
- Make filenames static const (Mauro)

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/settings.c | 183 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 152 insertions(+), 31 deletions(-)

diff --git a/runner/settings.c b/runner/settings.c
index d0f642a4..b57ab080 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -79,6 +79,9 @@ static struct {
 	{ 0, 0 },
 };
 
+static const char settings_filename[] = "metadata.txt";
+static const char env_filename[] = "environment.txt";
+
 static bool set_log_level(struct settings* settings, const char *level)
 {
 	typeof(*log_levels) *it;
@@ -504,6 +507,11 @@ static void free_env_vars(struct igt_list_head *env_vars) {
 	}
 }
 
+static bool file_exists_at(int dirfd, const char *filename)
+{
+	return faccessat(dirfd, filename, F_OK, 0) == 0;
+}
+
 static bool readable_file(const char *filename)
 {
 	return !access(filename, R_OK);
@@ -893,14 +901,78 @@ bool validate_settings(struct settings *settings)
 	return true;
 }
 
-static char settings_filename[] = "metadata.txt";
+/**
+ * fopenat() - wrapper for fdopen(openat(dirfd, filename))
+ * @dirfd: directory fd to pass to openat
+ * @filename: file name to open with openat
+ * @flags: flags to pass to openat
+ * @openat_mode: (octal) mode to pass to openat
+ * @fdopen_mode: (text) mode to pass to fdopen
+ */
+static FILE *fopenat(int dirfd, const char *filename, int flags, mode_t openat_mode, const char *fdopen_mode)
+{
+	int fd;
+	FILE *f;
+
+	if ((fd = openat(dirfd, filename, flags, openat_mode)) < 0) {
+		usage(stderr, "fopenat(%s, %d) failed: %s",
+		      filename, flags, strerror(errno));
+		return NULL;
+	}
+
+	f = fdopen(fd, fdopen_mode);
+	if (!f) {
+		close(fd);
+		return NULL;
+	}
+
+	return f;
+}
+
+static FILE *fopenat_read(int dirfd, const char *filename)
+{
+	return fopenat(dirfd, filename, O_RDONLY, 0000, "r");
+}
+
+static FILE *fopenat_create(int dirfd, const char *filename, bool overwrite)
+{
+	mode_t mode_if_exists = overwrite ? O_TRUNC : O_EXCL;
+	return fopenat(dirfd, filename, O_CREAT | O_RDWR | mode_if_exists, 0666, "w");
+}
+
+static bool serialize_environment(struct settings *settings, int dirfd)
+{
+	struct environment_variable *iter;
+	FILE *f;
+
+	if (file_exists_at(dirfd, env_filename) && !settings->overwrite) {
+		usage(stderr, "%s already exists, not overwriting", env_filename);
+		return false;
+	}
+
+	if ((f = fopenat_create(dirfd, env_filename, settings->overwrite)) == NULL)
+		return false;
+
+	igt_list_for_each_entry(iter, &settings->env_vars, link) {
+		fprintf(f, "%s=%s\n", iter->key, iter->value);
+	}
+
+	if (settings->sync) {
+		fflush(f);
+		fsync(fileno(f));
+	}
+
+	fclose(f);
+	return true;
+}
+
 bool serialize_settings(struct settings *settings)
 {
 #define SERIALIZE_LINE(f, s, name, format) fprintf(f, "%s : " format "\n", #name, s->name)
 
-	int dirfd, covfd, fd;
-	char path[PATH_MAX];
 	FILE *f;
+	int dirfd, covfd;
+	char path[PATH_MAX];
 
 	if (!settings->results_path) {
 		usage(stderr, "No results-path set; this shouldn't happen");
@@ -928,28 +1000,13 @@ bool serialize_settings(struct settings *settings)
 		}
 	}
 
-	if (!settings->overwrite &&
-	    faccessat(dirfd, settings_filename, F_OK, 0) == 0) {
-		usage(stderr, "Settings metadata already exists and not overwriting");
-		return false;
-	}
-
-	if (settings->overwrite &&
-	    unlinkat(dirfd, settings_filename, 0) != 0 &&
-	    errno != ENOENT) {
-		usage(stderr, "Error removing old settings metadata");
-		return false;
-	}
-
-	if ((fd = openat(dirfd, settings_filename, O_CREAT | O_EXCL | O_WRONLY, 0666)) < 0) {
-		usage(stderr, "Creating settings serialization file failed: %s", strerror(errno));
+	if (file_exists_at(dirfd, settings_filename) && !settings->overwrite) {
+		usage(stderr, "%s already exists, not overwriting", settings_filename);
 		close(dirfd);
 		return false;
 	}
 
-	f = fdopen(fd, "w");
-	if (!f) {
-		close(fd);
+	if ((f = fopenat_create(dirfd, settings_filename, settings->overwrite)) == NULL) {
 		close(dirfd);
 		return false;
 	}
@@ -980,11 +1037,22 @@ bool serialize_settings(struct settings *settings)
 	SERIALIZE_LINE(f, settings, code_coverage_script, "%s");
 
 	if (settings->sync) {
-		fsync(fd);
-		fsync(dirfd);
+		fflush(f);
+		fsync(fileno(f));
 	}
 
 	fclose(f);
+
+	if (!igt_list_empty(&settings->env_vars)) {
+		if (!serialize_environment(settings, dirfd)) {
+			close(dirfd);
+			return false;
+		}
+	}
+
+	if (settings->sync)
+		fsync(dirfd);
+
 	close(dirfd);
 	return true;
 
@@ -1053,22 +1121,62 @@ bool read_settings_from_file(struct settings *settings, FILE *f)
 #undef PARSE_LINE
 }
 
+/**
+ * read_env_vars_from_file() - load env vars from a file
+ * @env_vars: list head to add env var entries to
+ * @f: opened file stream to read
+ *
+ * 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,
+ * starting with a '#' (comments) or '=' (missing keys) 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) {
+		char *line_ptr = line;
+
+		while (isspace(line_ptr)) {
+			line_length--;
+			line_ptr++;
+		}
+
+		if (*line_ptr == '\0' || *line_ptr == '#' || *line_ptr == '=')
+			continue; /* Empty, whitespace, comment or missing key */
+
+		if (strchr(line_ptr, '=') == NULL)
+			continue; /* Missing value */
+
+		/* Strip the line feed, but keep trailing whitespace */
+		if (line_length > 0 && line[line_length - 1] == '\n')
+			line[line_length - 1] = '\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)
+	/* settings are always there */
+	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;
@@ -1076,5 +1184,18 @@ bool read_settings_from_dir(struct settings *settings, int dirfd)
 
 	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;
 }
-- 
2.37.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 5/7] runner: Set requested env vars during execution
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
                     ` (3 preceding siblings ...)
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 4/7] runner: Serialize the provided environment flags Ryszard Knop
@ 2022-08-08 16:18   ` Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 6/7] runner: Add help for the --environment flag Ryszard Knop
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

v2: Removed unnecessary checks, misc cleanup (Mauro)
Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/executor.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/runner/executor.c b/runner/executor.c
index 9b89cc09..964d0063 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1860,10 +1860,10 @@ bool execute(struct execute_state *state,
 	     struct settings *settings,
 	     struct job_list *job_list)
 {
+	int resdirfd, testdirfd, unamefd, timefd, sigfd;
+	struct environment_variable *env_var;
 	struct utsname unamebuf;
-	int resdirfd, testdirfd, unamefd, timefd;
 	sigset_t sigmask;
-	int sigfd;
 	double time_spent = 0.0;
 	bool status = true;
 
@@ -1872,6 +1872,10 @@ bool execute(struct execute_state *state,
 		return true;
 	}
 
+	igt_list_for_each_entry(env_var, &settings->env_vars, link) {
+		setenv(env_var->key, env_var->value, 1);
+	}
+
 	if ((resdirfd = open(settings->results_path, O_DIRECTORY | O_RDONLY)) < 0) {
 		/* Initialize state should have done this */
 		errf("Error: Failure opening results path %s\n",
-- 
2.37.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 6/7] runner: Add help for the --environment flag
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
                     ` (4 preceding siblings ...)
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 5/7] runner: Set requested env vars during execution Ryszard Knop
@ 2022-08-08 16:18   ` Ryszard Knop
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 7/7] runner: Add tests for environment flags Ryszard Knop
  2022-08-31  9:30   ` [igt-dev] [PATCH i-g-t v2 0/7] Add the --environment flag Petri Latvala
  7 siblings, 0 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/settings.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/runner/settings.c b/runner/settings.c
index b57ab080..cd40208b 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -286,6 +286,10 @@ static const char *usage_str =
 	"  -b, --blacklist FILENAME\n"
 	"                        Exclude all test matching to regexes from FILENAME\n"
 	"                        (can be used more than once)\n"
+	"  -e, --environment <KEY or KEY=VALUE>\n"
+	"                        Set an environment variable for the test process.\n"
+	"                        If only the key is provided, the current value is read\n"
+	"                        from the runner's environment (and saved for resumes).\n"
 	"  -L, --list-all        List all matching subtests instead of running\n"
 	"  --collect-code-cov    Enables gcov-based collect of code coverage for tests.\n"
 	"                        Requires --collect-script FILENAME\n"
-- 
2.37.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* [igt-dev] [PATCH i-g-t v2 7/7] runner: Add tests for environment flags
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
                     ` (5 preceding siblings ...)
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 6/7] runner: Add help for the --environment flag Ryszard Knop
@ 2022-08-08 16:18   ` Ryszard Knop
  2022-08-31  9:30   ` [igt-dev] [PATCH i-g-t v2 0/7] Add the --environment flag Petri Latvala
  7 siblings, 0 replies; 32+ messages in thread
From: Ryszard Knop @ 2022-08-08 16:18 UTC (permalink / raw)
  To: Development mailing list for IGT GPU Tools

Per Mauro's review, add some --environment tests:
- Check if -e/--environment works,
- Check both KEY=VALUE and KEY,
- Check if resumes load the variables correctly.

Signed-off-by: Ryszard Knop <ryszard.knop@intel.com>
---
 runner/runner_tests.c | 30 ++++++++++++++++++++++++++++++
 runner/settings.c     |  2 +-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/runner/runner_tests.c b/runner/runner_tests.c
index aca031fc..db0ce2ac 100644
--- a/runner/runner_tests.c
+++ b/runner/runner_tests.c
@@ -281,6 +281,7 @@ igt_main
 		igt_assert(!settings->dry_run);
 		igt_assert_eq(settings->include_regexes.size, 0);
 		igt_assert_eq(settings->exclude_regexes.size, 0);
+		igt_assert(igt_list_empty(&settings->env_vars));
 		igt_assert(!settings->sync);
 		igt_assert_eq(settings->log_level, LOG_LEVEL_NORMAL);
 		igt_assert(!settings->overwrite);
@@ -421,6 +422,8 @@ igt_main
 
 	igt_subtest("parse-all-settings") {
 		char blacklist_name[PATH_MAX], blacklist2_name[PATH_MAX];
+		struct environment_variable *env_var;
+
 		const char *argv[] = { "runner",
 				       "--allow-non-root",
 				       "-n", "foo",
@@ -433,6 +436,8 @@ igt_main
 				       "-t", "pattern2",
 				       "-x", "xpattern1",
 				       "-x", "xpattern2",
+				       "-e", "HAVE_A_NICE=TESTING",
+				       "--environment", "ENVS_WITH_JUST_KEYS",
 				       "-b", blacklist_name,
 				       "--blacklist", blacklist2_name,
 				       "-s",
@@ -453,6 +458,8 @@ igt_main
 				       "path-to-results",
 		};
 
+		setenv("ENVS_WITH_JUST_KEYS", "SHOULD_WORK", 1);
+
 		sprintf(blacklist_name, "%s/test-blacklist.txt", testdatadir);
 		sprintf(blacklist2_name, "%s/test-blacklist2.txt", testdatadir);
 
@@ -464,14 +471,27 @@ igt_main
 		igt_assert_eqstr(settings->name, "foo");
 		igt_assert(settings->dry_run);
 		igt_assert(settings->allow_non_root);
+
 		igt_assert_eq(settings->include_regexes.size, 2);
 		igt_assert_eqstr(settings->include_regexes.regex_strings[0], "pattern1");
 		igt_assert_eqstr(settings->include_regexes.regex_strings[1], "pattern2");
+
 		igt_assert_eq(settings->exclude_regexes.size, 4);
 		igt_assert_eqstr(settings->exclude_regexes.regex_strings[0], "xpattern1");
 		igt_assert_eqstr(settings->exclude_regexes.regex_strings[1], "xpattern2");
 		igt_assert_eqstr(settings->exclude_regexes.regex_strings[2], "xpattern3"); /* From blacklist */
 		igt_assert_eqstr(settings->exclude_regexes.regex_strings[3], "xpattern4"); /* From blacklist2 */
+
+		igt_assert(!igt_list_empty(&settings->env_vars));
+
+		env_var = igt_list_first_entry(&settings->env_vars, env_var, link);
+		igt_assert_eqstr(env_var->key, "HAVE_A_NICE");
+		igt_assert_eqstr(env_var->value, "TESTING");
+
+		env_var = igt_list_last_entry(&settings->env_vars, env_var, link);
+		igt_assert_eqstr(env_var->key, "ENVS_WITH_JUST_KEYS");
+		igt_assert_eqstr(env_var->value, "SHOULD_WORK");
+
 		igt_assert(settings->sync);
 		igt_assert_eq(settings->log_level, LOG_LEVEL_VERBOSE);
 		igt_assert(settings->overwrite);
@@ -1029,9 +1049,12 @@ igt_main
 
 		igt_subtest("dry-run-option") {
 			struct execute_state state;
+			struct environment_variable *env_var;
+
 			const char *argv[] = { "runner",
 					       "--dry-run",
 					       "--allow-non-root",
+					       "-e", "JUST_TESTING=ENV_VARS",
 					       "-x", "^abort",
 					       testdatadir,
 					       dirname,
@@ -1058,6 +1081,9 @@ igt_main
 			igt_assert_f((fd = openat(dirfd, "joblist.txt", O_RDONLY)) >= 0,
 				     "Dry run initialization didn't serialize the job list.\n");
 			close(fd);
+			igt_assert_f((fd = openat(dirfd, "environment.txt", O_RDONLY)) >= 0,
+			             "Dry run initialization didn't serialize the environment file.\n");
+			close(fd);
 			igt_assert_f((fd = openat(dirfd, "uname.txt", O_RDONLY)) < 0,
 				     "Dry run initialization created uname.txt.\n");
 
@@ -1078,6 +1104,10 @@ igt_main
 				     "Dry run resume didn't create result directory.\n");
 			igt_assert_f((fd = openat(subdirfd, "journal.txt", O_RDONLY)) >= 0,
 				     "Dry run resume didn't create a journal.\n");
+
+			env_var = igt_list_first_entry(&settings->env_vars, env_var, link);
+			igt_assert_eqstr(env_var->key, "JUST_TESTING");
+			igt_assert_eqstr(env_var->value, "ENV_VARS");
 		}
 
 		igt_fixture {
diff --git a/runner/settings.c b/runner/settings.c
index cd40208b..23aa8296 100644
--- a/runner/settings.c
+++ b/runner/settings.c
@@ -1146,7 +1146,7 @@ static bool read_env_vars_from_file(struct igt_list_head *env_vars, FILE *f)
 	while ((line_length = getline(&line, &line_buffer_length, f)) != -1) {
 		char *line_ptr = line;
 
-		while (isspace(line_ptr)) {
+		while (isspace(*line_ptr)) {
 			line_length--;
 			line_ptr++;
 		}
-- 
2.37.1

^ permalink raw reply related	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v2 0/7] Add the --environment flag
  2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
                     ` (6 preceding siblings ...)
  2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 7/7] runner: Add tests for environment flags Ryszard Knop
@ 2022-08-31  9:30   ` Petri Latvala
  2022-08-31 11:09     ` Knop, Ryszard
  7 siblings, 1 reply; 32+ messages in thread
From: Petri Latvala @ 2022-08-31  9:30 UTC (permalink / raw)
  To: Ryszard Knop; +Cc: Development mailing list for IGT GPU Tools

On Mon, Aug 08, 2022 at 06:18:19PM +0200, Ryszard Knop wrote:
> It sets the provided KEY=VALUE pair as the environment variable for
> executed test processes. If just the KEY is provided, the currently-set
> value is reused (and the run fails if it's not set for the runner).
> 
> The most important thing about this feature is that all the variables
> set this way are saved for resumed runs as well. This lets developers
> easily try something out from the terminal without having to remember
> to set the same vars/reuse a wrapper to continue a run.

Series is

Reviewed-by: Petri Latvala <petri.latvala@intel.com>

However, patchwork looks confused about this series, the individual
patches are there but they didn't create a series so no premerge
testing has been done for this. Can you resend for a peace of mind?


-- 
Petri Latvala

^ permalink raw reply	[flat|nested] 32+ messages in thread
* Re: [igt-dev] [PATCH i-g-t v2 0/7] Add the --environment flag
  2022-08-31  9:30   ` [igt-dev] [PATCH i-g-t v2 0/7] Add the --environment flag Petri Latvala
@ 2022-08-31 11:09     ` Knop, Ryszard
  0 siblings, 0 replies; 32+ messages in thread
From: Knop, Ryszard @ 2022-08-31 11:09 UTC (permalink / raw)
  To: Latvala, Petri; +Cc: igt-dev

On Wed, 2022-08-31 at 12:30 +0300, Petri Latvala wrote:
> Series is
> 
> Reviewed-by: Petri Latvala <petri.latvala@intel.com>
> 
> However, patchwork looks confused about this series, the individual
> patches are there but they didn't create a series so no premerge
> testing has been done for this. Can you resend for a peace of mind?

Thanks! Resent as https://patchwork.freedesktop.org/series/107966/

^ permalink raw reply	[flat|nested] 32+ messages in thread
end of thread, other threads:[~2022-08-31 11:09 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-28  9:44 [igt-dev] [PATCH i-g-t 0/7] Add the --environment flag Ryszard Knop
2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 1/7] runner: rename free_settings to clear_settings Ryszard Knop
2022-06-29 15:48   ` Mauro Carvalho Chehab
2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 2/7] runner: make the usage function variadic Ryszard Knop
2022-06-28 23:51   ` [igt-dev] [i-g-t,2/7] " Maíra Canal
2022-06-30 13:16     ` Knop, Ryszard
2022-06-29 15:51   ` [igt-dev] [PATCH i-g-t 2/7] " Mauro Carvalho Chehab
2022-07-25 11:53   ` Petri Latvala
2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 3/7] igt_list: Add the igt_list_empty_or_null helper Ryszard Knop
2022-06-29 16:00   ` Mauro Carvalho Chehab
2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 4/7] runner: Add the --environment flag Ryszard Knop
2022-06-29 16:13   ` Mauro Carvalho Chehab
     [not found]     ` <b1e8327b02fbbda0e180b217735b460c6f307f6b.camel@intel.com>
2022-07-05 19:58       ` Mauro Carvalho Chehab
2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 5/7] runner: Serialize the provided environment flags Ryszard Knop
2022-06-29 16:36   ` Mauro Carvalho Chehab
     [not found]     ` <6622adedd2931cbd6d4e0c3eb46a2d3f72343351.camel@intel.com>
2022-07-05 20:12       ` Mauro Carvalho Chehab
2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 6/7] runner: Set requested env vars during execution Ryszard Knop
2022-06-29 16:28   ` Mauro Carvalho Chehab
2022-06-28  9:44 ` [igt-dev] [PATCH i-g-t 7/7] runner: Add help for the --environment flag Ryszard Knop
2022-06-29 16:30   ` Mauro Carvalho Chehab
2022-06-28 10:36 ` [igt-dev] ✓ Fi.CI.BAT: success for Add " Patchwork
2022-06-29  0:03 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
2022-08-08 16:18 ` [igt-dev] [PATCH i-g-t v2 0/7] " Ryszard Knop
2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 1/7] runner: rename free_settings to clear_settings Ryszard Knop
2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 2/7] runner: make the usage function variadic Ryszard Knop
2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 3/7] runner: Add the --environment flag Ryszard Knop
2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 4/7] runner: Serialize the provided environment flags Ryszard Knop
2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 5/7] runner: Set requested env vars during execution Ryszard Knop
2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 6/7] runner: Add help for the --environment flag Ryszard Knop
2022-08-08 16:18   ` [igt-dev] [PATCH i-g-t v2 7/7] runner: Add tests for environment flags Ryszard Knop
2022-08-31  9:30   ` [igt-dev] [PATCH i-g-t v2 0/7] Add the --environment flag Petri Latvala
2022-08-31 11:09     ` Knop, Ryszard

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.