[igt-dev] [PATCH i-g-t] lib/igt_core: Rework extra options conflicts handling
Arkadiusz Hiler
arkadiusz.hiler at intel.com
Thu May 16 13:02:29 UTC 2019
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 at intel.com>
Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at 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
More information about the igt-dev
mailing list