[cairo] Error handling patch

[Owen Taylor]

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
> cairo.
> 
> 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:
> 
>                 cairo_scaled_font_extents
>                 cairo_surface_finish
> 
>         Add new functions to query object status:
> 
>                 cairo_scaled_font_status
>                 cairo_surface_status

These look OK to me.

>         * src/cairo-win32.h:
>         Change function to return type of void:
> 
>                 cairo_win32_scaled_font_select_font

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
return.

>  * 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.

Regards,
						Owen

* 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:

  if (scaled_font->status)
    _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
  (cairo_ft_scaled_font_lock_face)

* In several places you have something like:

   /* create image surface */
   if (image->base.status)
     return image->base.status;

  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
  returning?" 

  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 
  error pattern.

* cairo-png.c:read_png() leaks 'data' if
  cairo_image_surface_create_for_data() fails.

* 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
  not CAIRO_IN_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
  things.

* 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
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050727/c3ff79df/attachment.pgp