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

[Petri Latvala <petri.latvala@intel.com>]

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>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev