Re: abort() calls

Greg Hudson wrote:
> It's already our policy to throw an error--and not abort--when any
> external error occurs, even if it's an unexpected error like data
> corruption. The only exception is out-of-memory errors; as with most
> code, Subversion considers it too burdensome to recover from allocation
> failures since allocations occur so often.

I can accept that policy. But when you look at the code in trunk right
now, you'll find a *lot* of places where abort() is called just because
something isn't as expected. For example, I found some in the libsvn_wc
part where abort() is called simply because the entries file doesn't
have an entry which is expected. And that's something which must not
happen: corrupted working copies *can* happen, and especially for TSVN
it's not acceptable to have the whole explorer and desktop shutting down
simply because of that.

> It's currently our policy to assert or abort when a coding error is
> detected. There are a couple of reasons for this:
>
> 1. It's impossible to protect the caller from all coding errors; buggy
> C code can always cause a crash. (There's a counterargument: the
> calling code might be written in a safe language like Python, using the
> Subversion bindings. In many cases the bindings might be able to check
> for invalid inputs and throw language-specific exceptions, but not
> necessarily all cases.)

Of course you can't check for all invalid arguments that are passed to
an API. Ok, you could but for example if someone passes an invalid
pointer then a crash should happen.
But that's not what I'm upset about. It's the handling of situations
where invalid *data* causes an abort().

When I went through the code this weekend, I found just too many calls
to abort() where an error should have been returned instead. As Mark
Phippard already discovered with the ra_serf lib: abort() is called even
in situations where an url doesn't exist! Imagine FireFox would simply
close whenever a link points to a non-existing page!

> 2. There are interfaces like svn_path_basename which are much more
> convenient to use if they can be assumed to always succeed--which they
> will if they are provided with valid inputs.
>
> If you find a place where an assertion failure or abort could happen as
> a result of an external factor like corrupt data files or invalid
> command-line or network input, that's a bug.

Thanks for clearing that up. As soon as I have some time, I'll go
through the code again and point out all the places where an error
should be returned. If you like, just take my first mail of this thread:
it already contains a few places including my comments.

Stefan

-- 
        ___
   oo  // \\      "De Chelonian Mobile"
  (_,\/ \_/ \     TortoiseSVN
    \ \_/_\_/>    The coolest Interface to (Sub)Version Control
    /_/   \_\     http://tortoisesvn.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org