[PATCH weston] option-parser: Require integer option string values to be base-10

[Bryce Harrington]

On Fri, Jul 08, 2016 at 06:47:19PM -0700, Yong Bakos wrote:
> On Jul 8, 2016, at 5:52 PM, Bryce Harrington <bryce at osg.samsung.com> wrote:
> > 
> > The third arg to strtol() specifies the base to assume for the number.
> > When 0 is passed, as is currently done in option-parser.c, hexadecimal
> > and octal numbers are permitted and automatically detected and
> > converted.
> > 
> > In weston and the weston clients and tests using option-parser.c, the
> > options are all things that can be expected to be specified in base 10:
> > widths, heights, counts, scales, font sizes, ports, ttys, connectors,
> > etc.  The subsurfaces client uses two modes, limited to values 0 and 1
> > only.  The zuc testsuite has a --random parameter for specifying a seed,
> > which is the only option where using hexadecimal or octal numbers might
> > conceivably happen.
> > 
> > The benefit of limiting this to base-10 is to eliminate surprises when
> > parsing numbers from the command line.  Also, by making the code
> > consistent with other usages of strtol/strtoul, it may make it possible
> > to factor out the common code in the future.
> > 
> > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> 
> I can't think of any drawbacks, other than when some future option has,
> say, a hex color value. But that's "YAGNI" for now, and would be easy to
> change later if need be.

Right, that's my thinking too.  And if we did have a hex color option,
and someone did --color=336633, you'd want to force that to be handled as
hexidecimal and not permit it to accidentally autoconvert as decimal,
anyway.

> Reviewed-by: Yong Bakos <ybakos at humanoriented.com>

Thanks,
Bryce

> Regards,
> yong
> 
> 
> > ---
> > shared/option-parser.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/shared/option-parser.c b/shared/option-parser.c
> > index d5fee8e..33355b8 100644
> > --- a/shared/option-parser.c
> > +++ b/shared/option-parser.c
> > @@ -40,10 +40,10 @@ handle_option(const struct weston_option *option, char *value)
> > 
> > 	switch (option->type) {
> > 	case WESTON_OPTION_INTEGER:
> > -		* (int32_t *) option->data = strtol(value, &p, 0);
> > +		* (int32_t *) option->data = strtol(value, &p, 10);
> > 		return *value && !*p;
> > 	case WESTON_OPTION_UNSIGNED_INTEGER:
> > -		* (uint32_t *) option->data = strtoul(value, &p, 0);
> > +		* (uint32_t *) option->data = strtoul(value, &p, 10);
> > 		return *value && !*p;
> > 	case WESTON_OPTION_STRING:
> > 		* (char **) option->data = strdup(value);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel