GNOME Bugzilla – Bug 705629
secret_service_clear_finish returns NULL error on failure
Last modified: 2013-08-08 11:27:40 UTC
secret_service_clear_finish says: * @error: location to place an error on failure * * Returns: whether the removal was successful or not So it should always set the error when returning FALSE. This is not currently the case if nothing is deleted: return closure->deleted > 0; that returns FALSE when closure->deleted == 0 but no error is set. This causes empathy to crash in bug #692105. I'm not sure if we should return TRUE in that case or set an error and keep returning FALSE.
Created attachment 251099 [details] [review] Clarify documentation for secret_service_clear_xxx()
Attachment 251099 [details] pushed as 5c9001a - Clarify documentation for secret_service_clear_xxx()
I think this should explicitly call out the fact that it isn't following the documented GError convention, something like: > secret_service_clear_finish() > > Finish blah blah blah secret service. > > Note that this function may return %FALSE without setting @error, > which indicates that no items matched the desired @attributes. or perhaps > Returns: %TRUE if one or more items were removed, %FALSE > without setting @error if no matching items were found, > or %FALSE with @error set if removal was unsuccessful. I'm not sure whether g-i bindings will deal with this very well?
Sure, happy to take a patch which makes that explicit. However this is standard practice in GLib APIs. Returning NULL or FALSE doesn't necessarily mean a recoverable GError style exception has occurred. The indication whether an "exception" occurred is always @error, and from what I've seen gobject-introspection handles it that way. It would be pretty broken if it didn't.
(In reply to comment #4) > Returning NULL or FALSE doesn't necessarily mean a > recoverable GError style exception has occurred. That's what the GError docs say about NULL, but not what they say about FALSE (and the C coding style that seems to be encouraged is to look at the return, not at the GError - which led to Bug #692105). On Bug #660809, Matthias said: > In general, that is the pattern we want to follow though. > Any counterexamples are considered broken and should be > explicitly documented as breaking this pattern. I suggested a patch for this (and other issues); Colin didn't like my wording for this specific part, but seemed happy with the general idea. Colin, Matthias, is this still your understanding of the policy/convention or has it changed?
Indeed, g_key_file_has_key also says: > Note > > This function does not follow the rules for GError strictly; the > return value both carries meaning and signals an error. To use this > function, you must pass a GError pointer in error, and check whether > it is not NULL to see if an error occurred. That wording might be better than what I suggested.
Well, in my opinion it's really hacky that %FALSE return value is tied to whether GError was set. Non C code (like vala) cannot make guarantees like this, and it would be really broken if it did (think about it, in Vala you can easily return %FALSE from a function declared with a boolean value, without raising an exception). If you want to use @error then check whether @error was set or not. But, if the glib community is going to paint themselves in a corner here, then I guess I'll try to stick to the convention. So as a work around: I'd accept a patch that changes the gboolean return value to a gint return value indicating the number of passwords cleared, for the following functions: * secret_service_clear_finish() * secret_service_clear_sync() * secret_password_clear_sync() * secret_password_clear_finish() Yes, the intent *is* that failing to find anything to delete is *not* a GError. This is pretty obvious when you think about it, designing it otherwise would just be racy.