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

[Peter Hutterer]

On Mon, Jul 11, 2016 at 01:19:54PM -0700, Bryce Harrington wrote:
> 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.

a bool has a not-so-obvious side effect, it makes it clear in the signature
that the return code is *not* the converted value. compare:

    bool magic_atoi(const char *str, int *value);
    int magic_atoi(const char *str, int *value);

in the latter case, you may think that the value is also returned (there is
precedence for that with e.g. time(). a bool is non-ambiguous here.
and of course nothing stops you from setting errno as extra condition.

really, what we need is a sensible version of something like this:
    typedef int negative_errno;
    negative_errno magic_atoi(...)

but this is still C and we can't have nice things. And single-value structs
are type-safe but a huge pain otherwise.

Cheers,
   Peter

> 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