[PATCH 01/12] Fixes to parse_options

[Pekka Paalanen]

On Fri,  8 Aug 2014 12:59:50 -0700
Bill Spitzak <spitzak at gmail.com> wrote:

> Fail on malformed numbers, such as --width=100mm

Good.

> Fail on = after booleans, such as --flag=false

Okay.

> Multiple single-letter booleans in one switch allowed, ie
> -xyz is the same as -x -y -z. For wayland modules they all have
> to belong to the same module.

This was not accepted before, right? It is confusing that one needs to
know that they are from the same module, so I'd rather just not accept
combined single letter booleans at all.

> Previous version could use text after the null at the end of
> an argv entry. Now requires the = sign.

What does this mean? I mean, how could one have ever used the old
behaviour? Nothing comes to my mind, so I suppose that is ok.

There are some style issues. First is the patch subject, it should be
prefixed with the component the patch is touching. To get a feeling
what the components are, look at the git-log of the file your are
changing, and there should be a pattern.

Though, I see that option-parser.c doesn't really have any pattern. Oh
well, "shared:" should do then.

More style issues below.

> ---
>  shared/option-parser.c |   82 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 53 insertions(+), 29 deletions(-)
> 
> diff --git a/shared/option-parser.c b/shared/option-parser.c
> index c00349a..22bcf2f 100644
> --- a/shared/option-parser.c
> +++ b/shared/option-parser.c
> @@ -30,53 +30,77 @@
>  
>  #include "config-parser.h"
>  
> -static void
> +static int
>  handle_option(const struct weston_option *option, char *value)
>  {
> +	char* p;

Need empty line between declarations and code.

>  	switch (option->type) {
>  	case WESTON_OPTION_INTEGER:
> -		* (int32_t *) option->data = strtol(value, NULL, 0);
> -		return;
> +		* (int32_t *) option->data = strtol(value, &p, 0);
> +		return p > value && !*p;

Took me a couple of times to read this return statement.
How about:
	return *value != '\0' && *p == '\0';

Just a suggestion, doesn't matter.

>  	case WESTON_OPTION_UNSIGNED_INTEGER:
> -		* (uint32_t *) option->data = strtoul(value, NULL, 0);
> -		return;
> +		* (uint32_t *) option->data = strtoul(value, &p, 0);
> +		return p > value && !*p;
>  	case WESTON_OPTION_STRING:
>  		* (char **) option->data = strdup(value);
> -		return;
> -	case WESTON_OPTION_BOOLEAN:
> -		* (int32_t *) option->data = 1;
> -		return;
> +		return 1;
>  	default:
>  		assert(0);
>  	}
>  }
>  
> +static int
> +long_option(const struct weston_option *options, int count, char* arg)
> +{
> +	int k, len;

Need empty line between declarations and code.

> +	for (k = 0; k < count; k++) {
> +		if (! options[k].name) continue;

No space after in (! options[k].name).

'continue' should be on a new line

> +		len = strlen(options[k].name);
> +		if (strncmp(options[k].name, arg+2, len) != 0) continue;

add spaces: arg + 2

'continue' should be on a new line

> +		if (options[k].type == WESTON_OPTION_BOOLEAN) {
> +			if (! arg[len+2]) {

Extra space.

> +				* (int32_t *) options[k].data = 1;
> +				return 1;
> +			}
> +		} else if (arg[len+2] == '=') {

spaces: len + 2

> +			return handle_option(options+k, arg+len+3);

spaces: options + k, arg + len + 3

> +		}
> +	}
> +	return 0;
> +}
> +
> +static int
> +short_option(const struct weston_option *options, int count, char* arg)
> +{
> +	int i, k;

Need empty line between declarations and code.

> +	for (i = 1; arg[i]; i++) {
> +		for (k = 0; ; k++) {
> +			if (k >= count) return 0;

'return' to a new line.

> +			if (options[k].short_name == arg[i]) break;

'break' to a new line.

> +		}
> +		if (options[k].type == WESTON_OPTION_BOOLEAN)
> +			* (int32_t *) options[k].data = 1;
> +		else
> +			return handle_option(options+k, arg+i+1);

missing spaces

> +	}
> +	return arg[1]; /* error on just '-' */

Implicit cast to a "boolean" is kind of surprising, a comparison would
be more obvious:
	return arg[1] != '\0';

Or better yet, just exit the function in the first lines, if arg[1] ==
'\0'.

> +}
> +
> +
>  int
>  parse_options(const struct weston_option *options,
>  	      int count, int *argc, char *argv[])
>  {
> -	int i, j, k, len = 0;
> -
> +	int i, j;

Need empty line between declarations and code.

>  	for (i = 1, j = 1; i < *argc; i++) {
> -		for (k = 0; k < count; k++) {
> -			if (options[k].name)
> -				len = strlen(options[k].name);
> -			if (options[k].name &&
> -			    argv[i][0] == '-' &&
> -			    argv[i][1] == '-' &&
> -			    strncmp(options[k].name, &argv[i][2], len) == 0 &&
> -			    (argv[i][len + 2] == '=' || argv[i][len + 2] == '\0')) {
> -				handle_option(&options[k], &argv[i][len + 3]);
> -				break;
> -			} else if (options[k].short_name &&
> -				   argv[i][0] == '-' &&
> -				   options[k].short_name == argv[i][1]) {
> -				handle_option(&options[k], &argv[i][2]);
> -				break;
> -			}
> +		if (argv[i][0] == '-') {
> +			if (argv[i][1] == '-') {
> +				if (long_option(options, count, argv[i]))
> +					continue;
> +			} else if (short_option(options, count, argv[i]))
> +				continue;
>  		}
> -		if (k == count)
> -			argv[j++] = argv[i];
> +		argv[j++] = argv[i];
>  	}
>  	argv[j] = NULL;
>  	*argc = j;

But in general, the idea is good, IMHO.


Thanks,
pq