Re: [PATCH i-g-t 6/9] lib: s/IGT_DEBUG_INTERACTIVE/--interactive-debug=var

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 09, 2014 at 09:01:54PM -0500, Rodrigo Vivi wrote:
> Use cmdline variable for interactive debug instead of env var.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Looks nice, two small comments below.

> ---
>  lib/igt_aux.c  | 20 ++++++++++----------
>  lib/igt_aux.h  |  2 +-
>  lib/igt_core.c |  6 ++++++
>  lib/igt_core.h |  2 ++
>  4 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index 49d1ec4..ff668d4 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -372,32 +372,32 @@ void igt_drop_root(void)
>  
>  /**
>   * igt_debug_wait_for_keypress:
> - * @key: env var lookup to to enable this wait
> + * @var: var lookup to to enable this wait
>   *
>   * Waits for a key press when run interactively and when the corresponding debug
> - * key is set in the IGT_DEBUG_INTERACTIVE environment variable. Multiple keys
> + * var is set in the --interactive-debug=<var> variable. Multiple keys
>   * can be specified as a comma-separated list or alternatively "all" if a wait
> - * should happen for all keys.  When not connected to a terminal the environment
> - * setting is ignored and execution immediately continues.
> + * should happen for all cases.
> + *
> + * When not connected to a terminal interactive_debug is ignored
> + * and execution immediately continues.
>   *
>   * This is useful for display tests where under certain situation manual
>   * inspection of the display is useful. Or when running a testcase in the
>   * background.
>   */
> -void igt_debug_wait_for_keypress(const char *key)
> +void igt_debug_wait_for_keypress(const char *var)
>  {
>  	struct termios oldt, newt;
> -	const char *env;
>  
>  	if (!isatty(STDIN_FILENO))
>  		return;
>  
> -	env = getenv("IGT_DEBUG_INTERACTIVE");
> -
> -	if (!env)
> +	if (!igt_interactive_debug)
>  		return;
>  
> -	if (!strstr(env, key) && !strstr(env, "all"))
> +	if (!strstr(igt_interactive_debug, var) &&
> +	    !strstr(igt_interactive_debug, "all"))
>  		return;
>  
>  	igt_info("Press any key to continue ...\n");
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index 63e1b06..59022cd 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -60,7 +60,7 @@ void igt_system_suspend_autoresume(void);
>  /* dropping priviledges */
>  void igt_drop_root(void);
>  
> -void igt_debug_wait_for_keypress(const char *key);
> +void igt_debug_wait_for_keypress(const char *var);
>  
>  enum igt_runtime_pm_status {
>  	IGT_RUNTIME_PM_STATUS_ACTIVE,
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 13a52a5..461b1d3 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -225,6 +225,7 @@ enum {
>   OPT_RUN_SUBTEST,
>   OPT_DESCRIPTION,
>   OPT_DEBUG,
> + OPT_INTERACTIVE_DEBUG,
>   OPT_HELP = 'h'
>  };
>  
> @@ -391,6 +392,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>  	fprintf(f, "  --list-subtests\n"
>  		   "  --run-subtest <pattern>\n"
>  		   "  --debug\n"
> +		   "  --interactive-debug <pattern>\n"
>  		   "  --help-description\n"
>  		   "  --help\n");
>  	if (help_str)
> @@ -423,6 +425,7 @@ static int common_init(int argc, char **argv,
>  		{"run-subtest", 1, 0, OPT_RUN_SUBTEST},
>  		{"help-description", 0, 0, OPT_DESCRIPTION},
>  		{"debug", 0, 0, OPT_DEBUG},
> +		{"interactive-debug", 1, 0, OPT_INTERACTIVE_DEBUG},
>  		{"help", 0, 0, OPT_HELP},
>  		{0, 0, 0, 0}
>  	};
> @@ -508,6 +511,9 @@ static int common_init(int argc, char **argv,
>  	while ((c = getopt_long(argc, argv, short_opts, combined_opts,
>  			       &option_index)) != -1) {
>  		switch(c) {
> +		case OPT_INTERACTIVE_DEBUG:
> +			igt_interactive_debug = strdup(optarg);;
> +			break;

Hm, while we change this to cmdline option I think we should make the
optarg optional and if it's not set, set it to "all".

>  		case OPT_DEBUG:
>  			igt_log_level = IGT_LOG_DEBUG;
>  			break;
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index a258348..20942e4 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -511,6 +511,8 @@ bool igt_run_in_simulation(void);
>  
>  void igt_skip_on_simulation(void);
>  
> +const char *igt_interactive_debug;

Can you please add a bit of gtkdoc for this, too, now that it's exported.

Thanks, Daniel

> +
>  /* structured logging */
>  enum igt_log_level {
>  	IGT_LOG_DEBUG,
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx