Re: Some comments about GVFS


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

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.


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