[PATCH weston] config-parser: Improve error checks for strtol/strtoul calls

[Bryce Harrington]

On Mon, Jul 11, 2016 at 09:39:00AM -0700, Bill Spitzak wrote:
> On Sun, Jul 10, 2016 at 4:28 PM, Peter Hutterer <peter.hutterer at who-t.net>
> wrote:
> 
> > On Thu, Jul 07, 2016 at 02:24:39PM -0700, Bryce Harrington wrote:
> > > On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> > > > Check errno, which is set of over/underflow, out of range, etc.  Also
> > > > check for empty strings (the usages covered in this patch already also
> > > > cover the case where there are non-digits present).  Set errno to 0
> > > > before making the strto*l call in case of pre-existing errors
> > > > (i.e. ENOTTY when running under the testsuite).
> > > >
> > > > This follows the error checking style used in Wayland
> > > > (c.f. wayland-client.c and scanner.c).
> > > >
> > > > In tests, also check errno, and add testcases for parsing '0'.
> > > >
> > > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > >
> > > If this approach looks ok to folks, there are other irregular uses of
> > > strto*l I'll clean up as well subsequently.
> >
> > it'd be best to have a safe_strtol helper function in a weston-util.h
> > header and just enforce use of that. because most of the time all we care
> > about is "has it parsed and what's the result", you can get that with a
> >     bool safe_strtol(const char *string, long *result);
> >
> > or safe_atoi, or whatever you want to call it.
> >
> 
> The problem is that it is a pain to have to pass a reference to the output
> variable, which discourages use and why everybody keeps calling strtol. A
> function that returns a value is much more usable and easy to read in the
> source code.
> 
> To report the error setting errno will work. Programs can check this after
> return. Returning a bool, or passing a pointer to a bool, really does not
> help, the program can ignore that just as easily as ignoring errno, and you
> have made it harder to call the function and thus discouraged it's use.

Hmm, I get your point, but checking bool error code returns is way more
common than checking for errno, and I would think less likely to be
ignored.  As far as convenience goes... well error checking is annoying
in general, so it's a bit of a wash, but checking errno with a value
return is going to require slightly more typing than checking return
codes with a value passed by reference.  Using errno also places a
requirement for including errno.h on the caller.

But to me, those points are both really minor.  The real benefits to
relying on errno here would be: 1) the errors are more expressive
(e.g. ERANGE or EINVAL rather than just a simple true/false), and 2)
errno can be checked after making a sequence of conversion calls to
check if any of them failed, rather than checking the return code on
each individually.

On the first point, most strtol/strtoul calls in Weston don't seem to
care much why it failed.  For the second point, there is indeed a
sequence of calls in the config parser, where checking errno just once
after all calls were made might make for more concise code, but if there
*is* an error, then for debugging purposes I'm guessing folks would want
to know what line in the config file errored, in which case we'd be back
to needing to check errors after each conversion.

So, considering all these points it seems like the trade-off favors
something more like what Peter suggests - pass the value by reference
and indicate success or failure with a bool return value.  We could also
set errno in addition, in case the caller might want to know more
details about why the failure occurred, but since none of the callers
seem to care to that level of detail maybe it'd be better to just leave
errno unchanged, as Wayland's strtouint() does; a caller that *did* care
about the exact errno can always just do their own direct strtol call.

> Have the function log errors to stderr if you want to make it "impossible"
> to ignore errors.

Hmm, not sure on this.  From the software's point of view, I would argue
that logging messages to stderr and continuing on with processing is
indeed ignoring the errors.  Also, for general purpose utility functions
like this, it seems better to leave decisions about error logging to the
caller, because it'll have more context over what the parsing is trying
to do.  Some things might be best sent to weston_log() rather than
stderr for example.

Bryce