Re: [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 
> > 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 
> > > >
> > > > 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 

Re: [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 
> 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 
> > >
> > > 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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

On Mon, Jul 11, 2016 at 09:28:16AM +1000, Peter Hutterer 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 
> > 
> > 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.

I've pushed this specific cleanup with Eric's R-b but will follow up
with a proposal for this helper function later.  I think there may be a
few more cleanups to do first.

Bryce
 
> Cheers,
>Peter
> 
> 
> 
> > 
> > > ---
> > >  shared/config-parser.c |  6 --
> > >  tests/config-parser-test.c | 34 ++
> > >  2 files changed, 38 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/shared/config-parser.c b/shared/config-parser.c
> > > index 2256469..151ae9c 100644
> > > --- a/shared/config-parser.c
> > > +++ b/shared/config-parser.c
> > > @@ -169,8 +169,9 @@ weston_config_section_get_int(struct 
> > > weston_config_section *section,
> > >   return -1;
> > >   }
> > >  
> > > + errno = 0;
> > >   *value = strtol(entry->value, , 0);
> > > - if (*end != '\0') {
> > > + if (errno != 0 || end == entry->value || *end != '\0') {
> > >   *value = default_value;
> > >   errno = EINVAL;
> > >   return -1;
> > > @@ -195,8 +196,9 @@ weston_config_section_get_uint(struct 
> > > weston_config_section *section,
> > >   return -1;
> > >   }
> > >  
> > > + errno = 0;
> > >   *value = strtoul(entry->value, , 0);
> > > - if (*end != '\0') {
> > > + if (errno != 0 || end == entry->value || *end != '\0') {
> > >   *value = default_value;
> > >   errno = EINVAL;
> > >   return -1;
> > > diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
> > > index 5dcafc4..735da4e 100644
> > > --- a/tests/config-parser-test.c
> > > +++ b/tests/config-parser-test.c
> > > @@ -116,6 +116,7 @@ static struct zuc_fixture config_test_t1 = {
> > >   "[bar]\n"
> > >   "# more comments\n"
> > >   "number=5252\n"
> > > + "zero=0\n"
> > >   "flag=false\n"
> > >   "\n"
> > >   "[stuff]\n"
> > > @@ -263,6 +264,7 @@ ZUC_TEST_F(config_test_t1, test006, data)
> > >  
> > >   ZUC_ASSERT_EQ(0, r);
> > >   ZUC_ASSERT_EQ(5252, n);
> > > + ZUC_ASSERT_EQ(0, errno);
> > >  }
> > >  
> > >  ZUC_TEST_F(config_test_t1, test007, data)
> > > @@ -289,8 +291,10 @@ ZUC_TEST_F(config_test_t1, test008, data)
> > >  
> > >   section = weston_config_get_section(config, "bar", NULL, NULL);
> > >   r = weston_config_section_get_uint(section, "number", , 600);
> > > +
> > >   ZUC_ASSERT_EQ(0, r);
> > >   ZUC_ASSERT_EQ(5252, u);
> > > + ZUC_ASSERT_EQ(0, errno);
> > >  }
> > >  
> > >  ZUC_TEST_F(config_test_t1, test009, data)
> > > @@ -427,6 +431,36 @@ ZUC_TEST_F(config_test_t1, test017, data)
> > >   ZUC_ASSERT_EQ(5, i);
> > >  }
> > >  
> > > +ZUC_TEST_F(config_test_t1, test018, data)
> > > +{
> > > + int r;
> > > + int32_t n;
> > > + struct weston_config_section *section;
> > > + struct weston_config *config = data;
> > > +
> > > + section = weston_config_get_section(config, "bar", NULL, NULL);
> > > + r = weston_config_section_get_int(section, "zero", , 600);
> > > +
> > > + ZUC_ASSERT_EQ(0, r);
> > > + ZUC_ASSERT_EQ(0, n);
> > > + ZUC_ASSERT_EQ(0, errno);
> > > +}
> > > +
> > > +ZUC_TEST_F(config_test_t1, test019, data)
> > > +{
> > > + int r;
> > > + uint32_t n;
> > > + struct weston_config_section *section;
> > > + struct weston_config *config = data;
> > > +
> > > + section = weston_config_get_section(config, "bar", NULL, NULL);
> > > + r = weston_config_section_get_uint(section, "zero", , 600);
> > > +
> > > + ZUC_ASSERT_EQ(0, r);
> > > + ZUC_ASSERT_EQ(0, n);
> > > + ZUC_ASSERT_EQ(0, errno);
> > > +}
> > > +
> > >  ZUC_TEST_F(config_test_t2, doesnt_parse, data)
> > >  {
> > >   struct weston_config *config = data;
> > > -- 
> > > 1.9.1
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > 

Re: [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 
> 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 
> > >
> > > 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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

On Fri, Jul 08, 2016 at 11:33:56AM +0100, Eric Engestrom wrote:
> On Fri, Jul 08, 2016 at 10:26:43AM +0100, Eric Engestrom wrote:
> > On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> > > + errno = 0;
> > >   *value = strtol(entry->value, , 0);
> > > - if (*end != '\0') {
> > > + if (errno != 0 || end == entry->value || *end != '\0') {
> > 
> > Isn't the empty string case already covered by `*end != '\0'` ?
> 
> No, it's not: I misread that.
> I just re-read the patch, now that I'm a bit more awake, and my r-b
> still stands :)
> 
> > Either way, the duplicate test wouldn't hurt, so:
> > Reviewed-by: Eric Engestrom 

Thanks again, pushed.

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Eric Engestrom]

On Fri, Jul 08, 2016 at 10:26:43AM +0100, Eric Engestrom wrote:
> On Thu, Jul 07, 2016 at 02:08:28PM -0700, Bryce Harrington wrote:
> > +   errno = 0;
> > *value = strtol(entry->value, , 0);
> > -   if (*end != '\0') {
> > +   if (errno != 0 || end == entry->value || *end != '\0') {
> 
> Isn't the empty string case already covered by `*end != '\0'` ?

No, it's not: I misread that.
I just re-read the patch, now that I'm a bit more awake, and my r-b
still stands :)

> Either way, the duplicate test wouldn't hurt, so:
> Reviewed-by: Eric Engestrom 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

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 

If this approach looks ok to folks, there are other irregular uses of
strto*l I'll clean up as well subsequently.

> ---
>  shared/config-parser.c |  6 --
>  tests/config-parser-test.c | 34 ++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/shared/config-parser.c b/shared/config-parser.c
> index 2256469..151ae9c 100644
> --- a/shared/config-parser.c
> +++ b/shared/config-parser.c
> @@ -169,8 +169,9 @@ weston_config_section_get_int(struct 
> weston_config_section *section,
>   return -1;
>   }
>  
> + errno = 0;
>   *value = strtol(entry->value, , 0);
> - if (*end != '\0') {
> + if (errno != 0 || end == entry->value || *end != '\0') {
>   *value = default_value;
>   errno = EINVAL;
>   return -1;
> @@ -195,8 +196,9 @@ weston_config_section_get_uint(struct 
> weston_config_section *section,
>   return -1;
>   }
>  
> + errno = 0;
>   *value = strtoul(entry->value, , 0);
> - if (*end != '\0') {
> + if (errno != 0 || end == entry->value || *end != '\0') {
>   *value = default_value;
>   errno = EINVAL;
>   return -1;
> diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
> index 5dcafc4..735da4e 100644
> --- a/tests/config-parser-test.c
> +++ b/tests/config-parser-test.c
> @@ -116,6 +116,7 @@ static struct zuc_fixture config_test_t1 = {
>   "[bar]\n"
>   "# more comments\n"
>   "number=5252\n"
> + "zero=0\n"
>   "flag=false\n"
>   "\n"
>   "[stuff]\n"
> @@ -263,6 +264,7 @@ ZUC_TEST_F(config_test_t1, test006, data)
>  
>   ZUC_ASSERT_EQ(0, r);
>   ZUC_ASSERT_EQ(5252, n);
> + ZUC_ASSERT_EQ(0, errno);
>  }
>  
>  ZUC_TEST_F(config_test_t1, test007, data)
> @@ -289,8 +291,10 @@ ZUC_TEST_F(config_test_t1, test008, data)
>  
>   section = weston_config_get_section(config, "bar", NULL, NULL);
>   r = weston_config_section_get_uint(section, "number", , 600);
> +
>   ZUC_ASSERT_EQ(0, r);
>   ZUC_ASSERT_EQ(5252, u);
> + ZUC_ASSERT_EQ(0, errno);
>  }
>  
>  ZUC_TEST_F(config_test_t1, test009, data)
> @@ -427,6 +431,36 @@ ZUC_TEST_F(config_test_t1, test017, data)
>   ZUC_ASSERT_EQ(5, i);
>  }
>  
> +ZUC_TEST_F(config_test_t1, test018, data)
> +{
> + int r;
> + int32_t n;
> + struct weston_config_section *section;
> + struct weston_config *config = data;
> +
> + section = weston_config_get_section(config, "bar", NULL, NULL);
> + r = weston_config_section_get_int(section, "zero", , 600);
> +
> + ZUC_ASSERT_EQ(0, r);
> + ZUC_ASSERT_EQ(0, n);
> + ZUC_ASSERT_EQ(0, errno);
> +}
> +
> +ZUC_TEST_F(config_test_t1, test019, data)
> +{
> + int r;
> + uint32_t n;
> + struct weston_config_section *section;
> + struct weston_config *config = data;
> +
> + section = weston_config_get_section(config, "bar", NULL, NULL);
> + r = weston_config_section_get_uint(section, "zero", , 600);
> +
> + ZUC_ASSERT_EQ(0, r);
> + ZUC_ASSERT_EQ(0, n);
> + ZUC_ASSERT_EQ(0, errno);
> +}
> +
>  ZUC_TEST_F(config_test_t2, doesnt_parse, data)
>  {
>   struct weston_config *config = data;
> -- 
> 1.9.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Bryce Harrington]

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 
---
 shared/config-parser.c |  6 --
 tests/config-parser-test.c | 34 ++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/shared/config-parser.c b/shared/config-parser.c
index 2256469..151ae9c 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -169,8 +169,9 @@ weston_config_section_get_int(struct weston_config_section 
*section,
return -1;
}
 
+   errno = 0;
*value = strtol(entry->value, , 0);
-   if (*end != '\0') {
+   if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
return -1;
@@ -195,8 +196,9 @@ weston_config_section_get_uint(struct weston_config_section 
*section,
return -1;
}
 
+   errno = 0;
*value = strtoul(entry->value, , 0);
-   if (*end != '\0') {
+   if (errno != 0 || end == entry->value || *end != '\0') {
*value = default_value;
errno = EINVAL;
return -1;
diff --git a/tests/config-parser-test.c b/tests/config-parser-test.c
index 5dcafc4..735da4e 100644
--- a/tests/config-parser-test.c
+++ b/tests/config-parser-test.c
@@ -116,6 +116,7 @@ static struct zuc_fixture config_test_t1 = {
"[bar]\n"
"# more comments\n"
"number=5252\n"
+   "zero=0\n"
"flag=false\n"
"\n"
"[stuff]\n"
@@ -263,6 +264,7 @@ ZUC_TEST_F(config_test_t1, test006, data)
 
ZUC_ASSERT_EQ(0, r);
ZUC_ASSERT_EQ(5252, n);
+   ZUC_ASSERT_EQ(0, errno);
 }
 
 ZUC_TEST_F(config_test_t1, test007, data)
@@ -289,8 +291,10 @@ ZUC_TEST_F(config_test_t1, test008, data)
 
section = weston_config_get_section(config, "bar", NULL, NULL);
r = weston_config_section_get_uint(section, "number", , 600);
+
ZUC_ASSERT_EQ(0, r);
ZUC_ASSERT_EQ(5252, u);
+   ZUC_ASSERT_EQ(0, errno);
 }
 
 ZUC_TEST_F(config_test_t1, test009, data)
@@ -427,6 +431,36 @@ ZUC_TEST_F(config_test_t1, test017, data)
ZUC_ASSERT_EQ(5, i);
 }
 
+ZUC_TEST_F(config_test_t1, test018, data)
+{
+   int r;
+   int32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_int(section, "zero", , 600);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
+ZUC_TEST_F(config_test_t1, test019, data)
+{
+   int r;
+   uint32_t n;
+   struct weston_config_section *section;
+   struct weston_config *config = data;
+
+   section = weston_config_get_section(config, "bar", NULL, NULL);
+   r = weston_config_section_get_uint(section, "zero", , 600);
+
+   ZUC_ASSERT_EQ(0, r);
+   ZUC_ASSERT_EQ(0, n);
+   ZUC_ASSERT_EQ(0, errno);
+}
+
 ZUC_TEST_F(config_test_t2, doesnt_parse, data)
 {
struct weston_config *config = data;
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel