[PATCH i-g-t 0/8] command line parsing

[PATCH i-g-t 0/8] command line parsing

[Thomas Wood]

The following series attempts to standardise command line parsing and options
across the various types of tests. This allows all tests to respond to common
options such as --help and --debug, and also warn when unknown or invalid
options are used.

Thomas Wood (8):
  lib: warn when attempting to run an unknown subtest
  tests: remove unused getopt header includes
  lib: move option parsing into common_init
  lib: add igt_simple_init_parse_opts
  lib: don't ignore unknown options in multi-tests
  tests: convert simple tests to use igt_simple_init_parse_opts
  lib: always warn about unknown options
  lib: add a command line option to enable debug output in tests

 lib/drmtest.c                   |   1 -
 lib/igt_aux.c                   |   1 -
 lib/igt_core.c                  | 184 ++++++++++++++++++++++++----------------
 lib/igt_core.h                  |   6 ++
 lib/rendercopy_gen6.c           |   1 -
 lib/rendercopy_gen7.c           |   1 -
 lib/rendercopy_gen8.c           |   1 -
 lib/rendercopy_i830.c           |   1 -
 lib/rendercopy_i915.c           |   1 -
 tests/gem_ctx_basic.c           |  20 ++---
 tests/gem_media_fill.c          |   1 -
 tests/gem_render_copy.c         |  25 +++---
 tests/gem_render_copy_redux.c   |   1 -
 tests/gem_render_linear_blits.c |   1 -
 tests/gem_render_tiled_blits.c  |   1 -
 tests/gem_seqno_wrap.c          |  82 +++++++-----------
 tests/gem_stress.c              | 105 +++++++++++------------
 tests/gem_wait_render_timeout.c |   1 -
 tests/kms_setmode.c             |   1 -
 tests/pm_rps.c                  |   1 -
 20 files changed, 215 insertions(+), 221 deletions(-)

-- 
1.9.3

[PATCH i-g-t 1/8] lib: warn when attempting to run an unknown subtest

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index b197932..5c20581 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -204,6 +204,7 @@ static unsigned int exit_handler_count;
 /* subtests helpers */
 static bool list_subtests = false;
 static char *run_single_subtest = NULL;
+static bool run_single_subtest_found = false;
 static const char *in_subtest = NULL;
 static bool in_fixture = false;
 static bool test_with_subtests = false;
@@ -484,9 +485,14 @@ bool __igt_run_subtest(const char *subtest_name)
 		return false;
 	}
 
-	if (run_single_subtest &&
-	    strcmp(subtest_name, run_single_subtest) != 0)
-		return false;
+	if (run_single_subtest) {
+		if (strcmp(subtest_name, run_single_subtest) != 0)
+			return false;
+		else
+			run_single_subtest_found = true;
+	}
+
+
 
 	if (skip_subtests_henceforth) {
 		printf("Subtest %s: %s\n", subtest_name,
@@ -722,6 +728,12 @@ void igt_exit(void)
 {
 	igt_exit_called = true;
 
+	if (run_single_subtest && !run_single_subtest_found) {
+		igt_warn("Unknown subtest: %s\n", run_single_subtest);
+		exit(-1);
+	}
+
+
 	if (igt_only_list_subtests())
 		exit(IGT_EXIT_SUCCESS);
 
-- 
1.9.3

[PATCH i-g-t 2/8] tests: remove unused getopt header includes

[Thomas Wood]

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/drmtest.c                   | 1 -
 lib/igt_aux.c                   | 1 -
 lib/rendercopy_gen6.c           | 1 -
 lib/rendercopy_gen7.c           | 1 -
 lib/rendercopy_gen8.c           | 1 -
 lib/rendercopy_i830.c           | 1 -
 lib/rendercopy_i915.c           | 1 -
 tests/gem_media_fill.c          | 1 -
 tests/gem_render_copy_redux.c   | 1 -
 tests/gem_render_linear_blits.c | 1 -
 tests/gem_render_tiled_blits.c  | 1 -
 tests/gem_wait_render_timeout.c | 1 -
 tests/kms_setmode.c             | 1 -
 tests/pm_rps.c                  | 1 -
 14 files changed, 14 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 7be2e40..f921f67 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -39,7 +39,6 @@
 #include <sys/mman.h>
 #include <signal.h>
 #include <pciaccess.h>
-#include <getopt.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <sys/wait.h>
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index 7b277be..0bcaa3b 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -39,7 +39,6 @@
 #include <sys/mman.h>
 #include <signal.h>
 #include <pciaccess.h>
-#include <getopt.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <sys/wait.h>
diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c
index 7b3104c..afe7562 100644
--- a/lib/rendercopy_gen6.c
+++ b/lib/rendercopy_gen6.c
@@ -9,7 +9,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 #include "drm.h"
 #include "i915_drm.h"
 #include "drmtest.h"
diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c
index 5131d8f..2932f1e 100644
--- a/lib/rendercopy_gen7.c
+++ b/lib/rendercopy_gen7.c
@@ -9,7 +9,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 #include "drm.h"
 #include "i915_drm.h"
 #include "drmtest.h"
diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c
index a32871d..ff0b0c8 100644
--- a/lib/rendercopy_gen8.c
+++ b/lib/rendercopy_gen8.c
@@ -9,7 +9,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 
 #include <drm.h>
 #include <i915_drm.h>
diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c
index f0235a5..04215b1 100644
--- a/lib/rendercopy_i830.c
+++ b/lib/rendercopy_i830.c
@@ -8,7 +8,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 #include "drm.h"
 #include "i915_drm.h"
 #include "drmtest.h"
diff --git a/lib/rendercopy_i915.c b/lib/rendercopy_i915.c
index 1acf9da..fc9583c 100644
--- a/lib/rendercopy_i915.c
+++ b/lib/rendercopy_i915.c
@@ -8,7 +8,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 #include "drm.h"
 #include "i915_drm.h"
 #include "drmtest.h"
diff --git a/tests/gem_media_fill.c b/tests/gem_media_fill.c
index db2380b..b06a556 100644
--- a/tests/gem_media_fill.c
+++ b/tests/gem_media_fill.c
@@ -41,7 +41,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 #include "drm.h"
 #include "ioctl_wrappers.h"
 #include "drmtest.h"
diff --git a/tests/gem_render_copy_redux.c b/tests/gem_render_copy_redux.c
index f711fdb..cb48aa7 100644
--- a/tests/gem_render_copy_redux.c
+++ b/tests/gem_render_copy_redux.c
@@ -43,7 +43,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 
 #include <drm.h>
 
diff --git a/tests/gem_render_linear_blits.c b/tests/gem_render_linear_blits.c
index ee99dea..28fd8c8 100644
--- a/tests/gem_render_linear_blits.c
+++ b/tests/gem_render_linear_blits.c
@@ -46,7 +46,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 
 #include <drm.h>
 
diff --git a/tests/gem_render_tiled_blits.c b/tests/gem_render_tiled_blits.c
index 3d83f7c..ea1d59d 100644
--- a/tests/gem_render_tiled_blits.c
+++ b/tests/gem_render_tiled_blits.c
@@ -42,7 +42,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 
 #include <drm.h>
 
diff --git a/tests/gem_wait_render_timeout.c b/tests/gem_wait_render_timeout.c
index a34c006..3afab9c 100644
--- a/tests/gem_wait_render_timeout.c
+++ b/tests/gem_wait_render_timeout.c
@@ -36,7 +36,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 
 #include <drm.h>
 
diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c
index 0b765a9..8762255 100644
--- a/tests/kms_setmode.c
+++ b/tests/kms_setmode.c
@@ -29,7 +29,6 @@
 #include <stdint.h>
 #include <unistd.h>
 #include <string.h>
-#include <getopt.h>
 #include <sys/time.h>
 
 #include "drmtest.h"
diff --git a/tests/pm_rps.c b/tests/pm_rps.c
index c1156a5..96fec99 100644
--- a/tests/pm_rps.c
+++ b/tests/pm_rps.c
@@ -31,7 +31,6 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
-#include <getopt.h>
 #include <fcntl.h>
 #include <signal.h>
 #include <errno.h>
-- 
1.9.3

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

[Thomas Wood]

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);
+	}
+
+	if (ret < 0)
+		/* exit with no error for -h/--help */
+		exit(ret == -1 ? 0 : ret);
+
 	print_version();
 
 	return ret;
 }
 
-enum igt_log_level igt_log_level = IGT_LOG_INFO;
 
-static void common_init(void)
+/**
+ * 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)
 {
-	char *env = getenv("IGT_LOG_LEVEL");
+	int ret;
 
-	if (!env)
-		return;
+	test_with_subtests = true;
+	ret = common_init(argc, argv, extra_short_opts, extra_long_opts,
+			  help_str, extra_opt_handler);
+	igt_install_exit_handler(check_igt_exit);
 
-	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;
+	return ret;
 }
 
+enum igt_log_level igt_log_level = IGT_LOG_INFO;
+
 /**
  * igt_subtest_init:
  * @argc: argc from the test's main()
@@ -446,8 +471,6 @@ void igt_subtest_init(int argc, char **argv)
 
 	/* reset opt parsing */
 	optind = 1;
