[igt-dev] [PATCH i-g-t] lib/igt_core: Rework extra options conflicts handling

* [igt-dev] [PATCH i-g-t] lib/igt_core: Rework extra options conflicts handling
@ 2019-05-16 13:02 Arkadiusz Hiler
  2019-05-16 14:25 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Arkadiusz Hiler @ 2019-05-16 13:02 UTC (permalink / raw)
  To: igt-dev; +Cc: Petri Latvala

This started simple, as a fixup for a warning:
  In file included from ../lib/drmtest.h:39,
                   from ../lib/igt_core.c:60:
  ../lib/igt_core.c: In function ‘common_init’:
  ../lib/igt_core.h:891:24: warning: ‘%s’ directive argument is null [-Wformat-overflow=]
    891 | #define igt_warn(f...) igt_log(IGT_LOG_DOMAIN, IGT_LOG_WARN, f)
        |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ../lib/igt_core.c:704:4: note: in expansion of macro ‘igt_warn’
    704 |    igt_warn("Conflicting long and short option values between --%s and -%s\n",
        |    ^~~~~~~~
  ../lib/igt_core.c:704:73: note: format string is defined here
    704 |    igt_warn("Conflicting long and short option values between --%s and -%s\n",
        |                                                                         ^~

But it ended up doing the following things:
  1. Promote all igt_warns to _critical and assert afterwards.
  2. Use for loop instead of a while-doing-for's-job.
  3. Streamline calculation of the option list sizes.
  4. Add checks for long option names.
  5. Log about "'val' representation" instead of confusing "value".
  6. Log correct things so we won't %s on a NULL.
  7. Write tests to confirm that it works.

Cc: Petri Latvala <petri.latvala@intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
---
 lib/igt_core.c                   | 72 +++++++++++++----------
 lib/tests/igt_conflicting_args.c | 99 ++++++++++++++++++++++++++++++++
 lib/tests/meson.build            |  1 +
 3 files changed, 141 insertions(+), 31 deletions(-)
 create mode 100644 lib/tests/igt_conflicting_args.c

diff --git a/lib/igt_core.c b/lib/igt_core.c
index d5d4fce2..6bbc805c 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -666,12 +666,12 @@ static int common_init(int *argc, char **argv,
 {
 	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},
-		{"help-description", 0, 0, OPT_DESCRIPTION},
-		{"debug", optional_argument, 0, OPT_DEBUG},
-		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
-		{"help", 0, 0, OPT_HELP},
+		{"list-subtests",     no_argument,       NULL, OPT_LIST_SUBTESTS},
+		{"run-subtest",       required_argument, NULL, OPT_RUN_SUBTEST},
+		{"help-description",  no_argument,       NULL, OPT_DESCRIPTION},
+		{"debug",             optional_argument, NULL, OPT_DEBUG},
+		{"interactive-debug", optional_argument, NULL, OPT_INTERACTIVE_DEBUG},
+		{"help",              no_argument,       NULL, OPT_HELP},
 		{0, 0, 0, 0}
 	};
 	char *short_opts;
@@ -687,48 +687,58 @@ static int common_init(int *argc, char **argv,
 	if (strrchr(command_str, '/'))
 		command_str = strrchr(command_str, '/') + 1;
 
-	/* 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) {
+	/* Check for conflicts and calculate space for passed-in extra long options */
+	for  (extra_opt_count = 0; extra_long_opts && extra_long_opts[extra_opt_count].name; extra_opt_count++) {
+		char *conflicting_char;
 
 		/* 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,
+		for (i = 0; long_options[i].name; i++) {
+			if (0 == strcmp(extra_long_opts[extra_opt_count].name, long_options[i].name)) {
+				igt_critical("Conflicting extra long option defined --%s\n", long_options[i].name);
+				assert(0);
+
+			}
+
+			if (extra_long_opts[extra_opt_count].val == long_options[i].val) {
+				igt_critical("Conflicting long option 'val' representation between --%s and --%s\n",
+					 extra_long_opts[extra_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);
+				assert(0);
+			}
 		}
 
-
-		all_opt_count++;
+		/* check for conflicts with standard short options */
+		if (extra_long_opts[extra_opt_count].val != ':'
+		    && (conflicting_char = strchr(std_short_opts, extra_long_opts[extra_opt_count].val))) {
+			igt_critical("Conflicting long and short option 'val' representation between --%s and -%c\n",
+				 extra_long_opts[extra_opt_count].name,
+				 *conflicting_char);
+			assert(0);
+		}
 	}
-	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]);
+		if (strchr(std_short_opts, extra_short_opts[i])) {
+			igt_critical("Conflicting short option: -%c\n", std_short_opts[i]);
+			assert(0);
+		}
 
 		/* 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]);
+		for (x = 0; long_options[x].name; x++) {
+			if (long_options[x].val == extra_short_opts[i]) {
+				igt_critical("Conflicting short option and long option 'val' representation: --%s and -%c\n",
+					     long_options[x].name, extra_short_opts[i]);
+				assert(0);
+			}
+		}
 	}
 
-	all_opt_count += ARRAY_SIZE(long_options);
+	all_opt_count = extra_opt_count + ARRAY_SIZE(long_options);
 
 	combined_opts = malloc(all_opt_count * sizeof(*combined_opts));
 	if (extra_opt_count > 0)
diff --git a/lib/tests/igt_conflicting_args.c b/lib/tests/igt_conflicting_args.c
new file mode 100644
index 00000000..c357b6c5
--- /dev/null
+++ b/lib/tests/igt_conflicting_args.c
@@ -0,0 +1,99 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+/*
+ * DESCRIPTION: Make sure that IGT framework complains when test try to define
+ * conflicting options.
+ */
+
+#include <signal.h>
+#include <sys/wait.h>
+#include <errno.h>
+
+#include "drmtest.h"
+#include "igt_core.h"
+#include "igt_tests_common.h"
+
+static struct option long_options[2];
+static const char *short_options;
+
+static int opt_handler(int option, int option_index, void *input)
+{
+
+	return 0;
+}
+
+static int do_fork(void)
+{
+	char test_name[] = "test";
+
+	char *argv[] = { test_name };
+	int argc = ARRAY_SIZE(argv);
+
+	pid_t pid = fork();
+	internal_assert(pid != -1);
+
+	if (pid) {
+		int status;
+		while (waitpid(pid, &status, 0) == -1 && errno == EINTR)
+			;
+
+		return status;
+	}
+
+
+	igt_subtest_init_parse_opts(&argc, argv, short_options, long_options,
+				    "", opt_handler, NULL);
+	igt_subtest("dummy") {}
+	igt_exit();
+}
+
+int main(int argc, char **argv)
+{
+	/* no conflict */
+	long_options[0] = (struct option) { "iterations", required_argument, NULL, 'i'};
+	short_options = "";
+	internal_assert_wexited(do_fork(), 0);
+
+	/* conflict on short option */
+	long_options[0] = (struct option) { "iterations", required_argument, NULL, 'i'};
+	short_options = "h";
+	internal_assert_wsignaled(do_fork(), SIGABRT);
+
+	/* conflict on long option name */
+	long_options[0] = (struct option) { "help", required_argument, NULL, 'i'};
+	short_options = "";
+	internal_assert_wsignaled(do_fork(), SIGABRT);
+
+	/* conflict on long option 'val' representation vs short option */
+	long_options[0] = (struct option) { "iterations", required_argument, NULL, 'h'};
+	short_options = "";
+	internal_assert_wsignaled(do_fork(), SIGABRT);
+
+	/* conflict on long option 'val' representations */
+	long_options[0] = (struct option) { "iterations", required_argument, NULL, 0};
+	short_options = "";
+	internal_assert_wsignaled(do_fork(), SIGABRT);
+
+	return 0;
+}
diff --git a/lib/tests/meson.build b/lib/tests/meson.build
index 74efce39..9950bd59 100644
--- a/lib/tests/meson.build
+++ b/lib/tests/meson.build
@@ -2,6 +2,7 @@ lib_tests = [
 	'igt_assert',
 	'igt_can_fail',
 	'igt_can_fail_simple',
+	'igt_conflicting_args',
 	'igt_exit_handler',
 	'igt_fork',
 	'igt_fork_helper',
-- 
2.21.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 4+ messages in thread