Re: [PATCH i-g-t v2 6/6] runner: Add hook-exec-allowlist to execute hooks selectively

[Gustavo Sousa <gustavo.sousa@intel.com>]

Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:

> Hook scripts like GuC log copy takes few seconds to complete. This would
> lead to large increase of CI execution time.  Add -a/--hook-exec-allowlist
> argument to igt_runner to pass file with regexps which matched will
> allow executing hook scripts. Syntax of this file is same to blocklists.

I feel like this is more related to the GuC log hook than a general
allowlist that the user would like to apply to all types of hooks
enabled.

An alternative approach would be to have guc_copy_on_fail.sh
implementing this type of filtering, with a path that is passed via CLI
arguments or an environment variable.  I would prefer that instead of
adding --hook-exec-allowlist, as I'm not much confident it would be very
useful for other cases.

Since the hook is written in bash, the filtering could be applied with
something along the lines of grep -f path/to/allowlist... (It could be
implemented with bash builtins as well, but I'm not sure it is worth the
hassle).

--
Gustavo Sousa

>
> Some limitation of this code is hooks will be executed with subtest
> granularity - if multiple_mode is used regexp will compare only first
> subtest so all subtest in group will call hooks. This is likely not
> a big problem as CI executes each subtest individually (without
> multiple_mode).
>
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Ryszard Knop <ryszard.knop@intel.com>
> Cc: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
>  runner/executor.c | 32 +++++++++++++++++++++++++++++---
>  runner/settings.c | 14 +++++++++++++-
>  runner/settings.h |  2 ++
>  3 files changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/runner/executor.c b/runner/executor.c
> index bc421f7dbb..ab34ed8258 100644
> --- a/runner/executor.c
> +++ b/runner/executor.c
> @@ -1615,6 +1615,18 @@ static int monitor_output(pid_t child,
>  	return killed;
>  }
>  
> +static bool matches_any(const char *str, struct regex_list *list)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < list->size; i++) {
> +		if (g_regex_match(list->regexes[i], str, 0, NULL))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static void __attribute__((noreturn))
>  execute_test_process(int outfd, int errfd, int socketfd,
>  		     struct settings *settings,
> @@ -1623,6 +1635,7 @@ execute_test_process(int outfd, int errfd, int socketfd,
>  	struct igt_vec arg_vec;
>  	char *arg;
>  	size_t rootlen;
> +	bool use_hooks = true;
>  
>  	dup2(outfd, STDOUT_FILENO);
>  	dup2(errfd, STDERR_FILENO);
> @@ -1646,11 +1659,24 @@ execute_test_process(int outfd, int errfd, int socketfd,
>  		arg = strdup("--run-subtest");
>  		igt_vec_push(&arg_vec, &arg);
>  
> -		if ((dynbegin = strchr(entry->subtests[0], '@')) != NULL)
> +		if ((dynbegin = strchr(entry->subtests[0], '@')) != NULL) {
>  			argsize = dynbegin - entry->subtests[0];
> -		else
> +		} else {
>  			argsize = strlen(entry->subtests[0]);
>  
> +			if (settings->use_hook_allow_regexes) {
> +				char *piglit_name;
> +
> +				asprintf(&piglit_name, "igt@%s@%s",
> +					 entry->binary, entry->subtests[0]);
> +
> +				if (!matches_any(piglit_name, &settings->hook_allow_regexes))
> +					use_hooks = false;
> +
> +				free(piglit_name);
> +			}
> +		}
> +
>  		arg = malloc(argsize + 1);
>  		memcpy(arg, entry->subtests[0], argsize);
>  		arg[argsize] = '\0';
> @@ -1677,7 +1703,7 @@ execute_test_process(int outfd, int errfd, int socketfd,
>  		}
>  	}
>  
> -	for (size_t i = 0; i < igt_vec_length(&settings->hook_strs); i++) {
> +	for (size_t i = 0; use_hooks && i < igt_vec_length(&settings->hook_strs); i++) {
>  		arg = strdup("--hook");
>  		igt_vec_push(&arg_vec, &arg);
>  		arg = strdup(*((char **)igt_vec_elem(&settings->hook_strs, i)));
> diff --git a/runner/settings.c b/runner/settings.c
> index 7c29f2d3ab..eb3d05405c 100644
> --- a/runner/settings.c
> +++ b/runner/settings.c
> @@ -50,6 +50,7 @@ enum {
>  	OPT_TIMEOUT = 'c',
>  	OPT_WATCHDOG = 'g',
>  	OPT_BLACKLIST = 'b',
> +	OPT_HOOK_EXEC_ALLOWLIST = 'a',
>  	OPT_LIST_ALL = 'L',
>  };
>  
> @@ -320,6 +321,9 @@ static const char *usage_str =
>  	"                        Forward HOOK_STR to the --hook option of each test.\n"
>  	"  --help-hook\n"
>  	"                        Show detailed usage information for --hook.\n"
> +	"  -a, --hook-exec-allowlist FILENAME\n"
> +	"                        Tests name regexes from FILENAME for which hooks scripts\n"
> +	"                        may be executed\n"
>  	"\n"
>  	"  [test_root]           Directory that contains the IGT tests. The environment\n"
>  	"                        variable IGT_TEST_ROOT will be used if set, overriding\n"
> @@ -715,6 +719,7 @@ bool parse_options(int argc, char **argv,
>  		{"dmesg-warn-level", required_argument, NULL, OPT_DMESG_WARN_LEVEL},
>  		{"prune-mode", required_argument, NULL, OPT_PRUNE_MODE},
>  		{"blacklist", required_argument, NULL, OPT_BLACKLIST},
> +		{"hook-exec-allowlist", required_argument, NULL, OPT_HOOK_EXEC_ALLOWLIST},
>  		{"list-all", no_argument, NULL, OPT_LIST_ALL},
>  		{ 0, 0, 0, 0},
>  	};
> @@ -726,7 +731,7 @@ bool parse_options(int argc, char **argv,
>  	settings->dmesg_warn_level = -1;
>  	settings->prune_mode = -1;
>  
> -	while ((c = getopt_long(argc, argv, "hn:dt:x:e:fk::sl:omb:L",
> +	while ((c = getopt_long(argc, argv, "hn:dt:x:e:fk::sl:omb:a:L",
>  				long_options, NULL)) != -1) {
>  		switch (c) {
>  		case OPT_VERSION:
> @@ -852,6 +857,13 @@ bool parse_options(int argc, char **argv,
>  					"blacklist"))
>  				goto error;
>  			break;
> +		case OPT_HOOK_EXEC_ALLOWLIST:
> +			if (!parse_list(&settings->hook_allow_regexes,
> +					absolute_path(optarg),
> +					"hook-exec-allowlist"))
> +				goto error;
> +			settings->use_hook_allow_regexes = true;
> +			break;
>  		case OPT_LIST_ALL:
>  			settings->list_all = true;
>  			break;
> diff --git a/runner/settings.h b/runner/settings.h
> index 6c58c3282c..b81f6adc6d 100644
> --- a/runner/settings.h
> +++ b/runner/settings.h
> @@ -59,6 +59,8 @@ struct settings {
>  	bool allow_non_root;
>  	struct regex_list include_regexes;
>  	struct regex_list exclude_regexes;
> +	struct regex_list hook_allow_regexes;
> +	bool use_hook_allow_regexes;
>  	struct igt_list_head env_vars;
>  	struct igt_vec hook_strs;
>  	bool facts;
> -- 
> 2.43.0