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

Petri Latvala
2019-05-23T07:46:52Z

On Thu, May 16, 2019 at 04:02:29PM +0300, Arkadiusz Hiler wrote:
> 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);

Indentation here looks funky.

> -
> - /* 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);

Indentation funky here as well.

Otherwise LGTM. Thanks for doing this, this is a great help in my
upcoming series that touches testdisplay among other things...

Reviewed-by: Petri Latvala <petri.latvala@intel.com>