[cairo] Cleaning up the various set_error functions (bug #4094)

Behdad Esfahbod behdad at cs.toronto.edu
Fri Aug 19 12:14:40 PDT 2005


Hi Carl,

Nice to have this work.  BTW, the very first code change doesn't
look right to me.  It just adds about not overwriting the status,
but no code accompanying the comment :)  I mean in
_cairo_scaled_font_set_error.
didn't look at the rest of the patch though.

BTW, When overwriting is due to happen may be a good alert of
bugs in cairo itself, so you may want to have a preprocessor
macro for that and print out stuff for a while.

behdad


On Fri, 19 Aug 2005, Carl Worth wrote:

> Here's a cleanup in response to:
>
> 	https://bugs.freedesktop.org/show_bug.cgi?id=4094
>
> The most specific bugs mentioned there were fixed in the recent fixes
> for cairo_pattern_t error handling. But there was a general pattern of
> this bug occurring throughout the implementation.
>
> Here's what is hopefully a complete fix.
>
> The old pattern was that object entry-point functions, (whether public
> or private inter-module functions), had a check looking something like
> this:
>
> 	if (object->status) {
> 	    _cairo_object_set_error (object, object->status);
> 	    return; /* ... and, rarely, a value here. */
> 	}
>
> Here, the _cairo_object_set_error function is intended to do two
> things. First, it lodges a status value into object->status. This
> happens to be redundant in the pattern above. The redundancy seems
> harmless here, but is actually an error since the object may be a
> "nil" object which is read-only.
>
> Second, set_error calls _cairo_error to do whatever error notification
> we might implement in cairo. As it turns out, this second purpose is
> also redundant in the pattern above, since we're at a module
> entry-point, so the object->status error must have been set (and
> reported) from the return value of a previous call into this
> module. And this redundancy is also harmful since it will result in
> redundant error reports to the user which will distract from the root
> cause.
>
> So the patch changes occurrences of the pattern above to:
>
> 	if (object)
> 	    return; /* ... and, rarely, a value here. */
>
> The patch also puts an extra precaution within all of the set_error
> objects, which is to avoid overwriting object->status if it is
> previously set to an error status. This would independently solve the
> read-only bug.
>
> The change here is not complex, but it does touch a fair amount of
> code, so it wouldn't hurt if someone glanced over things.
>
> -Carl
>
>

--behdad
http://behdad.org/


More information about the cairo mailing list