Re: [PATCH i-g-t 3/8] lib: move option parsing into common_init

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 23, 2014 at 02:13:14PM +0200, Daniel Vetter wrote:
> On Wed, Jul 23, 2014 at 11:57:50AM +0100, Thomas Wood wrote:
> > Move option parsing into common_init so it can be shared between simple
> > tests and tests with subtests. This allows for more common command line
> > behaviour across all tests.
> > 
> > Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> > ---
> >  lib/igt_core.c | 118 ++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 74 insertions(+), 44 deletions(-)
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 5c20581..ee6f90c 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -291,30 +291,11 @@ static void oom_adjust_for_doom(void)
> >  	igt_assert(write(fd, always_kill, sizeof(always_kill)) == sizeof(always_kill));
> >  }
> >  
> > -/**
> > - * igt_subtest_init_parse_opts:
> > - * @argc: argc from the test's main()
> > - * @argv: argv from the test's main()
> > - * @extra_short_opts: getopt_long() compliant list with additional short options
> > - * @extra_long_opts: getopt_long() compliant list with additional long options
> > - * @help_str: help string for the additional options
> > - * @extra_opt_handler: handler for the additional options
> > - *
> > - * This function handles the subtest related cmdline options and allows an
> > - * arbitrary set of additional options. This is useful for tests which have
> > - * additional knobs to tune when run manually like the number of rounds execute
> > - * or the size of the allocated buffer objects.
> > - *
> > - * Tests without special needs should just use igt_subtest_init() or use
> > - * #igt_main directly instead of their own main() function.
> > - *
> > - * Returns: Forwards any option parsing errors from getopt_long.
> > - */
> > -int igt_subtest_init_parse_opts(int argc, char **argv,
> > -				const char *extra_short_opts,
> > -				struct option *extra_long_opts,
> > -				const char *help_str,
> > -				igt_opt_handler_t extra_opt_handler)
> > +static int common_init(int argc, char **argv,
> > +		       const char *extra_short_opts,
> > +		       struct option *extra_long_opts,
> > +		       const char *help_str,
> > +		       igt_opt_handler_t extra_opt_handler)
> >  {
> >  	int c, option_index = 0;
> >  	static struct option long_options[] = {
> > @@ -328,8 +309,18 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
> >  	int extra_opt_count;
> >  	int all_opt_count;
> >  	int ret = 0;
> > +	char *env = getenv("IGT_LOG_LEVEL");
> >  
> > -	test_with_subtests = true;
> > +	if (env) {
> > +		if (strcmp(env, "debug") == 0)
> > +			igt_log_level = IGT_LOG_DEBUG;
> > +		else if (strcmp(env, "info") == 0)
> > +			igt_log_level = IGT_LOG_INFO;
> > +		else if (strcmp(env, "warn") == 0)
> > +			igt_log_level = IGT_LOG_WARN;
> > +		else if (strcmp(env, "none") == 0)
> > +			igt_log_level = IGT_LOG_NONE;
> > +	}
> >  
> >  	command_str = argv[0];
> >  	if (strrchr(command_str, '/'))
> > @@ -389,36 +380,70 @@ int igt_subtest_init_parse_opts(int argc, char **argv,
> >  		}
> >  	}
> >  
> > -	igt_install_exit_handler(check_igt_exit);
> >  	oom_adjust_for_doom();
> >  
> >  out:
> >  	free(short_opts);
> >  	free(combined_opts);
> > +
> > +	/* exit immediately if this test has no subtests and a subtest or the
> > +	 * list of subtests has been requested */
> > +	if (!test_with_subtests) {
> > +		if (run_single_subtest) {
> > +			igt_warn("Unknown subtest: %s\n", run_single_subtest);
> > +			exit(-1);
> > +		}
> > +		if (list_subtests)
> > +			exit(-1);
> 
> Instead of -1 I think we should have a common IGT_EXIT_INVALID code like
> 79 similar to skip and timeout and use this one here and in the code
> you've added to igt_exit in a previous patch.
> 
> Probably easiest if you do this as a fixup on top.
> 
> Otherwise I think this goes into the right direction, so ack on the entire
> series. I haven't done an in-depth review, but didn't spot anything else
> really.

Aside: If we have this special exit code which no other tests uses
otherwise we can catch it in piglit. That will be extremely useful to
catch all those offenders who accidentally left a printf or similar
unguarded by igt_fixture or igt_subtest somewhere in their code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch