[cairo] error handling questions

[Behdad Esfahbod]

On Tue, 2007-07-03 at 10:32 -0700, Carl Worth wrote:
> On Sun, 01 Jul 2007 15:47:18 -0400, Behdad Esfahbod wrote:
> > On Mon, 2007-06-18 at 16:52 -0400, Vladimir Vukicevic wrote:
> > > We've been using CAIRO_STATUS_NO_MEMORY as "unknown error" in a bunch of
> > > places; I know that it's generally been the policy to use a real error
> > > code, but would anyone mind if I were to introduce
> > > CAIRO_STATUS_UNKNOWN_ERROR (or CAIRO_STATUS_FAILURE) so that we can
> > > distinguish between real OOM and unknown errors?
> >
> > I'm in favor.
> 
> Abusing NO_MEMORY was bad, inexcusable, and must be fixed. But adding
> UNKNOWN_ERROR is not any better, (and worse in that it actually
> _looks_ like it's intended as a random catch-all).

Your reasoning works by assuming that defining no catch-all status makes
people handle errors properly.  That has already failed.  An
unknown-error status on the other hand, can be used pretty much like the
current NO_MEMORY is (ab)used, but has the following benefits:

  - Is not confusing to users.

  - Lets developers grep for CAIRO_STATUS_UNKNOWN_ERROR and try to fix
one or two when they have a few minutes.

The second one is a huge advantage.  Right now we are stuck in a
situation that someone's got to go over all CAIRO_STATUS_NO_MEMORY
instances in cairo and check them...

> I don't want random catch-all error status values. If we wanted that,
> we could just reduce the enum to a Boolean and be done with it.

Just because there is an UNKNOWN_ERROR status define doesn't mean all
errors should return it.

> I want the error status values to contain as much information as
> reasonably possible. If the problem is that there's some backend that
> doesn't provide suitably detailed error information, then that's what
> the cairo error status should reflect, (CAIRO_STATUS_GDI_ERROR or
> whatever).
>
> > Any place that an error is initiated should call _cairo_error().  it's
> > not like that for many error paths right now and those should be fixed.
> > We don't have a strict rule though, I'm sure there are cases that
> > _cairo_error should not be called (for UNSUPPORTED only?).  Carl may
> > know how this is supposed to work,
> 
> If an error is getting lodged into an object or returned to the user,
> then _cairo_error must be called, (and ideally just once). It
> shouldn't ever be called at any other time.
> 
> > but I agree that our error handling is still a bit messy.
> 
> Yes, definitely. Chris recently, (or not so recently... I'm horribly
> behind on things), pulled together a document describing the
> error-handling strategy culled from a discussion between Havoc on
> myself. I think that's useful and I'm hoping to get the cairo wiki
> re-enabled _very_ shortly so it will have a place to live, (and so it
> can get incorporated into cairo's documentation).
> 
> But what would be even more useful would be to go over that discussion
> again and pull out the things that cairo's current error-handling
> implementation is missing. What makes it hard to track down and fix
> problems right now?
> 
> > Also, feel free to make _cairo_error() do something if CAIRO_DEBUG env
> > var is set.  Options are raising SIGTRAP and printing a backtrace.
> 
> And that's probably a big part of the answer, (especially the
> backtrace printing). I think a lot of what Havoc (and others) had to
> complain about was failure without cairo giving more information,
> (timely and detailed), about the problem.
> 
> > builds and it has been very handy.  The only thing I don't like here is
> > that to produce useful backtraces we need to use Jeff's
> > backtrace_symbols().
> 
> What's the concern here? Is it a lot of code to bloat the library?
> Less portable than would desired? Something else?

360 lines.  Not portable, probably not even to all versions of libbfd.
But main reason that I don't like it is that it's something that glibc
should have got right in the first place.  Doesn't make any sense for
apps/libs to have to provide their own :(.

(While we get glibc fixed, I should go ahead and propose including it in
glib...)

> -Carl

-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759