Re: Some comments about GVFS

On Thu, 03 May 2007 14:11:02 -0400, Havoc Pennington wrote:
> Yeah, I think this keeps the Cairo model from being "fully general" - it
> only works for some types of operations or objects.

Sure. If you want the object to remain usable after an error, then it
shouldn't shut down. And note that that with cairo, we do have
functions that do return an error indication instead of shutting down,
(cairo_surface_write_to_png, for example).

So I think a fair characterization of the "cairo model" is that it
recognizes that you don't always do the shutdown-on-error thing, but
it is darn handy when it's the right thing to do.

>                                                     Another thing about
> the Cairo model is that it's either weird or possibly broken if an
> object is used by multiple threads, which could arise in the current
> gvfs presumably.

For cairo, at least, there's nothing broken here. If you want to use
the same object from multiple threads, then you need to be doing your
own locking. As for weird, the shutdown thing should only be happening
if there's really no other appropriate response. There's a difference
between causing an error _in_ an object, (in which case it will
shutdown, and it's appropriate for it to be shutdown for all threads),
as opposed to causing an error _with_ an object, (in which case it
should just return the error indication and not shutdown).

> Another thing that's needed for gvfs for sure but not for Cairo perhaps
> is the error message.

The model of lodging errors inside the objects doesn't prevent you
from putting as detailed a message in their as you'd like. With cairo
itself, we don't actually store anything more than an enum, but...

> 8859-1"), but nonetheless in a lot of cases I think a detailed error can
> be better than a strerror-style string. Sometimes the error will be
> user-helpful, e.g. "You aren't allowed to save to directory Foo"

... even with the very strerror-style approach that cairo encourages,
it's very easy for the calling application to add context like this
when generating its error message.

> Returning *helpful* errors, even if only to programmers and techies,
> also frequently depends on context - meaning, the error can be more
> helpful if it's checked right away after every call.

Sure. So everytime you want to emit a useful error message, check the
status and generate the message with all the context you want.

Or, like I said above, you could envision applying the cairo model and
also adding a string to it. (That is, if applying the cairo model
pushes the GError from a parameter to every function to instead be a
field within the object, that doesn't mean that GErrror needs to
become any less capable).

> Finally, Cairo of course mixes programmer errors (g_return_if_fail in
> GLib-world) with should-be-reported errors (GError in GLib-world) with
> unavoidable-and-should-be-ignored errors (just get ignored in
> GLib-world). See GError docs for how these are separated in GLib:

I definitely agree with you that there are different classes of errors
and they require different kinds of handling. And I won't argue that
cairo gets all of this right yet. (In a very real sense I've always
felt that cairo's error-handling strategy was a big experiment, and it
would be interesting to see how it played out. So I'm quite glad to
see this discussion happening around it.)

In the meantime, it's not clear to me that either cairo or glib has
this all figured out yet. For example, the document above says:

	First and foremost: GError should only be used to report
	recoverable runtime errors, never to report programming
	errors. If the programmer has screwed up, then you should use
	g_warning(), g_return_if_fail(), g_assert(), g_error(), or
	some similar facility.

For that second sentence, if we're talking about a non-recoverable
programmer error, then what guidance is there for choosing
g_return_if_fail as opposed to g_assert? Are programmer errors
actually divided into two classes?

I could imagine a consistent argument that all programmer errors
should lead to an abort, (to force errors to get noticed and dealt
with early). And I could accept that some people would want to
configure the abort away as well, (which G_DISABLE_ASSERT allows for
example). So I could imagine a consistent argument that g_assert
should be used for all detected programmer errors.

But how does g_return_if_fail fit into your model? Isn't it really
doing basically the same thing as cairo's inert object functions?
Differences I see are that:

	1. g_return_if_fail prints a message

	2. cairo's inert objects come with a guarantee that once an
           object shuts down, no future call to that object will have
           any effect.

> For me when using Cairo drawing I would say should-be-reported errors
> aren't really possible. When the problem is that I passed nonsense to a
> Cairo API, I would prefer some verbose warning to the silent failure
> plus an error code I have to figure out the origin of.

So all you really want is a mode to make cairo be noisy on the first
detected error? That would be a simple change to the _cairo_error
function through which all of the shutdown-an-object errors should be
passing. People have mentioned wanting it before, but I haven't cared
about it enough to add it myself.

I do feel that cairo should not print anything unless it's about to
abort, or unless the user has explicitly requested it by setting some
environment variable or something.

But a little noise isn't really enough information to tell you where
the error came from, so you're still going to have to track it
down. What you'd really like is a nice application-level stack trace
from when the error gets detected by cairo, (hopefully people using
bindings are getting this already). Me, I just use gdb, set a
breakpoint on _cairo_error and trigger the problem to find the

>                                                         When the problem
> is something unavoidable (say not enough memory or whatever) then I just
> don't care, I'm not going to check the error code for this or in any way
> recover. So in neither case here do I find the error codes useful.

And here's the other big disagrement between you and me.

You're free to write applications that don't care about out of memory,
(most of mine don't---but I only write toys). But that's definitely
not a decision I'm willing to force on to all users of a system
library like cairo.

So the fact that some users want to be able to recover from this and
some don't care is definitely a case where cairo's error-handling
strategy really helps. Note that both users can still benefit from
having the warn_unused_result attribute turned on for every cairo
function that returns a value. (We actually had a paatch committed
during 1.4.x that added that attribute, but I've deferred it until

> When the programmer _should_ check the error, e.g.
> cairo_surface_write_to_png, I think GError-style reporting is ideal, and
> the return code as write_to_png has is OK and does the job and is pretty
> much equivalent to GError except there's no memory management (plus) and
> no error message (minus).

I'll agree that the memory management of GError is a huge minus. If it
makes me old-fashioned to care about providing an interface that's
easy to use from C, so be it.

>   - cairo model is most usable when it can be correct to ignore errors,
>     which is particularly common for programming errors
>     (GLib would use return_if_fail/assert) or unavoidable runtime errors
>     that are best handled by simply ignoring them

Yes, that's when shutdown-the-object works best.

>   - cairo model should perhaps be extended to support an error message
>     for more general applications (since in the drawing case the idea
>     is to almost always ignore errors, there's no point, but for file
>     and network ops, there may well be)

Sure. Shove a string in there if you want it.

>   - cairo model raises issues when the error state is shared by
>     multiple threads accessing one object

Looks like a non-issue to me. If the object is only getting shutdown
due to an error you want to ignore, then this shouldn't be a problem.

>   - when it's really probably wrong to not check errors (e.g. when
>     saving a file), a GError-type model with a function arg you must
>     consider, or a return value with warn_unused_result as Carl mentions,
>     is better than the cairo_t store-error-on-object model IMO

Yes. And the cairo library explicitly doesn't use
store-error-on-object for thiese cases.

>   - I personally believe programmer errors should get the return_if_fail
>     or assert type treatment and not be runtime-detectable, because
>     a nice warning is more helpful to the programmer or in bug reports,
>     and runtime detecting these is just wrong anyway - fix the bug, don't
>     recover from the bug by adding bug-handling code

I agree that programmers should fix their own bugs rather than writing
new code to recover from the bug. I don't see anything in the cairo
model that encourages writing bug-handling code.

> It's very very important IMO to distinguish the three types of errors
> (programming, recoverable, and unavoidable-and-ignorable) when designing
> any API in this area.

I agree it's important to distinguish. And for the three cases:

    Programming errors

	Glib seems inconsistent about these, (g_assert provides a
	reliable way to handle them, but sometimes g_return_if_fail is
	used instead which is inherently unreliable).

	Cairo tries to shutdown the affected object whenever possible,
	(choosing to just segfault instead if the user didn't even
	pass a valid object in the first place). But not that the
	"invalid object" is something that can't even arise from cairo
	itself, (which _is_ actually a benefit of the
	never-return-NULL-from-constructors approach).


	Glib and cairo are similar if the application author is
	checking the status after every call, (but for the memory
	management and error message points noted above).

	Cairo does allow for the error-checking to be deferred if


	Glib considers this category to exist, and aborts.

	Cairo does not admit the existence of this category. Instead
	cairo uses the same shutdown strategy as is used for
	programming errors. This allows recovery to be handled in the
	most convenient way possible by the application, (again,
	taking advantage of deferred error checks).

So that looks to me like if you modified _cairo_error to do the
equivalent of g_assert, you'd basically get what you want for your
recoverable and unavoidable cases. But that would leave you with
always having g_assert and never g_return_if_fail for programming
errors though. So please explain more to me about how that handling
should be distinguished.


Attachment: pgpEV2sx4hG9L.pgp
Description: PGP signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]