-
-	common_init();
 }
 
 /**
@@ -463,11 +486,18 @@ void igt_subtest_init(int argc, char **argv)
  */
 void igt_simple_init(int argc, char **argv)
 {
-	print_version();
+	int ret;
 
-	oom_adjust_for_doom();
+	/* supress getopt errors about unknown options */
+	opterr = 0;
+
+	ret = common_init(argc, argv, NULL, NULL, NULL, NULL);
+	if (ret < 0)
+		/* exit with no error for -h/--help */
+		exit(ret == -1 ? 0 : ret);
 
-	common_init();
+	/* reset opt parsing */
+	optind = 1;
 }
 
 /*
-- 
1.9.3

[PATCH i-g-t 4/8] lib: add igt_simple_init_parse_opts

[Thomas Wood]

This function allows simple tests to register additional command line
options.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 22 ++++++++++++++++++++++
 lib/igt_core.h |  5 +++++
 2 files changed, 27 insertions(+)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index ee6f90c..72c77e6 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -500,6 +500,28 @@ void igt_simple_init(int argc, char **argv)
 	optind = 1;
 }
 
+/**
+ * igt_simple_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 initializes a simple test without any support for subtests and allows
+ * an arbitrary set of additional options.
+ */
+void igt_simple_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)
+{
+	common_init(argc, argv, extra_short_opts, extra_long_opts, help_str,
+		    extra_opt_handler);
+}
+
 /*
  * Note: Testcases which use these helpers MUST NOT output anything to stdout
  * outside of places protected by igt_run_subtest checks - the piglit
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 6138487..408cf3a 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -162,6 +162,11 @@ bool igt_only_list_subtests(void);
 	static void igt_tokencat(__real_main, __LINE__)(void) \
 
 void igt_simple_init(int argc, char **argv);
+void igt_simple_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);
 
 /**
  * igt_simple_main:
-- 
1.9.3

[PATCH i-g-t 5/8] lib: don't ignore unknown options in multi-tests

[Thomas Wood]

None of the current tests have additional options that might make use of
any unknown options and igt_subtest_init_parse_opts is available that
integrates additional option parsing.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 72c77e6..0867c27 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -459,18 +459,7 @@ enum igt_log_level igt_log_level = IGT_LOG_INFO;
  */
 void igt_subtest_init(int argc, char **argv)
 {
-	int ret;
-
-	/* supress getopt errors about unknown options */
-	opterr = 0;
-
-	ret = igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL, NULL);
-	if (ret < 0)
-		/* exit with no error for -h/--help */
-		exit(ret == -1 ? 0 : ret);
-
-	/* reset opt parsing */
-	optind = 1;
+	igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL, NULL);
 }
 
 /**
-- 
1.9.3

[PATCH i-g-t 6/8] tests: convert simple tests to use igt_simple_init_parse_opts

[Thomas Wood]

Convert simple tests to use igt_simple_init_parse_opts if they require
extra options.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.h          |   1 +
 tests/gem_ctx_basic.c   |  20 +++------
 tests/gem_render_copy.c |  25 +++++-------
 tests/gem_seqno_wrap.c  |  82 +++++++++++++++----------------------
 tests/gem_stress.c      | 105 ++++++++++++++++++++++--------------------------
 5 files changed, 98 insertions(+), 135 deletions(-)

diff --git a/lib/igt_core.h b/lib/igt_core.h
index 408cf3a..b19a897 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -36,6 +36,7 @@
 #include <string.h>
 #include <sys/types.h>
 #include <stdarg.h>
+#include <getopt.h>
 
 /**
  * IGT_EXIT_TIMEOUT:
diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
index a2464fd..a0bec60 100644
--- a/tests/gem_ctx_basic.c
+++ b/tests/gem_ctx_basic.c
@@ -39,7 +39,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 #include "drm.h"
 #include "ioctl_wrappers.h"
 #include "drmtest.h"
@@ -119,11 +118,9 @@ static void *work(void *arg)
 	pthread_exit(NULL);
 }
 
-static void parse(int argc, char *argv[])
+static int opt_handler(int opt, int opt_index)
 {
-	int opt;
-	while ((opt = getopt(argc, argv, "i:c:n:muh?")) != -1) {
-		switch (opt) {
+	switch (opt) {
 		case 'i':
 			iter = atoi(optarg);
 			break;
@@ -136,20 +133,17 @@ static void parse(int argc, char *argv[])
 		case 'u':
 			uncontexted = 1;
 			break;
-		case 'h':
-		case '?':
-		default:
-			igt_success();
-			break;
-		}
 	}
+
+	return 0;
 }
 
 int main(int argc, char *argv[])
 {
 	int i;
 
-	igt_simple_init(argc, argv);
+	igt_simple_init_parse_opts(argc, argv, "i:c:n:mu", NULL, NULL,
+				   opt_handler);
 
 	fd = drm_open_any_render();
 	devid = intel_get_drm_devid(fd);
@@ -159,8 +153,6 @@ int main(int argc, char *argv[])
 		iter = 4;
 	}
 
-	parse(argc, argv);
-
 	threads = calloc(num_contexts, sizeof(*threads));
 
 	for (i = 0; i < num_contexts; i++)
diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c
index 76ba40e..6ff0c77 100644
--- a/tests/gem_render_copy.c
+++ b/tests/gem_render_copy.c
@@ -41,7 +41,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 
 #include <drm.h>
 
@@ -67,6 +66,7 @@ typedef struct {
 	drm_intel_bufmgr *bufmgr;
 	uint32_t linear[WIDTH * HEIGHT];
 } data_t;
+static int opt_dump_png = false;
 
 static void scratch_buf_write_to_png(struct igt_buf *buf, const char *filename)
 {
@@ -117,27 +117,24 @@ scratch_buf_check(data_t *data, struct igt_buf *buf, int x, int y,
 		     color, val, x, y);
 }
 
+static int opt_handler(int opt, int opt_index)
+{
+	if (opt == 'd') {
+		opt_dump_png = true;
+	}
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	data_t data = {0, };
 	struct intel_batchbuffer *batch = NULL;
 	struct igt_buf src, dst;
 	igt_render_copyfunc_t render_copy = NULL;
-	int opt;
-	int opt_dump_png = false;
 	int opt_dump_aub = igt_aub_dump_enabled();
 
-	igt_simple_init(argc, argv);
-
-	while ((opt = getopt(argc, argv, "d")) != -1) {
-		switch (opt) {
-		case 'd':
-			opt_dump_png = true;
-			break;
-		default:
-			break;
-		}
-	}
+	igt_simple_init_parse_opts(argc, argv, "d", NULL, NULL, opt_handler);
 
 	igt_fixture {
 		data.drm_fd = drm_open_any_render();
diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c
index 0fa722d..3a40860 100644
--- a/tests/gem_seqno_wrap.c
+++ b/tests/gem_seqno_wrap.c
@@ -38,7 +38,6 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <limits.h>
-#include <getopt.h>
 #include <signal.h>
 #include <errno.h>
 
@@ -452,45 +451,9 @@ static void background_run_once(void)
 		sleep(3);
 }
 
-static void print_usage(const char *s)
+static int parse_options(int opt, int opt_index)
 {
-	igt_info("%s: [OPTION]...\n", s);
-	igt_info("    where options are:\n");
-	igt_info("    -b --background       run in background inducing wraps\n");
-	igt_info("    -n --rounds=num       run num times across wrap boundary, 0 == forever\n");
-	igt_info("    -t --timeout=sec      set timeout to wait for testrun to sec seconds\n");
-	igt_info("    -d --dontwrap         don't wrap just run the test\n");
-	igt_info("    -p --prewrap=n        set seqno to WRAP - n for each testrun\n");
-	igt_info("    -r --norandom         dont randomize prewrap space\n");
-	igt_info("    -i --buffers          number of buffers to copy\n");
-	igt_fail(-1);
-}
-
-static void parse_options(int argc, char **argv)
-{
-	int c;
-	int option_index = 0;
-	static struct option long_options[] = {
-		{"rounds", required_argument, 0, 'n'},
-		{"background", no_argument, 0, 'b'},
-		{"timeout", required_argument, 0, 't'},
-		{"dontwrap", no_argument, 0, 'd'},
-		{"prewrap", required_argument, 0, 'p'},
-		{"norandom", no_argument, 0, 'r'},
-		{"buffers", required_argument, 0, 'i'},
-	};
-
-	options.rounds = SLOW_QUICK(50, 2);
-	options.background = 0;
-	options.dontwrap = 0;
-	options.timeout = 20;
-	options.random = 1;
-	options.prewrap_space = 21;
-	options.buffers = 10;
-
-	while((c = getopt_long(argc, argv, "n:bvt:dp:ri:",
-			       long_options, &option_index)) != -1) {
-		switch(c) {
+	switch(opt) {
 		case 'b':
 			options.background = 1;
 			igt_info("running in background inducing wraps\n");
@@ -520,17 +483,9 @@ static void parse_options(int argc, char **argv)
 			options.prewrap_space = atoi(optarg);
 			igt_info("prewrap set to %d (0x%x)\n", options.prewrap_space, UINT32_MAX - options.prewrap_space);
 			break;
-		default:
-			igt_info("unkown command options\n");
-			print_usage(argv[0]);
-			break;
-		}
 	}
 
-	if (optind < argc) {
-		igt_info("unkown command options\n");
-		print_usage(argv[0]);
-	}
+	return 0;
 }
 
 int main(int argc, char **argv)
@@ -538,9 +493,36 @@ int main(int argc, char **argv)
 	int wcount = 0;
 	int r = -1;
 
-	igt_simple_init(argc, argv);
+	static struct option long_options[] = {
+		{"rounds", required_argument, 0, 'n'},
+		{"background", no_argument, 0, 'b'},
+		{"timeout", required_argument, 0, 't'},
+		{"dontwrap", no_argument, 0, 'd'},
+		{"prewrap", required_argument, 0, 'p'},
+		{"norandom", no_argument, 0, 'r'},
+		{"buffers", required_argument, 0, 'i'},
+		{ 0, 0, 0, 0 }
+	};
+
+	const char *help =
+		"  -b --background       run in background inducing wraps\n"
+		"  -n --rounds=num       run num times across wrap boundary, 0 == forever\n"
+		"  -t --timeout=sec      set timeout to wait for testrun to sec seconds\n"
+		"  -d --dontwrap         don't wrap just run the test\n"
+		"  -p --prewrap=n        set seqno to WRAP - n for each testrun\n"
+		"  -r --norandom         dont randomize prewrap space\n"
+		"  -i --buffers          number of buffers to copy\n";
+
+	options.rounds = SLOW_QUICK(50, 2);
+	options.background = 0;
+	options.dontwrap = 0;
+	options.timeout = 20;
+	options.random = 1;
+	options.prewrap_space = 21;
+	options.buffers = 10;
 
-	parse_options(argc, argv);
+	igt_simple_init_parse_opts(argc, argv, "n:bvt:dp:ri:", long_options,
+				   help, parse_options);
 
 	card_index = drm_get_card();
 
diff --git a/tests/gem_stress.c b/tests/gem_stress.c
index c8d7393..3bbe487 100644
--- a/tests/gem_stress.c
+++ b/tests/gem_stress.c
@@ -58,7 +58,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/time.h>
-#include <getopt.h>
 
 #include <drm.h>
 
@@ -72,6 +71,10 @@
 
 #define CMD_POLY_STIPPLE_OFFSET       0x7906
 
+#define DUCTAPE 0xdead0001
+#define TILESZ	0xdead0002
+#define CHCK_RENDER 0xdead0003
+
 /** TODO:
  * - beat on relaxed fencing (i.e. mappable/fenceable tracking in the kernel)
  * - render copy (to check fence tracking and cache coherency management by the
@@ -624,54 +627,11 @@ static void sanitize_tiles_per_buf(void)
 		options.tiles_per_buf = options.scratch_buf_size / TILE_BYTES(options.tile_size);
 }
 
-static void parse_options(int argc, char **argv)
+static int parse_options(int opt, int opt_index)
 {
-	int c, tmp;
-	int option_index = 0;
-	static struct option long_options[] = {
-		{"no-hw", 0, 0, 'd'},
-		{"buf-size", 1, 0, 's'},
-		{"gpu-busy-load", 1, 0, 'g'},
-		{"no-signals", 0, 0, 'S'},
-		{"buffer-count", 1, 0, 'c'},
-		{"trace-tile", 1, 0, 't'},
-		{"disable-blt", 0, 0, 'b'},
-		{"disable-render", 0, 0, 'r'},
-		{"untiled", 0, 0, 'u'},
-		{"x-tiled", 0, 0, 'x'},
-		{"use-cpu-maps", 0, 0, 'm'},
-		{"rounds", 1, 0, 'o'},
-		{"no-fail", 0, 0, 'f'},
-		{"tiles-per-buf", 0, 0, 'p'},
-#define DUCTAPE 0xdead0001
-		{"remove-duct-tape", 0, 0, DUCTAPE},
-#define TILESZ	0xdead0002
-		{"tile-size", 1, 0, TILESZ},
-#define CHCK_RENDER 0xdead0003
-		{"check-render-cpyfn", 0, 0, CHCK_RENDER},
-		{NULL, 0, 0, 0},
-	};
-
-	options.scratch_buf_size = 256*4096;
-	options.no_hw = 0;
-	options.use_signal_helper = 1;
-	options.gpu_busy_load = 0;
-	options.num_buffers = 0;
-	options.trace_tile = -1;
-	options.use_render = 1;
-	options.use_blt = 1;
-	options.forced_tiling = -1;
-	options.use_cpu_maps = 0;
-	options.total_rounds = 512;
-	options.fail = 1;
-	options.ducttape = 1;
-	options.tile_size = 16;
-	options.tiles_per_buf = options.scratch_buf_size / TILE_BYTES(options.tile_size);
-	options.check_render_cpyfn = 0;
+	int tmp;
 
-	while((c = getopt_long(argc, argv, "ds:g:c:t:rbuxmo:fp:",
-			       long_options, &option_index)) != -1) {
-		switch(c) {
+	switch(opt) {
 		case 'd':
 			options.no_hw = 1;
 			igt_info("no-hw debug mode\n");
@@ -759,15 +719,8 @@ static void parse_options(int argc, char **argv)
 			options.check_render_cpyfn = 1;
 			igt_info("checking render copy function\n");
 			break;
-		default:
-			igt_info("unkown command options\n");
-			break;
-		}
 	}
 
-	if (optind < argc)
-		igt_info("unkown command options\n");
-
 	/* actually 32767, according to docs, but that kills our nice pot calculations. */
 	options.max_dimension = 16*1024;
 	if (options.use_render) {
@@ -777,6 +730,8 @@ static void parse_options(int argc, char **argv)
 			options.max_dimension = 8192;
 	}
 	igt_info("Limiting buffer to %dx%d\n", options.max_dimension, options.max_dimension);
