[PATCH v2c] weston: catch missing commandline values

[Bryce Harrington]

On Thu, May 12, 2016 at 08:12:09PM -0700, Florian Hänel wrote:
> Sorry for the spam, last version for today.
> Does not make sense to fprintf a value that causes ENOMEM.
> Fixed some more whitespace.
> 
> Florian

> >From 4c390be81da27e7b2331c3e71b848e03173b4632 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Florian=20H=C3=A4nel?= <florian.haenel at lge.com>
> Date: Thu, 12 May 2016 14:40:44 -0700
> Subject: [PATCH] Catch missing commandline values
> 
> Exit if commandline options are missing values.
> 
> Tested with:
> 
> missing = and value
> $ weston --tty
> missing =
> $ weston --tty 2
> missing value
> $ weston --tty=
> no digits found:
> $ weston --tty=ADASDASDAS
> overflow:
> $ weston --tty=99999999999999999999999999999999999999999999999999
> hex prefix is allowed:
> $ weston --tty=0x2
> ---
>  shared/option-parser.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/shared/option-parser.c b/shared/option-parser.c
> index 7061268..fb893ee 100644
> --- a/shared/option-parser.c
> +++ b/shared/option-parser.c
> @@ -27,6 +27,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <assert.h>
> +#include <errno.h>
>  
>  #include "config-parser.h"
>  
> @@ -34,20 +35,34 @@ static int
>  handle_option(const struct weston_option *option, char *value)
>  {
>  	char* p;
> +	if (strlen(value) == 0) {
> +		fprintf(stderr, "\nERROR: expected value after %s=\n", option->name);
> +		exit(EXIT_FAILURE);
> +	}
>  
>  	switch (option->type) {
>  	case WESTON_OPTION_INTEGER:
>  		* (int32_t *) option->data = strtol(value, &p, 0);
> +		if (errno == EINVAL || errno == ERANGE || p == value) goto err_parse;
>  		return *value && !*p;
>  	case WESTON_OPTION_UNSIGNED_INTEGER:
>  		* (uint32_t *) option->data = strtoul(value, &p, 0);
> +		if (errno == EINVAL || errno == ERANGE || p == value) goto err_parse;
>  		return *value && !*p;
>  	case WESTON_OPTION_STRING:
>  		* (char **) option->data = strdup(value);
> +		if (errno == ENOMEM) {
> +			fprintf(stderr, "ERROR: out of memory parsing commandline\n");
> +			exit(EXIT_FAILURE);

The first two cases use a goto for error handling but in this third case
the error handling is inline, which is inconsistent.  Given that there's
multiple error exits already, I'd suggest eliminating the gotos and just
inlining the fprintf and exit.

> +		};
>  		return 1;
>  	default:
> -		assert(0);
> +		fprintf(stderr, "\nERROR: internal commandline parsing error after option %s.\n",option->name);

Needs a space after the second comma

> +		exit(EXIT_FAILURE);

Is there a particular reason for dropping the assert here?  
The only way this branch could be hit is if there is a programming error
prior to the handle_option call.  An assert would stop the debugger at
this point so the issue could be investigated, so I'm not certain it
would be better to exit with an error message.

>  	}
> +    err_parse:
> +		fprintf(stderr, "\nERROR: invalid value (%s) for option %s.\n", value, option->name);
> +		exit(EXIT_FAILURE);
>  }
>  
>  static int
> @@ -71,6 +86,9 @@ long_option(const struct weston_option *options, int count, char *arg)
>  			}
>  		} else if (arg[len+2] == '=') {
>  			return handle_option(options + k, arg + len + 3);
> +		} else {
> +			fprintf(stderr, "\nERROR: expected syntax: --%s=<value>\n", options[k].name);

It looks a bit awkward to have two colons in an error message, maybe
rephrase the error message?

> +			exit(EXIT_FAILURE);
>  		}
>  	}

Bryce