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

[Behdad Esfahbod]

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/