[PATCH weston v2 2/7] shared/option-parser: Rework option parsing

[Bill Spitzak]

This looks good to me, especially with all the automatic tests added.

Quentin Glidic wrote:

> -static void
> -handle_option(const struct weston_option *option, char *value)
> +enum state {
> +	OK,
> +	EATEN,
> +	ERROR
> +};
> +
> +static enum state
> +handle_option(const struct weston_option *option, const char *value)
>  {
>  	switch (option->type) {
>  	case WESTON_OPTION_INTEGER:
> +		if (value == NULL || value[0] == '\0')
> +			return ERROR;
>  		* (int32_t *) option->data = strtol(value, NULL, 0);

Since you already checked for a zero-length string, you can detect 
non-numbers with:

	        const char* p;
		... option->data = strtol(value, 0, &p);
		if (*p) return ERROR;

> -		return;
> +		return EATEN;
>  	case WESTON_OPTION_UNSIGNED_INTEGER:
> +		if (value == NULL || value[0] == '\0')
> +			return ERROR;
>  		* (uint32_t *) option->data = strtoul(value, NULL, 0);
> -		return;
> +		return EATEN;
>  	case WESTON_OPTION_STRING:
> +		if (value == NULL)
> +			return ERROR;
>  		* (char **) option->data = strdup(value);
> -		return;
> +		return EATEN;
>  	case WESTON_OPTION_BOOLEAN:
>  		* (int32_t *) option->data = 1;
> -		return;
> +		return OK;
>  	default:
>  		assert(0);
>  	}
> @@ -54,26 +66,89 @@ parse_options(const struct weston_option *options,
>  	      int count, int *argc, char *argv[])
>  {
>  	int i, j, k, len = 0;
> +	const char *arg, *value;
>  
>  	for (i = 1, j = 1; i < *argc; i++) {
> -		for (k = 0; k < count; k++) {
> -			if (options[k].name)
> +		arg = argv[i];
> +		value = argv[i+1];
> +
> +		if (arg[0] != '-')
> +		{
> +			/* Not an option, just skip it */
> +			argv[j++] = argv[i];
> +			continue;
> +		}
> +		++arg; /* Skip the first dash */
> +
> +		/* We have an option, check for which one */
> +		if (arg[0] == '-') {
> +			/* Long option */
> +			++arg; /* Skip the second dash */
> +			for (k = 0; k < count; k++) {
> +				if (!options[k].name)
> +					/* No long variant for this option */
> +					continue;
> +
>  				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]);
> +				if (strncmp(options[k].name, arg, len) != 0)
> +					/* Not matching */
> +					continue;
> +
> +				switch (arg[len]) {
> +				case '=': value = arg + (len+1);
> +				case '\0': break;
> +				default: continue; /* Not fully matching */
> +				}
> +
> +				switch (handle_option(&options[k], value)) {
> +				case OK: break;
> +				case EATEN:
> +					if (arg[len] == '\0')
> +						++i;
> +					break;
> +				case ERROR: return -1;
> +				}
> +
>  				break;
> -			} else if (options[k].short_name &&
> -				   argv[i][0] == '-' &&
> -				   options[k].short_name == argv[i][1]) {
> -				handle_option(&options[k], &argv[i][2]);
> +			}
> +		} else {
> +			/* Short option */
> +			for (k = 0; k < count; k++) {
> +				if (!options[k].short_name)
> +					/* no short variant for this option */
> +					continue;
> +
> +				if (options[k].short_name != arg[0])
> +					/* Not matching */
> +					continue;
> +
> +				if (arg[1] != '\0')
> +					value = arg+1;
> +
> +				switch (arg[1]) {
> +				case '\0': break;
> +				case '=': value = arg + 2; break;
> +				default: value = arg + 1;
> +				}
> +				//fprintf(stderr, "arg = %s, value = '%s'\n", arg, value);
> +
> +				switch (handle_option(&options[k], value)) {
> +				case OK:
> +					if (arg[1] != '\0')
> +						/* Do not support merged short options */
> +						return -1;

Good idea, leave this possibility open without deciding on it now.

> +					break;
> +				case EATEN:
> +					if (arg[1] == '\0')
> +						++i;
> +					break;
> +				case ERROR: return -1;
> +				}
>  				break;
>  			}
>  		}
>  		if (k == count)
> +			/* None matched */
>  			argv[j++] = argv[i];
>  	}
>  	argv[j] = NULL;