GNOME Bugzilla – Bug 124022
Reporting complex errors with GError
Last modified: 2018-05-24 10:29:49 UTC
I would like to be able to store "complicated" error reports in GError, making it more closely like exception handling in e.g. C++ and Java. For example, if an XML document is not well-formed, I would like to be able to pass a struct containing complete file & line number information about all of the parser errors back to my GUI and provide a sensible user interface for this. I've got this working at the moment, but as soon as I reach a GError interface I have no way of preserving the rich information, and can only save a string. So I would like to be able to store arbitrary user data in a GError. Attached is a rough idea about what I had in mind. Unfortunately it breaks GLib ABI compatibility. The idea is that you can store some kind of user data and have it copied, freed etc as the GError is copied and freed, and code can interpret it in a meaningful way if it recognises the domain and code.
Created attachment 20538 [details] [review] Patch showing proposed changes to gerror.h
Couldn't you use a dataset to attach additional data to GErrors without breaking ABI ? See http://developer.gnome.org/doc/API/2.0/glib/glib-Datasets.html
I don't see why extending the GError structure breaks ABI compat at all... we don't allow stack allocation of a GError or anything like that.
If you consider extending GError, it would be cool to have "stacked" errors, so that higher-level functions can add the context the error occurred in; see http://article.gmane.org/gmane.comp.version-control.arch.devel/49/ for an explanation of that model.
David, did you have a complete patch which adds g_error_new_with_data()/g_set_error_with_data() and handles the copying and freeing at the right places ?
Matthias: no, I wanted to see if the API would be acceptable first.
looks pretty acceptable to me. As pointed out in comment #4, it might be nice to have a way to generically handle the case that the attached data is a nested error. Comparing free_func to g_error_free would be one way to do that, I guess. Might also be nice to add a special g_error_new_with_nested_error() which would automatically supply the right copy and free functions.
Not gonna make 2.8 api freeze at this point.
Ok, would like to implement this feature, but I don't like the extra ordinary long argument list of g_error_new_with_data/g_set_error_with_data. So I'd suggest this instead: g_error_set_details (GError *error, gpointer details, GErrorCopyFunc copy_details, GErrorFreeFunc free_details); maybe paired with: g_set_error_details (GError **error, gpointer details, GErrorCopyFunc copy_details, GErrorFreeFunc free_details); The convenience API for nested errors needs some dicussion. Would it be: g_error_set_nested_error (GError *error, GError *nested); g_set_nested_error (GError **error, GError *nested); taking ownership of the nested error, or would it be g_error_set_nested_error (GError *error, const GError *nested); g_set_nested_error (GError **error, const GError *nested); internally calling g_error_copy?
I'd like to second comment 9's suggestion to use g_error_set_details/set_nested_error instead of overlong g_error_new_* variants. As for the convenience APIs, I think they should take ownership of the data/nested errors, not copy them. (In reply to comment #3) > I don't see why extending the GError structure breaks ABI > compat at all... we don't allow stack allocation of a GError > or anything like that. It seems some people are doing just that; see http://mail.gnome.org/archives/gtk-devel-list/2009-September/msg00011.html ... Note that the example cited there wouldn't break since dbus_g_method_return_error doesn't use any GError API and just accesses its known fields directly, but there may other users out there where this _would_ break. We can of course just declare that this is indeed not allowed :) Otherwise, it's back to gdataset as suggested in comment 2.
Created attachment 148768 [details] [review] patch
Created attachment 330014 [details] [review] Proposal for extending GError There is my take on extending the GError for better error reporting. It is more complex than the patches above as it adds several things - contexts, sub-errors and extra data. Context is basically an array of strings. I'd say it supersedes the g_prefix_error as it allows error handling code for better formatting/displaying the error instead of relying on formatting a library writer decided to use. I added a recommendation of adding a context to an error by the caller if the called function failed. Extra data is basically some data for which we specify the copy and destroy functions. It can be anything. I added a recommendation that only a function that creates the error should set the extra data. The callers of the function that failed shouldn't do it. Sub-errors an is array of #GErrors. It serves two roles: 1. Some operation can fail, because all possibilities to accomplish the task failed. An example would be downloading some data from a list of mirrors. If all mirrors fail, it might be useful to store the reasons what exactly failed in each mirror (context may be useful here to specify the mirror address). 2. Hiding a low-lever error (with its own extra data) behind a higher level error (with its own extra data). I extended GError in a GPtrArray style. (GRealError and stuff). I should probably add some notes that contexts, extra data and sub-errors should not be used in GErrors that are part of some struct like struct ExtError { GError error; Foo bar; }
I haven't looked at this in detail but in ostree I really often want to attach a lot of context to things like HTTP request errors, and allow the client app to print it if it wants.
I must admit that it were the errors from ostree that actually motivated me to write this patch.
I’m not sold on all of these features. I think I’d be in favour of adding either a `gpointer user_data` (plus copy and free funcs), or adding a key–value table (like with the new g_log_structured() stuff) to GErrors, but nothing more prescribed than that. One thing I would particularly like is if any expansions to GError don’t introduce new allocations. For example, extra printf()s to format strings which probably won’t actually be looked at by the caller. Specifically: • Context (the array of strings) doesn’t really help UI code which needs to format an error, as it only allows the GError to provide an additional list of strings which the UI could present. The UI really needs to construct its own presentation of the GError, and what’s useful for this is the underlying data which is relevant to the error (most likely, the format parameters which would be put into the printf() context strings you’re returning). • Extra data is what I agree with (modulo potentially turning it into a key–value store). • Sub-errors for array operations: what do these provide that an array of GErrors don’t provide? Note that typically you will also want to return successful results for the operations which do succeed, so you’re more likely to want to return an array of { gpointer some_success_object; GError *failure; } structs. • Sub-errors for chained errors: this makes documentation and matching errors a complete pain, and makes it easier to expose implementation details of your code unnecessarily. I would prefer it if people (created and) returned more specific error codes rather than a hierarchy of error codes. Do we have some really specific use cases we want to improve here? The two concrete use cases so far are in comment #0 and comment #13, and are both solved by adding a simple `gpointer user_data` (plus copy and free funcs) to GError.
Created attachment 362958 [details] [review] Proposal for extending GError, v2 Attaching an updated patch that only contains extra data. It's a bit similar to what Christian wrote, but with the GRealError stuff.
One note - I went with the gpointer instead of key-value table, because it was simpler and I'm lazy. Also, I could not use GHashTable for the key-value table, because values may be of different types. Keyed data list could be used with an additional hashtable mapping quarks to copy funcs, because value setters in GData don't take any copy function for the passed value - it is only passed when calling g_datalist_id_dup_data. We would use g_datalist_id_dup_data in g_error_copy, so that's where the ghashtable containing copy function would be needed. In general this sounds ugly. Dataset has the same problem. I suppose it is the same thing with a bit different API. I could probably extend the GDataElt structure to hold another pointer, this time for the copy func, but I'm not sure if the additional memory overhead would be acceptable - usually data sets/lists are not copied at all. I could probably write some another key-value storage for GError, but see the first paragraph.
Created attachment 363952 [details] [review] Proposal for extending GError, key-value store, v1 Attaching an alternative patch for extending GError, this time with key-value store.
Thanks for the patches Krzesimir. I’ll try and find time to look at them soon, but I want to give them a good amount of attention, so it might take me a while.
No worries and no hurry, the bug was filed 14 years ago, so it can wait a little bit more then. :) About the patches - I know that they are missing the tests. I'll add them once we decide which approach to additional data we will take in the end. For now you could view at the patches as straw man proposals.
Except that Telepathy-GLib uses stack allocated GErrors all over, we should triple check that nothing outside of these functions relies on this feature. Maybe verify if the domain is an extended domain before accessing the extended data.
(In reply to Owen Taylor from comment #3) > I don't see why extending the GError structure breaks ABI > compat at all... we don't allow stack allocation of a GError > or anything like that. Except that some code in the wild does exactly that: - dbus-glib - telepathy-glib To be fair, they just pass the structure around to functions taking a `const GError*`, which *should* supposedly be safe if we just added a pointer field.
The same API is in GLib as g_dbus_method_invocation_return_gerror() so we have no idea who else is doing this. I'd suggest not modifying the size of GError, maybe instead use a qdata ?
(In reply to Emmanuele Bassi (:ebassi) from comment #22) > (In reply to Owen Taylor from comment #3) > > I don't see why extending the GError structure breaks ABI > > compat at all... we don't allow stack allocation of a GError > > or anything like that. > > Except that some code in the wild does exactly that: > > - dbus-glib > - telepathy-glib > > To be fair, they just pass the structure around to functions taking a `const > GError*`, which *should* supposedly be safe if we just added a pointer field. It is not safe if the passed structure is then copied with g_error_copy, because it will try to copy the new field. But I haven't checked the code of telepathy-glib or dbus-glib. (In reply to Olivier Crête from comment #23) > The same API is in GLib as g_dbus_method_invocation_return_gerror() so we > have no idea who else is doing this. I'd suggest not modifying the size of > GError, maybe instead use a qdata ? In this case, the function is only accessing the public members of the GError struct, without copying the struct, so it should work fine. By qdata I think you mean using g_dataset_* API, right? Which is kind of meh, because that would involve locking every time we copy or free the struct. (In reply to Olivier Crête from comment #21) > Except that Telepathy-GLib uses stack allocated GErrors all over, we should > triple check that nothing outside of these functions relies on this feature. > Maybe verify if the domain is an extended domain before accessing the > extended data. I'm not sure how to implement the extended domain which is a GQuark.
Since we’re now very near the API freeze for 2.56, I’m punting this to 2.58.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/14.