[PATCH weston 1/2] config-parser: Add weston_config_section_get_color

[Eric Engestrom]

On Wed, Jul 13, 2016 at 07:01:28PM -0700, Bryce Harrington wrote:
> Previously weston_config_section_get_uint was serving dual purpose for
> parsing both unsigned decimal integer values (ids, counts, seconds,
> etc.)  and hexadecimal values (colors), by relying on strtoul's
> auto-detection mechanism.
> 
> However, this usage is unable to catch certain kinds of error
> conditions, such as specifying a negative number where an unsigned
> should be used.  And for colors in particular, it would misparse hex
> values if the leading 0x was omitted.  E.g. "background-color=99999999"
> would render a near-black background (effectively 0x05f5e0ff) instead of
> medium grey, and "background-color=ffffffff" would be treated as an
> error rather than white.  "background-color=0x01234567",
> "background-color=01234567", and "background-color=1234567" each
> resulted in the value being parsed as hexadecimal, octal, and decimal
> respectively, resulting in colors 0x01234567, 0x00053977, and 0x0012d687
> being displayed.
> 
> This new routine forces hexadecimal to be used in all cases when parsing
> color values, so "0x01234567", "01234567", and "1234567" all result in
> the same color value, "99999999" is grey, and "ffffffff" is white.
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
>  clients/desktop-shell.c            |  8 ++--
>  clients/ivi-shell-user-interface.c |  2 +-
>  shared/config-parser.c             | 27 +++++++++++++
>  shared/config-parser.h             |  4 ++
>  tests/config-parser-test.c         | 80 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 116 insertions(+), 5 deletions(-)
> 

[...]

> diff --git a/shared/config-parser.c b/shared/config-parser.c
> index 1e08759..f040380 100644
> --- a/shared/config-parser.c
> +++ b/shared/config-parser.c
> @@ -209,6 +209,33 @@ weston_config_section_get_uint(struct weston_config_section *section,
>  
>  WL_EXPORT
>  int
> +weston_config_section_get_color(struct weston_config_section *section,
> +				const char *key,
> +				uint32_t *color, uint32_t default_color)
> +{
> +	struct weston_config_entry *entry;
> +	char *end;
> +
> +	entry = config_section_get_entry(section, key);
> +	if (entry == NULL) {
> +		*color = default_color;
> +		errno = ENOENT;
> +		return -1;
> +	}
> +
> +	errno = 0;
> +	*color = strtoul(entry->value, &end, 16);
> +	if (errno != 0 || end == entry->value || *end != '\0') {
> +		*color = default_color;
> +		errno = EINVAL;
> +		return -1;
> +	}

Looks good to me, except I would put some validation on the expected
input string, eg:

	switch (strlen(entry->value))
	{
		case 10:
			if (entry->value[0] != '0' || entry->value[1] != 'x')
				goto invalid;
			entry->value += 2;

		case 8:
			size_t i;
			for (i = 0; i < 8; i++)
				if (!isxdigit(entry->value[i]))
					goto invalid;
			break;

		default:
			goto invalid;
	}

	/* ... */

	invalid:
		*color = default_color;
		errno = EINVAL;
		return -1;


Additionally, I agree with Quentin, since you made a special function
for colors, we might as well have it support more color formats, but IMO
that should be a followup patch.

So, with the added input validation (and 7-char example removed from the
commit message), this patch is:
Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>

> +
> +	return 0;
> +}
> +
> +WL_EXPORT
> +int
>  weston_config_section_get_double(struct weston_config_section *section,
>  				 const char *key,
>  				 double *value, double default_value)