+
+	return 0;
 }
 
 static void init(void)
@@ -864,14 +819,50 @@ int main(int argc, char **argv)
 {
 	int i, j;
 	unsigned *current_permutation, *tmp_permutation;
+	static struct option long_options[] = {
+		{"no-hw", 0, 0, 'd'},
+		{"buf-size", 1, 0, 's'},
+		{"gpu-busy-load", 1, 0, 'g'},
+		{"no-signals", 0, 0, 'S'},
+		{"buffer-count", 1, 0, 'c'},
+		{"trace-tile", 1, 0, 't'},
+		{"disable-blt", 0, 0, 'b'},
+		{"disable-render", 0, 0, 'r'},
+		{"untiled", 0, 0, 'u'},
+		{"x-tiled", 0, 0, 'x'},
+		{"use-cpu-maps", 0, 0, 'm'},
+		{"rounds", 1, 0, 'o'},
+		{"no-fail", 0, 0, 'f'},
+		{"tiles-per-buf", 0, 0, 'p'},
+		{"remove-duct-tape", 0, 0, DUCTAPE},
+		{"tile-size", 1, 0, TILESZ},
+		{"check-render-cpyfn", 0, 0, CHCK_RENDER},
+		{NULL, 0, 0, 0},
+	};
+
+	options.scratch_buf_size = 256*4096;
+	options.no_hw = 0;
+	options.use_signal_helper = 1;
+	options.gpu_busy_load = 0;
+	options.num_buffers = 0;
+	options.trace_tile = -1;
+	options.use_render = 1;
+	options.use_blt = 1;
+	options.forced_tiling = -1;
+	options.use_cpu_maps = 0;
+	options.total_rounds = 512;
+	options.fail = 1;
+	options.ducttape = 1;
+	options.tile_size = 16;
+	options.tiles_per_buf = options.scratch_buf_size / TILE_BYTES(options.tile_size);
+	options.check_render_cpyfn = 0;
 
-	igt_simple_init(argc, argv);
+	igt_simple_init_parse_opts(argc, argv,"ds:g:c:t:rbuxmo:fp:",
+				   long_options, NULL, parse_options);
 
 	drm_fd = drm_open_any();
 	devid = intel_get_drm_devid(drm_fd);
 
-	parse_options(argc, argv);
-
 	/* start our little helper early before too may allocations occur */
 	if (options.use_signal_helper)
 		igt_fork_signal_helper();
-- 
1.9.3

[PATCH i-g-t 7/8] lib: always warn about unknown options

[Thomas Wood]

All tests can now register extra options, so there should not be any
unknown options.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 0867c27..42b22fc 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -362,17 +362,9 @@ static int common_init(int argc, char **argv,
 			ret = -1;
 			goto out;
 		case '?':
-			if (opterr) {
-				print_usage(command_str, help_str, true);
-				ret = -2;
-				goto out;
-			}
-			/*
-			 * Just ignore the error, since the unknown argument
-			 * can be something the caller understands and will
-			 * parse by doing a second getopt scanning.
-			 */
-			break;
+			print_usage(command_str, help_str, true);
+			ret = -2;
+			goto out;
 		default:
 			ret = extra_opt_handler(c, option_index);
 			if (ret)
@@ -475,18 +467,7 @@ void igt_subtest_init(int argc, char **argv)
  */
 void igt_simple_init(int argc, char **argv)
 {
-	int ret;
-
-	/* supress getopt errors about unknown options */
-	opterr = 0;
-
-	ret = common_init(argc, argv, NULL, NULL, NULL, NULL);
-	if (ret < 0)
-		/* exit with no error for -h/--help */
-		exit(ret == -1 ? 0 : ret);
-
-	/* reset opt parsing */
-	optind = 1;
+	common_init(argc, argv, NULL, NULL, NULL, NULL);
 }
 
 /**
-- 
1.9.3

[PATCH i-g-t 8/8] lib: add a command line option to enable debug output in tests

[Thomas Wood]

Add --debug as a common command line option for all tests to enable
debug output.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 42b22fc..d90e6bb 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -276,7 +276,9 @@ static void print_usage(const char *command_str, const char *help_str,
 
 	fprintf(f, "Usage: %s [OPTIONS]\n"
 		   "  --list-subtests\n"
-		   "  --run-subtest <pattern>\n", command_str);
+		   "  --run-subtest <pattern>\n"
+		   "  --debug\n"
+		   "  --help\n", command_str);
 	if (help_str)
 		fprintf(f, "%s\n", help_str);
 }
@@ -301,6 +303,7 @@ static int common_init(int argc, char **argv,
 	static struct option long_options[] = {
 		{"list-subtests", 0, 0, 'l'},
 		{"run-subtest", 1, 0, 'r'},
+		{"debug", 0, 0, 'd'},
 		{"help", 0, 0, 'h'},
 	};
 	const char *command_str;
@@ -349,6 +352,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 'd':
+			igt_log_level = IGT_LOG_DEBUG;
+			break;
 		case 'l':
 			if (!run_single_subtest)
 				list_subtests = true;
-- 
1.9.3

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

[Daniel Vetter]

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.
-Daniel


> +	}
> +
> +	if (ret < 0)
> +		/* exit with no error for -h/--help */
> +		exit(ret == -1 ? 0 : ret);
> +
>  	print_version();
>  
>  	return ret;
>  }
>  
> -enum igt_log_level igt_log_level = IGT_LOG_INFO;
>  
> -static void common_init(void)
> +/**
> + * 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)
>  {
> -	char *env = getenv("IGT_LOG_LEVEL");
> +	int ret;
>  
> -	if (!env)
> -		return;
> +	test_with_subtests = true;
> +	ret = common_init(argc, argv, extra_short_opts, extra_long_opts,
> +			  help_str, extra_opt_handler);
> +	igt_install_exit_handler(check_igt_exit);
>  
> -	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;
> +	return ret;
>  }
>  
> +enum igt_log_level igt_log_level = IGT_LOG_INFO;
> +
>  /**
>   * igt_subtest_init:
>   * @argc: argc from the test's main()
> @@ -446,8 +471,6 @@ void igt_subtest_init(int argc, char **argv)
>  
>  	/* reset opt parsing */
>  	optind = 1;
> -
> -	common_init();
>  }
>  
>  /**
> @@ -463,11 +486,18 @@ void igt_subtest_init(int argc, char **argv)
>   */
>  void igt_simple_init(int argc, char **argv)
>  {
> -	print_version();
> +	int ret;
>  
> -	oom_adjust_for_doom();
> +	/* supress getopt errors about unknown options */
> +	opterr = 0;
> +
> +	ret = common_init(argc, argv, NULL, NULL, NULL, NULL);
> +	if (ret < 0)
> +		/* exit with no error for -h/--help */
> +		exit(ret == -1 ? 0 : ret);
>  
> -	common_init();
> +	/* reset opt parsing */
> +	optind = 1;
>  }
>  
>  /*
> -- 
> 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

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

[Daniel Vetter]

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

Re: [PATCH i-g-t 1/8] lib: warn when attempting to run an unknown subtest

[Gore, Tim]

Looks fine to me

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 1/8] lib: warn when attempting to run an unknown
> subtest
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
Reviewed-by: Tim Gore <tim.gore@intel.com>
> ---
>  lib/igt_core.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c index b197932..5c20581 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -204,6 +204,7 @@ static unsigned int exit_handler_count;
>  /* subtests helpers */
>  static bool list_subtests = false;
>  static char *run_single_subtest = NULL;
> +static bool run_single_subtest_found = false;
>  static const char *in_subtest = NULL;
>  static bool in_fixture = false;
>  static bool test_with_subtests = false; @@ -484,9 +485,14 @@ bool
> __igt_run_subtest(const char *subtest_name)
>  		return false;
>  	}
> 
> -	if (run_single_subtest &&
> -	    strcmp(subtest_name, run_single_subtest) != 0)
> -		return false;
> +	if (run_single_subtest) {
> +		if (strcmp(subtest_name, run_single_subtest) != 0)
> +			return false;
> +		else
> +			run_single_subtest_found = true;
> +	}
> +
> +
> 
>  	if (skip_subtests_henceforth) {
>  		printf("Subtest %s: %s\n", subtest_name, @@ -722,6
> +728,12 @@ void igt_exit(void)  {
>  	igt_exit_called = true;
> 
> +	if (run_single_subtest && !run_single_subtest_found) {
> +		igt_warn("Unknown subtest: %s\n", run_single_subtest);
> +		exit(-1);
> +	}
> +
> +
>  	if (igt_only_list_subtests())
>  		exit(IGT_EXIT_SUCCESS);
> 
> --
> 1.9.3

Re: [PATCH i-g-t 2/8] tests: remove unused getopt header includes

[Gore, Tim]

Looks fine to me.

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 2/8] tests: remove unused getopt header includes
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
Reviewed-by: Tim Gore <tim.gore@intel.com>
> ---
>  lib/drmtest.c                   | 1 -
>  lib/igt_aux.c                   | 1 -
>  lib/rendercopy_gen6.c           | 1 -
>  lib/rendercopy_gen7.c           | 1 -
>  lib/rendercopy_gen8.c           | 1 -
>  lib/rendercopy_i830.c           | 1 -
>  lib/rendercopy_i915.c           | 1 -
>  tests/gem_media_fill.c          | 1 -
>  tests/gem_render_copy_redux.c   | 1 -
>  tests/gem_render_linear_blits.c | 1 -
>  tests/gem_render_tiled_blits.c  | 1 -
>  tests/gem_wait_render_timeout.c | 1 -
>  tests/kms_setmode.c             | 1 -
>  tests/pm_rps.c                  | 1 -
>  14 files changed, 14 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c index 7be2e40..f921f67 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -39,7 +39,6 @@
>  #include <sys/mman.h>
>  #include <signal.h>
>  #include <pciaccess.h>
> -#include <getopt.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <sys/wait.h>
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c index 7b277be..0bcaa3b 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -39,7 +39,6 @@
>  #include <sys/mman.h>
>  #include <signal.h>
>  #include <pciaccess.h>
> -#include <getopt.h>
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <sys/wait.h>
> diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c index
> 7b3104c..afe7562 100644
> --- a/lib/rendercopy_gen6.c
> +++ b/lib/rendercopy_gen6.c
> @@ -9,7 +9,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
>  #include "drm.h"
>  #include "i915_drm.h"
>  #include "drmtest.h"
> diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c index
> 5131d8f..2932f1e 100644
> --- a/lib/rendercopy_gen7.c
> +++ b/lib/rendercopy_gen7.c
> @@ -9,7 +9,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
>  #include "drm.h"
>  #include "i915_drm.h"
>  #include "drmtest.h"
> diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c index
> a32871d..ff0b0c8 100644
> --- a/lib/rendercopy_gen8.c
> +++ b/lib/rendercopy_gen8.c
> @@ -9,7 +9,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
> 
>  #include <drm.h>
>  #include <i915_drm.h>
> diff --git a/lib/rendercopy_i830.c b/lib/rendercopy_i830.c index
> f0235a5..04215b1 100644
> --- a/lib/rendercopy_i830.c
> +++ b/lib/rendercopy_i830.c
> @@ -8,7 +8,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
>  #include "drm.h"
>  #include "i915_drm.h"
>  #include "drmtest.h"
> diff --git a/lib/rendercopy_i915.c b/lib/rendercopy_i915.c index
> 1acf9da..fc9583c 100644
> --- a/lib/rendercopy_i915.c
> +++ b/lib/rendercopy_i915.c
> @@ -8,7 +8,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
>  #include "drm.h"
>  #include "i915_drm.h"
>  #include "drmtest.h"
> diff --git a/tests/gem_media_fill.c b/tests/gem_media_fill.c index
> db2380b..b06a556 100644
> --- a/tests/gem_media_fill.c
> +++ b/tests/gem_media_fill.c
> @@ -41,7 +41,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
>  #include "drm.h"
>  #include "ioctl_wrappers.h"
>  #include "drmtest.h"
> diff --git a/tests/gem_render_copy_redux.c
> b/tests/gem_render_copy_redux.c index f711fdb..cb48aa7 100644
> --- a/tests/gem_render_copy_redux.c
> +++ b/tests/gem_render_copy_redux.c
> @@ -43,7 +43,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
> 
>  #include <drm.h>
> 
> diff --git a/tests/gem_render_linear_blits.c
> b/tests/gem_render_linear_blits.c index ee99dea..28fd8c8 100644
> --- a/tests/gem_render_linear_blits.c
> +++ b/tests/gem_render_linear_blits.c
> @@ -46,7 +46,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
> 
>  #include <drm.h>
> 
> diff --git a/tests/gem_render_tiled_blits.c b/tests/gem_render_tiled_blits.c
> index 3d83f7c..ea1d59d 100644
> --- a/tests/gem_render_tiled_blits.c
> +++ b/tests/gem_render_tiled_blits.c
> @@ -42,7 +42,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
> 
>  #include <drm.h>
> 
> diff --git a/tests/gem_wait_render_timeout.c
> b/tests/gem_wait_render_timeout.c index a34c006..3afab9c 100644
> --- a/tests/gem_wait_render_timeout.c
> +++ b/tests/gem_wait_render_timeout.c
> @@ -36,7 +36,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
> 
>  #include <drm.h>
> 
> diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c index
> 0b765a9..8762255 100644
> --- a/tests/kms_setmode.c
> +++ b/tests/kms_setmode.c
> @@ -29,7 +29,6 @@
>  #include <stdint.h>
>  #include <unistd.h>
>  #include <string.h>
> -#include <getopt.h>
>  #include <sys/time.h>
> 
>  #include "drmtest.h"
> diff --git a/tests/pm_rps.c b/tests/pm_rps.c index c1156a5..96fec99 100644
> --- a/tests/pm_rps.c
> +++ b/tests/pm_rps.c
> @@ -31,7 +31,6 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> -#include <getopt.h>
>  #include <fcntl.h>
>  #include <signal.h>
>  #include <errno.h>
> --
> 1.9.3

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

[Gore, Tim]

Looks fine.

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 3/8] lib: move option parsing into common_init
> 
> 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>
Reviewed-by: Tim Gore <tim.gore@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);
> +	}
> +
> +	if (ret < 0)
> +		/* exit with no error for -h/--help */
> +		exit(ret == -1 ? 0 : ret);
> +
>  	print_version();
> 
>  	return ret;
>  }
> 
> -enum igt_log_level igt_log_level = IGT_LOG_INFO;
> 
> -static void common_init(void)
> +/**
> + * 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)
>  {
> -	char *env = getenv("IGT_LOG_LEVEL");
> +	int ret;
> 
> -	if (!env)
> -		return;
> +	test_with_subtests = true;
> +	ret = common_init(argc, argv, extra_short_opts, extra_long_opts,
> +			  help_str, extra_opt_handler);
> +	igt_install_exit_handler(check_igt_exit);
> 
> -	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;
> +	return ret;
>  }
> 
> +enum igt_log_level igt_log_level = IGT_LOG_INFO;
> +
>  /**
>   * igt_subtest_init:
>   * @argc: argc from the test's main()
> @@ -446,8 +471,6 @@ void igt_subtest_init(int argc, char **argv)
> 
>  	/* reset opt parsing */
>  	optind = 1;
> -
> -	common_init();
>  }
> 
>  /**
> @@ -463,11 +486,18 @@ void igt_subtest_init(int argc, char **argv)
>   */
>  void igt_simple_init(int argc, char **argv)  {
> -	print_version();
> +	int ret;
> 
> -	oom_adjust_for_doom();
> +	/* supress getopt errors about unknown options */
> +	opterr = 0;
> +
> +	ret = common_init(argc, argv, NULL, NULL, NULL, NULL);
> +	if (ret < 0)
> +		/* exit with no error for -h/--help */
> +		exit(ret == -1 ? 0 : ret);
> 
> -	common_init();
> +	/* reset opt parsing */
> +	optind = 1;
>  }
> 
>  /*
> --
> 1.9.3

Re: [PATCH i-g-t 4/8] lib: add igt_simple_init_parse_opts

[Gore, Tim]

Looks fine.

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 4/8] lib: add igt_simple_init_parse_opts
> 
> This function allows simple tests to register additional command line options.
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
Reviewed-by: Tim Gore <tim.gore@intel.com>
> ---
>  lib/igt_core.c | 22 ++++++++++++++++++++++  lib/igt_core.h |  5 +++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c index ee6f90c..72c77e6 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -500,6 +500,28 @@ void igt_simple_init(int argc, char **argv)
>  	optind = 1;
>  }
> 
> +/**
> + * igt_simple_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 initializes a simple test without any support for subtests and
> +allows
> + * an arbitrary set of additional options.
> + */
> +void igt_simple_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) {
> +	common_init(argc, argv, extra_short_opts, extra_long_opts,
> help_str,
> +		    extra_opt_handler);
> +}
> +
>  /*
>   * Note: Testcases which use these helpers MUST NOT output anything to
> stdout
>   * outside of places protected by igt_run_subtest checks - the piglit diff --git
> a/lib/igt_core.h b/lib/igt_core.h index 6138487..408cf3a 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -162,6 +162,11 @@ bool igt_only_list_subtests(void);
>  	static void igt_tokencat(__real_main, __LINE__)(void) \
> 
>  void igt_simple_init(int argc, char **argv);
> +void igt_simple_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);
> 
>  /**
>   * igt_simple_main:
> --
> 1.9.3

Re: [PATCH i-g-t 5/8] lib: don't ignore unknown options in multi-tests

[Gore, Tim]

Looks sensible.

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 5/8] lib: don't ignore unknown options in multi-tests
> 
> None of the current tests have additional options that might make use of any
> unknown options and igt_subtest_init_parse_opts is available that integrates
> additional option parsing.
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
Reviewed-by: Tim Gore <tim.gore@intel.com>
> ---
>  lib/igt_core.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c index 72c77e6..0867c27 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -459,18 +459,7 @@ enum igt_log_level igt_log_level = IGT_LOG_INFO;
>   */
>  void igt_subtest_init(int argc, char **argv)  {
> -	int ret;
> -
> -	/* supress getopt errors about unknown options */
> -	opterr = 0;
> -
> -	ret = igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL,
> NULL);
> -	if (ret < 0)
> -		/* exit with no error for -h/--help */
> -		exit(ret == -1 ? 0 : ret);
> -
> -	/* reset opt parsing */
> -	optind = 1;
> +	igt_subtest_init_parse_opts(argc, argv, NULL, NULL, NULL, NULL);
>  }
> 
>  /**
> --
> 1.9.3

Re: [PATCH i-g-t 6/8] tests: convert simple tests to use igt_simple_init_parse_opts

[Gore, Tim]

There is a problem here with the -r option in gem_seqno_wrap, as this conflicts
with the --run-subtest option in common_init. This is a general issue with this
method of splitting the responsibility for parsing the command line. As the
common command line options defined in common_init (--run-subtest etc)
do not support short options, a simple solution might be to use some non
printing characters for their getopt return codes, eg

    {"list-subtests", 0, 0, '\x0c'}, ....

This way, individual tests are unlikely to clash with these. (I chose '\x0c' since
'l' is the twelfth character ?) I tried this locally and it seemed to work.

  Tim

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 6/8] tests: convert simple tests to use
> igt_simple_init_parse_opts
> 
> Convert simple tests to use igt_simple_init_parse_opts if they require extra
> options.
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  lib/igt_core.h          |   1 +
>  tests/gem_ctx_basic.c   |  20 +++------
>  tests/gem_render_copy.c |  25 +++++-------  tests/gem_seqno_wrap.c  |  82
> +++++++++++++++----------------------
>  tests/gem_stress.c      | 105 ++++++++++++++++++++++------------------------
> --
>  5 files changed, 98 insertions(+), 135 deletions(-)
> 
> diff --git a/lib/igt_core.h b/lib/igt_core.h index 408cf3a..b19a897 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -36,6 +36,7 @@
>  #include <string.h>
>  #include <sys/types.h>
>  #include <stdarg.h>
> +#include <getopt.h>
> 
>  /**
>   * IGT_EXIT_TIMEOUT:
> diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c index
> a2464fd..a0bec60 100644
> --- a/tests/gem_ctx_basic.c
> +++ b/tests/gem_ctx_basic.c
> @@ -39,7 +39,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
>  #include "drm.h"
>  #include "ioctl_wrappers.h"
>  #include "drmtest.h"
> @@ -119,11 +118,9 @@ static void *work(void *arg)
>  	pthread_exit(NULL);
>  }
> 
> -static void parse(int argc, char *argv[])
> +static int opt_handler(int opt, int opt_index)
>  {
> -	int opt;
> -	while ((opt = getopt(argc, argv, "i:c:n:muh?")) != -1) {
> -		switch (opt) {
> +	switch (opt) {
>  		case 'i':
>  			iter = atoi(optarg);
>  			break;
> @@ -136,20 +133,17 @@ static void parse(int argc, char *argv[])
>  		case 'u':
>  			uncontexted = 1;
>  			break;
> -		case 'h':
> -		case '?':
> -		default:
> -			igt_success();
> -			break;
> -		}
>  	}
> +
> +	return 0;
>  }
> 
>  int main(int argc, char *argv[])
>  {
>  	int i;
> 
> -	igt_simple_init(argc, argv);
> +	igt_simple_init_parse_opts(argc, argv, "i:c:n:mu", NULL, NULL,
> +				   opt_handler);
> 
>  	fd = drm_open_any_render();
>  	devid = intel_get_drm_devid(fd);
> @@ -159,8 +153,6 @@ int main(int argc, char *argv[])
>  		iter = 4;
>  	}
> 
> -	parse(argc, argv);
> -
>  	threads = calloc(num_contexts, sizeof(*threads));
> 
>  	for (i = 0; i < num_contexts; i++)
> diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c index
> 76ba40e..6ff0c77 100644
> --- a/tests/gem_render_copy.c
> +++ b/tests/gem_render_copy.c
> @@ -41,7 +41,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
> 
>  #include <drm.h>
> 
> @@ -67,6 +66,7 @@ typedef struct {
>  	drm_intel_bufmgr *bufmgr;
>  	uint32_t linear[WIDTH * HEIGHT];
>  } data_t;
> +static int opt_dump_png = false;
> 
>  static void scratch_buf_write_to_png(struct igt_buf *buf, const char
> *filename)  { @@ -117,27 +117,24 @@ scratch_buf_check(data_t *data,
> struct igt_buf *buf, int x, int y,
>  		     color, val, x, y);
>  }
> 
> +static int opt_handler(int opt, int opt_index) {
> +	if (opt == 'd') {
> +		opt_dump_png = true;
> +	}
> +
> +	return 0;
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	data_t data = {0, };
>  	struct intel_batchbuffer *batch = NULL;
>  	struct igt_buf src, dst;
>  	igt_render_copyfunc_t render_copy = NULL;
> -	int opt;
> -	int opt_dump_png = false;
>  	int opt_dump_aub = igt_aub_dump_enabled();
> 
> -	igt_simple_init(argc, argv);
> -
> -	while ((opt = getopt(argc, argv, "d")) != -1) {
> -		switch (opt) {
> -		case 'd':
> -			opt_dump_png = true;
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> +	igt_simple_init_parse_opts(argc, argv, "d", NULL, NULL,
> opt_handler);
> 
>  	igt_fixture {
>  		data.drm_fd = drm_open_any_render();
> diff --git a/tests/gem_seqno_wrap.c b/tests/gem_seqno_wrap.c index
> 0fa722d..3a40860 100644
> --- a/tests/gem_seqno_wrap.c
> +++ b/tests/gem_seqno_wrap.c
> @@ -38,7 +38,6 @@
>  #include <sys/types.h>
>  #include <sys/wait.h>
>  #include <limits.h>
> -#include <getopt.h>
>  #include <signal.h>
>  #include <errno.h>
> 
> @@ -452,45 +451,9 @@ static void background_run_once(void)
>  		sleep(3);
>  }
> 
> -static void print_usage(const char *s)
> +static int parse_options(int opt, int opt_index)
>  {
> -	igt_info("%s: [OPTION]...\n", s);
> -	igt_info("    where options are:\n");
> -	igt_info("    -b --background       run in background inducing wraps\n");
> -	igt_info("    -n --rounds=num       run num times across wrap
> boundary, 0 == forever\n");
> -	igt_info("    -t --timeout=sec      set timeout to wait for testrun to sec
> seconds\n");
> -	igt_info("    -d --dontwrap         don't wrap just run the test\n");
> -	igt_info("    -p --prewrap=n        set seqno to WRAP - n for each
> testrun\n");
> -	igt_info("    -r --norandom         dont randomize prewrap space\n");
> -	igt_info("    -i --buffers          number of buffers to copy\n");
> -	igt_fail(-1);
> -}
> -
> -static void parse_options(int argc, char **argv) -{
> -	int c;
> -	int option_index = 0;
> -	static struct option long_options[] = {
> -		{"rounds", required_argument, 0, 'n'},
> -		{"background", no_argument, 0, 'b'},
> -		{"timeout", required_argument, 0, 't'},
> -		{"dontwrap", no_argument, 0, 'd'},
> -		{"prewrap", required_argument, 0, 'p'},
> -		{"norandom", no_argument, 0, 'r'},
> -		{"buffers", required_argument, 0, 'i'},
> -	};
> -
> -	options.rounds = SLOW_QUICK(50, 2);
> -	options.background = 0;
> -	options.dontwrap = 0;
> -	options.timeout = 20;
> -	options.random = 1;
> -	options.prewrap_space = 21;
> -	options.buffers = 10;
> -
> -	while((c = getopt_long(argc, argv, "n:bvt:dp:ri:",
> -			       long_options, &option_index)) != -1) {
> -		switch(c) {
> +	switch(opt) {
>  		case 'b':
>  			options.background = 1;
>  			igt_info("running in background inducing wraps\n");
> @@ -520,17 +483,9 @@ static void parse_options(int argc, char **argv)
>  			options.prewrap_space = atoi(optarg);
>  			igt_info("prewrap set to %d (0x%x)\n",
> options.prewrap_space, UINT32_MAX - options.prewrap_space);
>  			break;
> -		default:
> -			igt_info("unkown command options\n");
> -			print_usage(argv[0]);
> -			break;
> -		}
>  	}
> 
> -	if (optind < argc) {
> -		igt_info("unkown command options\n");
> -		print_usage(argv[0]);
> -	}
> +	return 0;
>  }
> 
>  int main(int argc, char **argv)
> @@ -538,9 +493,36 @@ int main(int argc, char **argv)
>  	int wcount = 0;
>  	int r = -1;
> 
> -	igt_simple_init(argc, argv);
> +	static struct option long_options[] = {
> +		{"rounds", required_argument, 0, 'n'},
> +		{"background", no_argument, 0, 'b'},
> +		{"timeout", required_argument, 0, 't'},
> +		{"dontwrap", no_argument, 0, 'd'},
> +		{"prewrap", required_argument, 0, 'p'},
> +		{"norandom", no_argument, 0, 'r'},
> +		{"buffers", required_argument, 0, 'i'},
> +		{ 0, 0, 0, 0 }
> +	};
> +
> +	const char *help =
> +		"  -b --background       run in background inducing wraps\n"
> +		"  -n --rounds=num       run num times across wrap boundary,
> 0 == forever\n"
> +		"  -t --timeout=sec      set timeout to wait for testrun to sec
> seconds\n"
> +		"  -d --dontwrap         don't wrap just run the test\n"
> +		"  -p --prewrap=n        set seqno to WRAP - n for each
> testrun\n"
> +		"  -r --norandom         dont randomize prewrap space\n"
> +		"  -i --buffers          number of buffers to copy\n";
> +
> +	options.rounds = SLOW_QUICK(50, 2);
> +	options.background = 0;
> +	options.dontwrap = 0;
> +	options.timeout = 20;
> +	options.random = 1;
> +	options.prewrap_space = 21;
> +	options.buffers = 10;
> 
> -	parse_options(argc, argv);
> +	igt_simple_init_parse_opts(argc, argv, "n:bvt:dp:ri:", long_options,
> +				   help, parse_options);
> 
>  	card_index = drm_get_card();
> 
> diff --git a/tests/gem_stress.c b/tests/gem_stress.c index c8d7393..3bbe487
> 100644
> --- a/tests/gem_stress.c
> +++ b/tests/gem_stress.c
> @@ -58,7 +58,6 @@
>  #include <errno.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
> -#include <getopt.h>
> 
>  #include <drm.h>
> 
> @@ -72,6 +71,10 @@
> 
>  #define CMD_POLY_STIPPLE_OFFSET       0x7906
> 
> +#define DUCTAPE 0xdead0001
> +#define TILESZ	0xdead0002
> +#define CHCK_RENDER 0xdead0003
> +
>  /** TODO:
>   * - beat on relaxed fencing (i.e. mappable/fenceable tracking in the kernel)
>   * - render copy (to check fence tracking and cache coherency management
> by the @@ -624,54 +627,11 @@ static void sanitize_tiles_per_buf(void)
>  		options.tiles_per_buf = options.scratch_buf_size /
> TILE_BYTES(options.tile_size);  }
> 
> -static void parse_options(int argc, char **argv)
> +static int parse_options(int opt, int opt_index)
>  {
> -	int c, tmp;
> -	int option_index = 0;
> -	static struct option long_options[] = {
> -		{"no-hw", 0, 0, 'd'},
> -		{"buf-size", 1, 0, 's'},
> -		{"gpu-busy-load", 1, 0, 'g'},
> -		{"no-signals", 0, 0, 'S'},
> -		{"buffer-count", 1, 0, 'c'},
> -		{"trace-tile", 1, 0, 't'},
> -		{"disable-blt", 0, 0, 'b'},
> -		{"disable-render", 0, 0, 'r'},
> -		{"untiled", 0, 0, 'u'},
> -		{"x-tiled", 0, 0, 'x'},
> -		{"use-cpu-maps", 0, 0, 'm'},
> -		{"rounds", 1, 0, 'o'},
> -		{"no-fail", 0, 0, 'f'},
> -		{"tiles-per-buf", 0, 0, 'p'},
> -#define DUCTAPE 0xdead0001
> -		{"remove-duct-tape", 0, 0, DUCTAPE},
> -#define TILESZ	0xdead0002
> -		{"tile-size", 1, 0, TILESZ},
> -#define CHCK_RENDER 0xdead0003
> -		{"check-render-cpyfn", 0, 0, CHCK_RENDER},
> -		{NULL, 0, 0, 0},
> -	};
> -
> -	options.scratch_buf_size = 256*4096;
> -	options.no_hw = 0;
> -	options.use_signal_helper = 1;
> -	options.gpu_busy_load = 0;
> -	options.num_buffers = 0;
> -	options.trace_tile = -1;
> -	options.use_render = 1;
> -	options.use_blt = 1;
> -	options.forced_tiling = -1;
> -	options.use_cpu_maps = 0;
> -	options.total_rounds = 512;
> -	options.fail = 1;
> -	options.ducttape = 1;
> -	options.tile_size = 16;
> -	options.tiles_per_buf = options.scratch_buf_size /
> TILE_BYTES(options.tile_size);
> -	options.check_render_cpyfn = 0;
> +	int tmp;
> 
> -	while((c = getopt_long(argc, argv, "ds:g:c:t:rbuxmo:fp:",
> -			       long_options, &option_index)) != -1) {
> -		switch(c) {
> +	switch(opt) {
>  		case 'd':
>  			options.no_hw = 1;
>  			igt_info("no-hw debug mode\n");
> @@ -759,15 +719,8 @@ static void parse_options(int argc, char **argv)
>  			options.check_render_cpyfn = 1;
>  			igt_info("checking render copy function\n");
>  			break;
> -		default:
> -			igt_info("unkown command options\n");
> -			break;
> -		}
>  	}
> 
> -	if (optind < argc)
> -		igt_info("unkown command options\n");
> -
>  	/* actually 32767, according to docs, but that kills our nice pot
> calculations. */
>  	options.max_dimension = 16*1024;
>  	if (options.use_render) {
> @@ -777,6 +730,8 @@ static void parse_options(int argc, char **argv)
>  			options.max_dimension = 8192;
>  	}
>  	igt_info("Limiting buffer to %dx%d\n", options.max_dimension,
> options.max_dimension);
> +
> +	return 0;
>  }
> 
>  static void init(void)
> @@ -864,14 +819,50 @@ int main(int argc, char **argv)  {
>  	int i, j;
>  	unsigned *current_permutation, *tmp_permutation;
> +	static struct option long_options[] = {
> +		{"no-hw", 0, 0, 'd'},
> +		{"buf-size", 1, 0, 's'},
> +		{"gpu-busy-load", 1, 0, 'g'},
> +		{"no-signals", 0, 0, 'S'},
> +		{"buffer-count", 1, 0, 'c'},
> +		{"trace-tile", 1, 0, 't'},
> +		{"disable-blt", 0, 0, 'b'},
> +		{"disable-render", 0, 0, 'r'},
> +		{"untiled", 0, 0, 'u'},
> +		{"x-tiled", 0, 0, 'x'},
> +		{"use-cpu-maps", 0, 0, 'm'},
> +		{"rounds", 1, 0, 'o'},
> +		{"no-fail", 0, 0, 'f'},
> +		{"tiles-per-buf", 0, 0, 'p'},
> +		{"remove-duct-tape", 0, 0, DUCTAPE},
> +		{"tile-size", 1, 0, TILESZ},
> +		{"check-render-cpyfn", 0, 0, CHCK_RENDER},
> +		{NULL, 0, 0, 0},
> +	};
> +
> +	options.scratch_buf_size = 256*4096;
> +	options.no_hw = 0;
> +	options.use_signal_helper = 1;
> +	options.gpu_busy_load = 0;
> +	options.num_buffers = 0;
> +	options.trace_tile = -1;
> +	options.use_render = 1;
> +	options.use_blt = 1;
> +	options.forced_tiling = -1;
> +	options.use_cpu_maps = 0;
> +	options.total_rounds = 512;
> +	options.fail = 1;
> +	options.ducttape = 1;
> +	options.tile_size = 16;
> +	options.tiles_per_buf = options.scratch_buf_size /
> TILE_BYTES(options.tile_size);
> +	options.check_render_cpyfn = 0;
> 
> -	igt_simple_init(argc, argv);
> +	igt_simple_init_parse_opts(argc, argv,"ds:g:c:t:rbuxmo:fp:",
> +				   long_options, NULL, parse_options);
> 
>  	drm_fd = drm_open_any();
>  	devid = intel_get_drm_devid(drm_fd);
> 
> -	parse_options(argc, argv);
> -
>  	/* start our little helper early before too may allocations occur */
>  	if (options.use_signal_helper)
>  		igt_fork_signal_helper();
> --
> 1.9.3

Re: [PATCH i-g-t 7/8] lib: always warn about unknown options

[Gore, Tim]

Yes, this looks sensible.

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 7/8] lib: always warn about unknown options
> 
> All tests can now register extra options, so there should not be any unknown
> options.
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
Reviewed-by: Tim Gore <tim.gore@intel.com>

