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

[Peter Hutterer]

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.

I'm really struggling to take you seriously here. For comparison:
putting seatbelts on is a pain but the only time when it matters is when
things go wrong and, oh boy, will you be glad when your feet don't ask your
head to remove the windscreen so they can explore the space beyond.

I also find it rather entertaining how you believe to speak for everybody.
have you thought about how maybe "everybody" uses strtol because it's
already implemented and the only one reliably available? have you given
thought to all the projects that wrap it because strtol it's a terrible API?

> 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.
> Have the function log errors to stderr if you want to make it "impossible"
> to ignore errors.

let me be clear about "the program can ignore $USEFUL_THING" because
arguments similar to this have come from you in the past:
This is a free software project with several gatekeepers with motivations
that go beyond the mere monetary interests to make it work no matter what.
On average, we not only care about the code right now but also whether this
will make our lives easier, better or at least less embarrassing in the
future. Yes, "a program can ignore $USEFUL_THING" is true but we are free to
ignore to patches that don't do the right thing just because the author is
lazy and, even better, we are free to ignore authors of patches that
repeatedly show they value their own lazyness above everyone else's time.
coincidentally, this thread started exactly *because* of such a discussion
to improve things for the future even at the cost of minimally more effort
right now.

anyway, I really thought long about whether it's worth replying but in the
end I decided I wanted the above paragraph in a readily accessible link.
Don't expect me to continue this conversation otherwise.

Cheers,
   Peter