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

[Bryce Harrington]

On Thu, Jul 14, 2016 at 01:35:09PM +0100, Eric Engestrom wrote:
> 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;
> 	}

I like the extra checking, however I think these might be redundant with
strtoul's existing checks?  In the case of a ten digit number, that
should trip the ERANGE error; and in the case of non-valid digits
present in the 8 character length string, shouldn't that cause strtoul
to point *end at that character in the string?  Our *end test in the if
clause should catch that I think.

But your idea of verifying the string length to be exactly 8 or 10
characters could be good.  Maybe we also want to support allowing
"color=0".  I could definitely see someone typoing a color entry as 5 or
7 characters and being confused why it didn't work, or putting an RGB
color entry instead of ARGB and wondering why it's just black.

> 	/* ... */
> 
> 	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.

Yep totally, I figured when splitting it out that more could be added to
allow it to support alternate color specification syntaxes in the future.

Bryce

> 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)