Re: Some comments about GVFS

[Havoc Pennington <hp redhat com>]

Hi,

Carl Worth wrote:
> 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.

"cairo model" is probably confusing - write_to_png is a more typical and 
essentially GError-equivalent model, while "cairo_t model" is the thing 
that people mean is unique to Cairo.

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

Of course. The case where this would break is an object with its own 
locks, e.g. DBusConnection. I'm just guessing that gvfs has its own 
locks since I believe gnome-vfs did.

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

There is an official GLib/GTK+ policy on this, which is that g_assert is 
used only for bugs in the library, and return_if_fail is used for bugs 
in the app. It is missing from the docs indeed.

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

D-Bus in fact does this unless you disable it by env variable. It is 
pretty controversial, though. Or at least there's a vocal group who 
doesn't like it.

Obviously I think said group is wrong ;-)

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

The big differences are 1) that it prints a message and 2) that all 
behavior is undefined after a return_if_fail - GTK doesn't contain 
handling code that frees resources or tries to recover or puts objects 
in a no-op state as a result of the return_if_fail.

I don't think 2) really matters to library users, it only affects 
library maintainer effort.

> So all you really want is a mode to make cairo be noisy on the first
> detected error?

Well, if and only if the error is a programming error. That's the 
problem, I don't want noisy in write_to_png, nor do I want noisy if the 
error is something I should be handling or is something I'm legitimately 
ignoring. I only want noisy about bugs.

What D-Bus does in a couple of cases (and GTK may have one or two 
examples of this also, iirc) is that if you pass NULL to ignore the 
DBusError/GError, it is noisy, and if you don't pass NULL it is not 
noisy. However, this is only OK if passing NULL when an error occurs 
amounts to a programming bug, i.e. when you are only allowed to pass 
NULL if you know errors are impossible. If it would be legitimate and 
normal for the error to be silently ignored in some cases, you can't be 
verbose when one happens. I can't actually find an example in the code 
quickly, but I swear we do this in a couple places.

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

For example say you pass nonsense values; the warning can be 
"function_name: assertion failed: arg_foo != 0.0" so I look at my calls 
to function_name where arg_foo might be 0.0.

With cairo_t drawing code, you have to add temporary code to check the 
status and print errors, then to figure out which function sets the 
error you have to keep moving this temporary code around, then you have 
to figure out what the status means once you know which function sets 
the error status.

Setting a break on _cairo_error is certainly easier (thanks for that 
tip) but the reason I've never done that is that I did not know cairo 
had an internal symbol that was always used to set errors ;-) for the 
analogous situation in gtk, gdk_x_error, the warning when an X error is 
received has something in it like "try setting a break on gdk_x_error" 
even, or used to.

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

D-Bus does this on glibc systems - GNU libc has a function to get a 
backtrace for you. So the warning includes the function, the line, the 
assertion or precondition that failed, and a trace.

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

I don't disagree with you on this. D-Bus as another low-level library 
also allows OOM handling.

I do think OOM handling would be insane in GTK+ purely because it would 
be intractably hard, but for cairo it makes sense to me.

I think the cost of OOM handling in D-Bus is about a 30% code size 
increase, and an absolute requirement to have substantial unit test 
coverage that tests the OOM codepaths. For GTK+, I think it's an 
intractable problem because the unit testing is intractable or at least 
a monumental task. You could write the 30% code size but 10% of that 
bloat would be broken and never get tested. And apps would never check 
for OOM anyway.

In cairo I think ignoring OOM silently by default and allowing people to 
check for it if they care is right.

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

Agreed. Please note, I don't have anything against the cairo_t model for 
cairo_t (though I do wish it was not used for programming bugs, or at 
least that optionally it was not used for programming bugs, or 
optionally warning spam with function name, arg name, and assertion 
failed could be printed for programming bugs). I only followed up to 
point out that it isn't globally better and specifically I doubt it's 
right for many things in gvfs.

> I'll agree that the memory management of GError is a huge minus. 

But it's also required to get an error message, unless you use globals 
or happen to have a context object to stick the message to, like 
cairo_t. If GError were only an error code it would have no management.

Also GError only has to be freed if an error in fact occurs, and most 
GError-using functions also return a success/failure bool so if you 
don't care about the specific error that happened, you can pass NULL to 
avoid the GError and just check the return value.

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

In GLib/GTK it's library bugs vs. app bugs, D-Bus maintains the same 
distinction for _dbus_return_if_fail vs. _dbus_assert *but* D-Bus makes 
them both fatal so it doesn't matter much anyhow.

Havoc