[PATCH weston 1/2] config-parser: Add weston_config_section_get_color
Eric Engestrom
eric.engestrom at imgtec.com
Fri Jul 15 09:22:54 UTC 2016
On Fri, Jul 15, 2016 at 10:13:03AM +0100, Eric Engestrom wrote:
> 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 */
Sorry, repeating the single-char color for the alpha doesn't make sense,
this should be:
case 1: color_string = { default_alpha, default_alpha, 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)
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list