> ---
>  lib/igt_core.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c index 0867c27..42b22fc 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -362,17 +362,9 @@ static int common_init(int argc, char **argv,
>  			ret = -1;
>  			goto out;
>  		case '?':
> -			if (opterr) {
> -				print_usage(command_str, help_str, true);
> -				ret = -2;
> -				goto out;
> -			}
> -			/*
> -			 * Just ignore the error, since the unknown argument
> -			 * can be something the caller understands and will
> -			 * parse by doing a second getopt scanning.
> -			 */
> -			break;
> +			print_usage(command_str, help_str, true);
> +			ret = -2;
> +			goto out;
>  		default:
>  			ret = extra_opt_handler(c, option_index);
>  			if (ret)
> @@ -475,18 +467,7 @@ void igt_subtest_init(int argc, char **argv)
>   */
>  void igt_simple_init(int argc, char **argv)  {
> -	int ret;
> -
> -	/* supress getopt errors about unknown options */
> -	opterr = 0;
> -
> -	ret = common_init(argc, argv, NULL, NULL, NULL, NULL);
> -	if (ret < 0)
> -		/* exit with no error for -h/--help */
> -		exit(ret == -1 ? 0 : ret);
> -
> -	/* reset opt parsing */
> -	optind = 1;
> +	common_init(argc, argv, NULL, NULL, NULL, NULL);
>  }
> 
>  /**
> --
> 1.9.3

Re: [PATCH i-g-t 8/8] lib: add a command line option to enable debug output in tests

[Gore, Tim]

See my comment on patch 6.  --debug returns 'd' from getopt, which can conflict
with the short options for some tests, such as gem_render_copy and gem_seqn0_wrap.
This problem also exists for the --run-subtest.

  Tim

> -----Original Message-----
> From: Thomas Wood [mailto:thomas.wood@intel.com]
> Sent: Wednesday, July 23, 2014 11:58 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Gore, Tim; daniel.vetter@ffwll.ch
> Subject: [PATCH i-g-t 8/8] lib: add a command line option to enable debug
> output in tests
> 
> Add --debug as a common command line option for all tests to enable debug
> output.
> 
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  lib/igt_core.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c index 42b22fc..d90e6bb 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -276,7 +276,9 @@ static void print_usage(const char *command_str,
> const char *help_str,
> 
>  	fprintf(f, "Usage: %s [OPTIONS]\n"
>  		   "  --list-subtests\n"
> -		   "  --run-subtest <pattern>\n", command_str);
> +		   "  --run-subtest <pattern>\n"
> +		   "  --debug\n"
> +		   "  --help\n", command_str);
>  	if (help_str)
>  		fprintf(f, "%s\n", help_str);
>  }
> @@ -301,6 +303,7 @@ static int common_init(int argc, char **argv,
>  	static struct option long_options[] = {
>  		{"list-subtests", 0, 0, 'l'},
>  		{"run-subtest", 1, 0, 'r'},
> +		{"debug", 0, 0, 'd'},
>  		{"help", 0, 0, 'h'},
>  	};
>  	const char *command_str;
> @@ -349,6 +352,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 'd':
> +			igt_log_level = IGT_LOG_DEBUG;
> +			break;
>  		case 'l':
>  			if (!run_single_subtest)
>  				list_subtests = true;
> --
> 1.9.3

[PATCH i-g-t] lib: avoid getopt value conflicts with tests

[Thomas Wood]

Most tests use a printable character as the value for getopt to return,
so avoid conflicts by using non-printing values for the standard options.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index a0c9499..882614a 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -218,6 +218,13 @@ int num_test_children;
 int test_children_sz;
 bool test_child;
 
+enum {
+ OPT_LIST_SUBTESTS,
+ OPT_RUN_SUBTEST,
+ OPT_DEBUG,
+ OPT_HELP
+};
+
 __attribute__((format(printf, 1, 2)))
 static void kmsg(const char *format, ...)
 #define KERN_INFO "<5>"
@@ -320,10 +327,10 @@ static int common_init(int argc, char **argv,
 {
 	int c, option_index = 0;
 	static struct option long_options[] = {
-		{"list-subtests", 0, 0, 'l'},
-		{"run-subtest", 1, 0, 'r'},
-		{"debug", 0, 0, 'd'},
-		{"help", 0, 0, 'h'},
+		{"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
+		{"run-subtest", 1, 0, OPT_RUN_SUBTEST},
+		{"debug", 0, 0, OPT_DEBUG},
+		{"help", 0, 0, OPT_HELP},
 	};
 	char *short_opts;
 	struct option *combined_opts;
@@ -370,18 +377,18 @@ static int common_init(int argc, char **argv,
 	while ((c = getopt_long(argc, argv, short_opts, combined_opts,
 			       &option_index)) != -1) {
 		switch(c) {
-		case 'd':
+		case OPT_DEBUG:
 			igt_log_level = IGT_LOG_DEBUG;
 			break;
-		case 'l':
+		case OPT_LIST_SUBTESTS:
 			if (!run_single_subtest)
 				list_subtests = true;
 			break;
-		case 'r':
+		case OPT_RUN_SUBTEST:
 			if (!list_subtests)
 				run_single_subtest = strdup(optarg);
 			break;
-		case 'h':
+		case OPT_HELP:
 			print_usage(help_str, false);
 			ret = -1;
 			goto out;
-- 
1.9.3

Re: [PATCH i-g-t] lib: avoid getopt value conflicts with tests

[Paulo Zanoni]

2014-07-25 13:08 GMT-03:00 Thomas Wood <thomas.wood@intel.com>:
> Most tests use a printable character as the value for getopt to return,
> so avoid conflicts by using non-printing values for the standard options.

Instead of this patch, isn't there any way to verify if the tests are
using any character that is "reserved" to these general options?
Having "-r" instead of "--run-subtest" is quite nice. That said, I'm
not against your patch either.

>
> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
> ---
>  lib/igt_core.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index a0c9499..882614a 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -218,6 +218,13 @@ int num_test_children;
>  int test_children_sz;
>  bool test_child;
>
> +enum {
> + OPT_LIST_SUBTESTS,
> + OPT_RUN_SUBTEST,
> + OPT_DEBUG,
> + OPT_HELP
> +};
> +
>  __attribute__((format(printf, 1, 2)))
>  static void kmsg(const char *format, ...)
>  #define KERN_INFO "<5>"
> @@ -320,10 +327,10 @@ static int common_init(int argc, char **argv,
>  {
>         int c, option_index = 0;
>         static struct option long_options[] = {
> -               {"list-subtests", 0, 0, 'l'},
> -               {"run-subtest", 1, 0, 'r'},
> -               {"debug", 0, 0, 'd'},
> -               {"help", 0, 0, 'h'},
> +               {"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
> +               {"run-subtest", 1, 0, OPT_RUN_SUBTEST},
> +               {"debug", 0, 0, OPT_DEBUG},
> +               {"help", 0, 0, OPT_HELP},
>         };
>         char *short_opts;
>         struct option *combined_opts;
> @@ -370,18 +377,18 @@ static int common_init(int argc, char **argv,
>         while ((c = getopt_long(argc, argv, short_opts, combined_opts,
>                                &option_index)) != -1) {
>                 switch(c) {
> -               case 'd':
> +               case OPT_DEBUG:
>                         igt_log_level = IGT_LOG_DEBUG;
>                         break;
> -               case 'l':
> +               case OPT_LIST_SUBTESTS:
>                         if (!run_single_subtest)
>                                 list_subtests = true;
>                         break;
> -               case 'r':
> +               case OPT_RUN_SUBTEST:
>                         if (!list_subtests)
>                                 run_single_subtest = strdup(optarg);
>                         break;
> -               case 'h':
> +               case OPT_HELP:
>                         print_usage(help_str, false);
>                         ret = -1;
>                         goto out;
> --
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH i-g-t] lib: avoid getopt value conflicts with tests

[Thomas Wood]

On 25 July 2014 17:57, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-07-25 13:08 GMT-03:00 Thomas Wood <thomas.wood@intel.com>:
>> Most tests use a printable character as the value for getopt to return,
>> so avoid conflicts by using non-printing values for the standard options.
>
> Instead of this patch, isn't there any way to verify if the tests are
> using any character that is "reserved" to these general options?
> Having "-r" instead of "--run-subtest" is quite nice. That said, I'm
> not against your patch either.

The only standard short option is "-h" for "--help", which needs to be
fixed in the patch. Adding a warning for conflicting short options and
getopt return values could be added, but probably in a separate patch.


>
>>
>> Signed-off-by: Thomas Wood <thomas.wood@intel.com>
>> ---
>>  lib/igt_core.c | 23 +++++++++++++++--------
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/igt_core.c b/lib/igt_core.c
>> index a0c9499..882614a 100644
>> --- a/lib/igt_core.c
>> +++ b/lib/igt_core.c
>> @@ -218,6 +218,13 @@ int num_test_children;
>>  int test_children_sz;
>>  bool test_child;
>>
>> +enum {
>> + OPT_LIST_SUBTESTS,
>> + OPT_RUN_SUBTEST,
>> + OPT_DEBUG,
>> + OPT_HELP
>> +};
>> +
>>  __attribute__((format(printf, 1, 2)))
>>  static void kmsg(const char *format, ...)
>>  #define KERN_INFO "<5>"
>> @@ -320,10 +327,10 @@ static int common_init(int argc, char **argv,
>>  {
>>         int c, option_index = 0;
>>         static struct option long_options[] = {
>> -               {"list-subtests", 0, 0, 'l'},
>> -               {"run-subtest", 1, 0, 'r'},
>> -               {"debug", 0, 0, 'd'},
>> -               {"help", 0, 0, 'h'},
>> +               {"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
>> +               {"run-subtest", 1, 0, OPT_RUN_SUBTEST},
>> +               {"debug", 0, 0, OPT_DEBUG},
>> +               {"help", 0, 0, OPT_HELP},
>>         };
>>         char *short_opts;
>>         struct option *combined_opts;
>> @@ -370,18 +377,18 @@ static int common_init(int argc, char **argv,
>>         while ((c = getopt_long(argc, argv, short_opts, combined_opts,
>>                                &option_index)) != -1) {
>>                 switch(c) {
>> -               case 'd':
>> +               case OPT_DEBUG:
>>                         igt_log_level = IGT_LOG_DEBUG;
>>                         break;
>> -               case 'l':
>> +               case OPT_LIST_SUBTESTS:
>>                         if (!run_single_subtest)
>>                                 list_subtests = true;
>>                         break;
>> -               case 'r':
>> +               case OPT_RUN_SUBTEST:
>>                         if (!list_subtests)
>>                                 run_single_subtest = strdup(optarg);
>>                         break;
>> -               case 'h':
>> +               case OPT_HELP:
>>                         print_usage(help_str, false);
>>                         ret = -1;
>>                         goto out;
>> --
>> 1.9.3
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>

[PATCH i-g-t] lib: check test options for conflicts

[Thomas Wood]

Check any test specific options for conflicts with the standard set of
options.

Signed-off-by: Thomas Wood <thomas.wood@intel.com>
---
 lib/igt_core.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 0e254e3..f4761ca 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -325,14 +325,16 @@ static int common_init(int argc, char **argv,
 		       const char *help_str,
 		       igt_opt_handler_t extra_opt_handler)
 {
-	int c, option_index = 0;
+	int c, option_index = 0, i, x;
 	static struct option long_options[] = {
 		{"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
 		{"run-subtest", 1, 0, OPT_RUN_SUBTEST},
 		{"debug", 0, 0, OPT_DEBUG},
 		{"help", 0, 0, OPT_HELP},
+		{0, 0, 0, 0}
 	};
 	char *short_opts;
+	const char *std_short_opts = "h";
 	struct option *combined_opts;
 	int extra_opt_count;
 	int all_opt_count;
@@ -356,10 +358,45 @@ static int common_init(int argc, char **argv,
 
 	/* First calculate space for all passed-in extra long options */
 	all_opt_count = 0;
-	while (extra_long_opts && extra_long_opts[all_opt_count].name)
+	while (extra_long_opts && extra_long_opts[all_opt_count].name) {
+
+		/* check for conflicts with standard long option values */
+		for (i = 0; long_options[i].name; i++)
+			if (extra_long_opts[all_opt_count].val == long_options[i].val)
+				igt_warn("Conflicting long option values between --%s and --%s\n",
+					 extra_long_opts[all_opt_count].name,
+					 long_options[i].name);
+
+		/* check for conflicts with short options */
+		if (extra_long_opts[all_opt_count].val != ':'
+		    && strchr(std_short_opts, extra_long_opts[all_opt_count].val)) {
+			igt_warn("Conflicting long and short option values between --%s and -%s\n",
+				 extra_long_opts[all_opt_count].name,
+				 long_options[i].name);
+		}
+
+
 		all_opt_count++;
+	}
 	extra_opt_count = all_opt_count;
 
+	/* check for conflicts in extra short options*/
+	for (i = 0; extra_short_opts && extra_short_opts[i]; i++) {
+
+		if (extra_short_opts[i] == ':')
+			continue;
+
+		/* check for conflicts with standard short options */
+		if (strchr(std_short_opts, extra_short_opts[i]))
+			igt_warn("Conflicting short option: -%c\n", std_short_opts[i]);
+
+		/* check for conflicts with standard long option values */
+		for (x = 0; long_options[x].name; x++)
+			if (long_options[x].val == extra_short_opts[i])
+				igt_warn("Conflicting short option and long option value: --%s and -%c\n",
+					 long_options[x].name, extra_short_opts[i]);
+	}
+
 	all_opt_count += ARRAY_SIZE(long_options);
 
 	combined_opts = malloc(all_opt_count * sizeof(*combined_opts));
@@ -370,8 +407,9 @@ static int common_init(int argc, char **argv,
 	memcpy(&combined_opts[extra_opt_count], long_options,
 		ARRAY_SIZE(long_options) * sizeof(*combined_opts));
 
-	ret = asprintf(&short_opts, "%sh",
-		       extra_short_opts ? extra_short_opts : "");
+	ret = asprintf(&short_opts, "%s%s",
+		       extra_short_opts ? extra_short_opts : "",
+		       std_short_opts);
 	assert(ret >= 0);
 
 	while ((c = getopt_long(argc, argv, short_opts, combined_opts,
-- 
1.9.3