[cairo] Consistent error handling [draft patch]

[Carl Worth]

On Sun, 08 May 2005 15:58:44 -0400, Owen Taylor wrote:
> There are various questions about the details of the approach:
> 
>  - Do we really want to apply it to *all* objects? It makes
>    a lot of sense for cairo_t and cairo_pattern_t, but I'm 
>    not so sure about fonts and surfaces.

Surfaces are the object I most cared about adding this feature to.

Now that using cairo always requires managing two objects, (a
cairo_surface_t and a cairo_t), we won't ever get the actual benefits
of this error handling strategy unless both objects support it.

>    My concern with these objects is basically that there is
>    frequently an implied connection between the cairo object
>    and underlying non-cairo objects, and returning a cairo
>    object that *doesn't* wrap the non-cairo object seems like
>    breaking the contract.

Sure, but it doesn't really break the contract any more than NULL does.

>    E.g., if I call cairo_image_surface_new_for_data(), getting
>    back a surface that reports a height/width other than what
>    I passed in might break the calling application in subtle
>    ways, rather then the obvious ways if it returned NULL.

The error status will still be there and it will get propagated onto
the cairo_t if the surface is ever used as a source or a target. So I
think that eventually it will be obvious that an error has occurred at
some point in the past.

There's still the outstanding problem that it's currently difficult
for a user to associate a detected error back to the original
operation that triggered it. I want to provide something better for
that, but that is an orthogonal issue as to which objects use this
error scheme.

>  - What to do about cairo_copy_path()? While that could return
>    a special no-memory empty path, it might be more confusing  
>    than just returning NULL, since the path data wouldn't be
>    what was expected.

Actually, cairo_copy_path already does return a special no-memory
path. I had added this function after we had decided on the current
plan to extend the error-handling scheme to more objects.

The nice side effect is that we now have two independent
implementations of these special objects, so I'll compare the two and
see what interesting differences show up.

>  - I've used the convention of ref_count == (unsigned int)-1
>    to mark the no-memory objects.

Sounds fine. With cairo_path_t there is no reference count, so that
didn't come up there.

>  - I used separate no-memory objects for the different subtypes
>    of pattern.

This seems fine to me.

>  - I left the cairo_status_t returns on 
>    cairo_pattern_add_color_stop_rgb(), cairo_set/get_matrix(),
>    etc. To be consistent with cairo_t, they probably should
>    be removed.

Yes, they should. I'll do this.

>  - I made internal functions like _cairo_pattern_acquire_surface()
>    return pattern->status if set, but the structure of the code
>    is such that errored patterns should never get there.
>    (cairo-gstate.c checks before calling down to the lower code)
>    So, assertions might make more sense.

Yes. Internal consistency checks should be assertions, not new code
paths.

>  - There is no testing of the code, and thus, I don't trust it any
>    more than I trust the current out-of-memory code paths (that is,
>    not at all.) The only real way I know of testing out-of-memory
>    code paths is the D-BUS approach of having a test suite that
>    retries a test successively failing malloc 1/2/3/4... 
>    getting that going could be pretty painful, though, especially
>    as libpixman/cairo don't have facilities for replacing malloc/free.
>    (It could be done GNU-libc specific using hooks there.)

I'd love to have that kind of test infrastructure in place. It doesn't
strike me as being all that much work really. (And I don't mind if
pieces of the test framework like this are less portable than the
implementation in general).

> I think the patch is reasonably workable as far as it goes, but 
> review of the approach and comments on extending it to
> font and surface types would be useful.

I've got a few simple notes below, and I'll follow up later with an
updated version of the patch, (which will likely add surface support
as well). I'm planning to leave the fonts alone for the moment, (just
because I'm not as personally familiar with the cairo font code, and
not that I necessarily think it shouldn't get the same treatment).

> +#define PREDRAW_CHECKS()					\
> +    if (gstate->source->status)					\
> +         return gstate->source->status;				\
> +    if (gstate->surface->level != gstate->surface_level)	\
> +	return CAIRO_STATUS_BAD_NESTING;

Eww... hiding a return statement in a macro is pretty nasty. That kind
of thing can lead to some missed cleanup code later on. Speaking of
which, one of the goals I have for this error handling work is to go
through and basically eliminate goto-based bailout code. I'm
interested to see how much of that we can do with failsafe objects.

> +static const cairo_solid_pattern_t no_memory_solid_pattern = {

For cairo_path_data_t, the name of the object I came up with is
_cairo_path_nil. I ended up without a static object since there are
functions in cairo.c that want to return it. But I think a static
object named cairo_solid_pattern_nil would work just fine here.

> +cairo_status_t
> +cairo_pattern_status (cairo_pattern_t *pattern)
> +{
> +    return pattern->status;
> +}

Now that we're adding a second function that returns cairo_status_t,
we'll definitely want a general function to do the cairo_status_t ->
string conversion, (as recently mentioned in a separate thread).

>  void
>  cairo_pattern_destroy (cairo_pattern_t *pattern)
>  {
>      if (pattern == NULL)
>  	return;
>  
> +    if (pattern->ref_count == (unsigned int)-1)
> +	return;

Oh, thanks. That reminds me that I missed the equivalent test in
cairo_path_destroy.

>  cairo_status_t
>  cairo_pattern_get_matrix (cairo_pattern_t *pattern, cairo_matrix_t *matrix)
>  {
> +    if (pattern->status)
> +	return pattern->status;

These should all be void. But should we leave the matrix alone in this
case, or initialize it to some known value? (And similarly for other
function that accept return pointers or that return values).

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050524/baeb00f5/attachment.pgp