GNOME Bugzilla – Bug 660371
is it ever valid to have 0 as a GError domain?
Last modified: 2012-01-16 04:30:40 UTC
g_error_new() criticals if the domain is 0; g_error_new_valist() doesn't. They can't both be right. g_set_error_literal and g_set_error, respectively, inherit this. My personal opinion is that 0 can't/shouldn't be a GError domain (it stringifies to NULL, which isn't a string). Cc Dan Williams because of <https://bugs.freedesktop.org/show_bug.cgi?id=27799> in dbus-glib, in which he added a regression test involving a GError with domain and code 0 in order to test a fix for a crash seen in NetworkManager. In the dbus-glib case, I think there's a distinction between: * error domains registered with dbus-glib (like MY_OBJECT_ERROR in our tests), which are mapped to a D-Bus error name according to their registration * error domains not registered with dbus-glib (I used G_IO_ERROR in a new test), which I suspect may be what Dan actually saw in NetworkManager, and are mapped to something like o.fd.DBus.GLib.UnmappedError.g_io_error_quark.Code42 * error domain 0, which is currently tested in dbus-glib, but makes little sense, and I think it should be considered to be programmer error * error domains that aren't valid quarks (nonzero integers where g_quark_to_string would return NULL), which I think should be considered to be programmer error
In current g_dbus_error_encode_gerror(), the first two categories in Comment #0 work correctly (and analogously to dbus-glib), and the last two categories segfault. If those last two categories are thought to be programmer error, maybe g_dbus_error_encode_gerror() should have more g_return_val_if_fail(); if they're thought to be valid, that function is among those that need fixing.
I agree that error domain of 0 should be considered an error. We use it internally in some places in NM due to laziness, but we've attempted to ensure that these errors do not make it back out over D-Bus. Any occurance of unknown/unmapped errors in NM making it into dbus-glib should be considered a bug in NM that we'll fix. Obviously it would be a lot nicer if it was simpler to create GError domains instead of the 30 - 40 lines of boilerplate pastecode that you have to do every time you make a new domain with a few errors...
(In reply to comment #2) > we've attempted to ensure > that these errors do not make it back out over D-Bus. Any occurance of > unknown/unmapped errors in NM making it into dbus-glib should be considered a > bug in NM that we'll fix. I have no objection to unmapped errors - we map those to org.freedesktop.DBus.GLib.UnmappedError.nm_error_quark.Code42 or something and don't crash[1]. The recipient gets an incomprehensible error, but it's no worse than "Failed", and a well-implemented client should treat it as equivalent to "unknown error". GDBus does this even better (arguably), by using a reversible encoding, so clients also using GDBus can recover the original error, although clients using dbus-glib/Qt/Python/whatever still suffer a bit. The thing I object to (and want to turn into a critical) is alleged domains that aren't even a valid quark. I'll sketch out some patches for GLib and GDBus later today. If the laziness you refer to is "too lazy to define a useful error code right now", perhaps you could replace (0, 0) with something uselessly generic but valid, like (NM_ERROR, NM_ERROR_FAILED) (assuming you have at least one error domain already), or even (G_IO_ERROR, G_IO_ERROR_FAILED)? > Obviously it would be a lot nicer if it was simpler to create GError domains > instead of the 30 - 40 lines of boilerplate pastecode that you have to do every > time you make a new domain with a few errors... Indeed; particularly if you want dbus-glib and/or GDBus to understand it. Perhaps this could be streamlined with some sort of varargs-based DSL? [1] Well, actually we do crash if the code is negative, but that's a bug, which I fixed recently (pending review).
0 is not a valid domain. If that is not enforced everywhere, it is an oversight.
See also bug 560482 which was about the same thing.
Places with g_set_error (x, 0, ...) according to Google Code Search: * a couple of GLib regression tests (oops!) * beast * blinkenlights.de projects: blinkensim, blccc * bluez (but apparently only in some sort of embedded mini-GLib, so perhaps this can be ignored) * evolution 2.7.92 and older * gimp 2.2.3 and older * gnome-media 2.13.5 and older * gnome-panel 2.18 * gst-plugins 0.8.8 and older * Gtk2 Perl bindings * gtk/gtktextbufferrichtext.c in something claiming to be gtk * hal 0.5.8 on FreeBSD * <http://gconf-cleaner.googlecode.com/> * <http://ogmrip.sf.net> * <http://rdffs.googlecode.com/> * <http://turnpike.googlecode.com/> * imsettings * jtalk * libgda 1.9.100 * libgda 3.0.0 and older * libgnomedb 3.0.0 * libgpod * LogJam on Win32 * mail-notification 4.0 and older (on savannah) * ModemManager of unknown age * NetworkManager of unknown version * quarry (on gna.org) * sound-juicer 2.15 * streamtuner (on savannah) * totem 2.17 * upower on FreeBSD Bonus points for hard-coding 1 as an error-domain quark that is likely to work, but semantically wrong: * app-install * upower * zif Perhaps we need to compromise and g_warn_if_fail()?
Created attachment 197771 [details] [review] GError: don't allow 0 as a domain --- Probably not a good idea to apply this, sadly - people still use it a lot.
Created attachment 197772 [details] [review] g_dbus_error_encode_gerror: don't segfault on bad domains
Created attachment 197773 [details] [review] sleepy-stream test: use a real GError domain
Created attachment 197774 [details] [review] markup-subparser test: use a real GError domain
Review of attachment 197771 [details] [review]: Sadly true; we can keep it in mind for glib 4
Review of attachment 197772 [details] [review]: Looks good
Review of attachment 197773 [details] [review]: Looks good
Review of attachment 197774 [details] [review]: Looks good
Comment on attachment 197772 [details] [review] g_dbus_error_encode_gerror: don't segfault on bad domains committed, e60e4999b9d4904b74e1a38bd5c24b9fd047f95d
Comment on attachment 197773 [details] [review] sleepy-stream test: use a real GError domain committed as 7aad93c5b43faa383ccb609852209d480548dd64
Comment on attachment 197774 [details] [review] markup-subparser test: use a real GError domain committed as c48a0d881313676f2c215b30ba2f8674673071ad
Created attachment 197876 [details] [review] g_error_new_valist, g_error_copy: warn if domain is 0 or message is NULL Neither of those usages is valid, but there's a lot of use of 0 as a domain "in the wild", so we can't g_return_if_fail yet. --- How's this as a compromise?
(In reply to comment #18) > g_error_new_valist, g_error_copy: warn if domain is 0 or message is NULL Can we do this, as a compromise between g_return[_val]_if_fail (which would break the apps in Comment #6) and silently accepting it (which contradicts Comment #4)?
The following fix has been pushed: 6d9f874 g_error_new_valist, g_error_copy: warn if domain is 0 or message is NULL
Created attachment 205331 [details] [review] g_error_new_valist, g_error_copy: warn if domain is 0 or message is NULL Neither of those usages is valid, but there's a lot of use of 0 as a domain "in the wild", so we can't g_return_if_fail yet. Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Bug: