[cairo] Error handling patch
otaylor at redhat.com
Wed Jul 27 12:36:06 PDT 2005
On Tue, 2005-07-26 at 19:30 -0700, Carl Worth wrote:
> Here's a small change to finish out the error handling strategy for
> The API changes are small:
> * src/cairo.h: Add CAIRO_STATUS_INVALID_CONTENT,
> CAIRO_STATUS_INVALID_FORMAT, and CAIRO_STATUS_INVALID_VISUAL.
> Change functions to return type of void:
> Add new functions to query object status:
These look OK to me.
> * src/cairo-win32.h:
> Change function to return type of void:
I don't like this one much - explained in my notes below; since it's
fairly specialized I guess it could be sacrificed to the general
rule, though. Another possibility would be to give it a boolean
> * One goal of this work is that no function returning a
> cairo_<object>_t* will ever return NULL. I haven't done complete
> auditing to guarantee this, but if I missed anything, then that
> will be an easy bug/documentation fix that will not affect API.
> Note that there are still cairo function calls that can return
> NULL, particularly the few that return pointers to non-cairo
> objects such as cairo_ft_scaled_font_lock_face.
cairo_font_face_t objects don't seem to have been done in your patch.
OK. Here's a bunch of notes. Most of this stuff is details; a few
of the issues here (especially about whether you need to free surfaces
that you create and are returned with errors) are more global.
* In several places in cairo-font.c, you added
if (scaled_font == NULL) return; - silently doing nothing a NULL
font seems dubious to me ... wouldn't a segfault be more useful
to the programmer than that?
* For cairo_scaled_font_extents(), if called on a error'ed font,
extents is left uninitialized. This could lead to things like
allocating gigantic temporary surfaces. Wouldn't setting the
extents to empty be better?
* For cairo_scaled_font_t, since handling EOM on font creation
is left to the backend, the resulting empty font is stored in
the font cache ... this seems like a bad thing to me, and
makes recovery impossible. I think the cache needs to be bypassed
in some fashion for that scenario.
One way would be to leave the backend returning NULL and make
the frontend return a "generic" empty object. If we always
check status before we check type in subclass methods, there
wouldn't be a noticeable difference. Or we could test the
refcount on the return.
* In several places, you have:
_cairo_scaled_font_set_error (scaled_font, scaled_font->status);
[ same for cairo_pattern, etc. ]
Is this really useful compared to just returning? If we make
_cairo_error() debug spew, then we'll get tons and tons of irrelevant
debug spew after the first root cause error.
* I don't see how errors in computing glyph extents are handled;
if the backend's scaled_font_font_glyph_extents() fails, is it
supposed to set an error on the font? If so, why is there a
cairo_status_t return from that virtual function?
* NULL should generally be written %NULL in gtk-doc doc comments
* In several places you have something like:
/* create image surface */
When I read it I wonder "could image->base.status be anything other
than NO_MEMORY? If so, shouldn't we be freeing the image before
If we are making the guarantee that all creation functions succeed
or return the nil object and never return an errored object (it would
be easy to slip up on that, so perhaps there should be a style habit
of adding assertions at the end of create functions) then it
seems clearer to just return CAIRO_STATUS_NO_MEMORY directly.
In particular the existence of _cairo_surface_create_in_error()
makes me wonder if these are just leaks ...
* What happened to error handling for font faces in cairo-ft-font.c?
* glitz-surface.c returns _cairo_surface_nil in various places without
calling _cairo_error(), similar stuff in cairo-font.c.
* Why does cairo_image_surface_create() return
CAIRO_STATUS_INVALID_FORMAT but cairo_image_surface_create_for_data()
return INVALID_CONTENT for the same if (!CAIRO_FORMAT_VALID (format))?
* I don't quite understand why you are exporting
_cairo_image_surface_composite() - to remove an indirection?
* Docs for _cairo_scaled_font_set_error(), _cairo_pattern_set_error(),
_cairo_surface_set_error have scaled_font->->status etc.
* I don't understand setting the error on the pattern *again* in
_cairo_pattern_create_rgb/rgba if create_solid() returns a
* cairo-png.c:read_png() leaks 'data' if
* cairo-ps-surface.c:_cairo_ps_surface_create_for_stream() checks
_cairo_meta_surface_create() return against NULL
* Docs for __fallback_init() mention CAIRO_STATUS_NOTHING_TO_DO
* Your change to _cairo_surface_allocate_clip_serial() makes it
violate its docs ("will not return 0")
* I feel that cairo_win32_font_select_font() probably should be
left with a cairo_status_t return, since you *are* really supposed
to check that. If you didn't select the font you thought you
were going to select, following GDI commands may do very unexpected
* You changed the return of _compute_a8_mask on error (I don't
think this was really necessary) but didn't change the code
that checked that return value.
* Error handling for Win32 font faces is also missing
* cairo_get_target() has different memory management semantics
when it turns _cairo_surface_create_in_error(cr->status);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050727/c3ff79df/attachment.pgp
More information about the cairo