You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/01/26 18:54:14 UTC
svn commit: r903342 - /subversion/trunk/subversion/libsvn_diff/parse-diff.c
Author: stsp
Date: Tue Jan 26 17:54:14 2010
New Revision: 903342
URL: http://svn.apache.org/viewvc?rev=903342&view=rev
Log:
* subversion/libsvn_diff/parse-diff.c
(parse_offset): Clearing errno is never necessary, so don't.
Modified:
subversion/trunk/subversion/libsvn_diff/parse-diff.c
Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/parse-diff.c?rev=903342&r1=903341&r2=903342&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
+++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Tue Jan 26 17:54:14 2010
@@ -47,7 +47,6 @@
{
apr_int64_t parsed_offset;
- errno = 0; /* clear errno for safety */
parsed_offset = apr_atoi64(number);
if (errno == ERANGE || parsed_offset < 0)
return FALSE;
Re: svn commit: r903342 -
/subversion/trunk/subversion/libsvn_diff/parse-diff.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jan 26, 2010 at 11:23:18AM -0800, Blair Zajac wrote:
> On 01/26/2010 10:32 AM, Stefan Sperling wrote:
> >I've been told some time ago, by long-time unix hackers,
> >that clearning errno is usually not necessary, one of the few
> >exceptions to this rule being the strtol() family of functions.
> >I didn't account for this exception when writing the log message,
> >which was wrong.
>
> My understand is that if you ever need to check the errno then you
> should set it 0 yourself because system and library calls will not
> set it to 0 for you.
Usually you won't compare errno to zero, but to some known
value which the function in question will set errno to in case
of failure. The failure itself is not indicated via errno, but
through other means, like the return value. Examining errno then
gives you the reason for the failure.
E.g. if open(2) fails, it indicates failure by returning -1.
You would then check errno for a number of values, e.g. ENOTDIR,
ENAMETOOLONG, ENOENT, EACCES, etc. to find out what happened.
But in case open returns a valid fd, you can just go on and
use the fd. errno won't contain interesting information.
> How is strtol() different?
If the parsed number is out of range, strtol() uses errno as the
only channel to signal an error. Actually, the same applies to
apr_strtoi64(), so I really should not have made the change which
prompted this discussion! A better interface would probably require
allocating the converted number and returning a pointer to it.
Then it could return NULL on failure, instead of something
which looks like a number.
For reference: http://www.openbsd.org/cgi-bin/man.cgi?query=strtol
Ensuring that a string is a valid number (i.e., in range and containing
no trailing characters) requires clearing errno beforehand explicitly
since errno is not changed on a successful call to strtol(), and the re-
turn value of strtol() cannot be used unambiguously to signal an error:
char *ep;
long lval;
...
errno = 0;
lval = strtol(buf, &ep, 10);
if (buf[0] == '\0' || *ep != '\0')
goto not_a_number;
if (errno == ERANGE && (lval == LONG_MAX || lval == LONG_MIN))
goto out_of_range;
Stefan
Re: svn commit: r903342 - /subversion/trunk/subversion/libsvn_diff/parse-diff.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 01/26/2010 10:32 AM, Stefan Sperling wrote:
> On Tue, Jan 26, 2010 at 10:02:02AM -0800, Blair Zajac wrote:
>> On 01/26/2010 09:54 AM, stsp@apache.org wrote:
>>> Author: stsp
>>> Date: Tue Jan 26 17:54:14 2010
>>> New Revision: 903342
>>>
>>> URL: http://svn.apache.org/viewvc?rev=903342&view=rev
>>> Log:
>>> * subversion/libsvn_diff/parse-diff.c
>>> (parse_offset): Clearing errno is never necessary, so don't.
>>
>> Why are you claiming this?
>
> I've been told some time ago, by long-time unix hackers,
> that clearning errno is usually not necessary, one of the few
> exceptions to this rule being the strtol() family of functions.
> I didn't account for this exception when writing the log message,
> which was wrong.
Thanks for checking this, I saw the same with with 0.9.
My understand is that if you ever need to check the errno then you
should set it 0 yourself because system and library calls will not set
it to 0 for you. How is strtol() different?
> So I'll revert the change and put a better comment in place
> then the one that was there before.
Thanks!
Regards,
Blair
Re: svn commit: r903342 -
/subversion/trunk/subversion/libsvn_diff/parse-diff.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jan 26, 2010 at 10:02:02AM -0800, Blair Zajac wrote:
> On 01/26/2010 09:54 AM, stsp@apache.org wrote:
> >Author: stsp
> >Date: Tue Jan 26 17:54:14 2010
> >New Revision: 903342
> >
> >URL: http://svn.apache.org/viewvc?rev=903342&view=rev
> >Log:
> >* subversion/libsvn_diff/parse-diff.c
> > (parse_offset): Clearing errno is never necessary, so don't.
>
> Why are you claiming this?
I've been told some time ago, by long-time unix hackers,
that clearning errno is usually not necessary, one of the few
exceptions to this rule being the strtol() family of functions.
I didn't account for this exception when writing the log message,
which was wrong.
But looking at the APR code, apr_atoi64() is not using strol() behind
the scenes, it's a custom implementation. In APR-1.3.x, apr_atoi64()
actually clears errno itself before doing anything.
But (and this is why I am happy that you asked this question and
made me look harder) errno isn't cleared in apr_atoi64() of APR-0.9.
This could be a problem.
So I'll revert the change and put a better comment in place
then the one that was there before.
Thanks for asking,
Stefan
Re: svn commit: r903342 - /subversion/trunk/subversion/libsvn_diff/parse-diff.c
Posted by Blair Zajac <bl...@orcaware.com>.
On 01/26/2010 09:54 AM, stsp@apache.org wrote:
> Author: stsp
> Date: Tue Jan 26 17:54:14 2010
> New Revision: 903342
>
> URL: http://svn.apache.org/viewvc?rev=903342&view=rev
> Log:
> * subversion/libsvn_diff/parse-diff.c
> (parse_offset): Clearing errno is never necessary, so don't.
Why are you claiming this?
Regards,
Blair