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

[Eric Engestrom]

On Thu, Jul 14, 2016 at 11:30:39AM -0700, Bryce Harrington wrote:
> 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.

You're right, the only useful check are the length and prefix, the rest
is already covered. To be honest, I wrote it like that with the idea
that it could then easily be extended to add other length cases and do
the conversion needed.

Maybe something like this would be better, with format conversions
included:

	char color_string[9] = ""; /* AARRGGBB */
	char default_alpha = 'F';
	switch(strlen(entry->value)
	{
		case 10: if (entry->value[0] != '0' || entry->value[1] != 'x') goto invalid;
		         color_string = { entry->value[2], entry->value[3], entry->value[4], entry->value[5], entry->value[6], entry->value[7], entry->value[8], entry->value[9] }; break; /* 0xAARRGGBB */
		case  8: color_string = { entry->value[0], entry->value[1], entry->value[2], entry->value[3], entry->value[4], entry->value[5], entry->value[6], entry->value[7] }; break; /*   AARRGGBB */
		case  6: color_string = {   default_alpha,   default_alpha, entry->value[0], entry->value[1], entry->value[2], entry->value[3], entry->value[4], entry->value[5] }; break; /*     RRGGBB */
		case  4: color_string = { entry->value[0], entry->value[0], entry->value[1], entry->value[1], entry->value[2], entry->value[2], entry->value[3], entry->value[3] }; break; /*    A R G B */
		case  3: color_string = {   default_alpha,   default_alpha, entry->value[0], entry->value[0], entry->value[1], entry->value[1], entry->value[2], entry->value[2] }; break; /*      R G B */
		case  1: color_string = { entry->value[0], entry->value[0], entry->value[0], entry->value[0], entry->value[0], entry->value[0], entry->value[0], entry->value[0] }; break; /* single character repeated */
		default: goto bad_range;
	}

and then feed color_string to strtoul().

(Sorry for the long lines, but I think folding them makes it unreadable.)

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