GNOME Bugzilla – Bug 728776
Document GResolver return value guarantees
Last modified: 2014-05-28 13:46:19 UTC
The documentation for g_resolver_lookup_by_name() isn’t clear about whether it guarantees to return a *non-empty* list of GInetAddresses, or whether it’s possible to return NULL and no error. There is a code path which returns NULL with no error if all of the addresses returned by getaddrinfo() in do_lookup_by_name() in gthreadedresolver.c can’t be built into GSocketAddresses — i.e. g_socket_address_new_from_native() fails on them. So I guess it would be nice to either: • Explicitly document that g_resolver_lookup_by_name() can return NULL and no error; or • change it to set an error (G_RESOLVER_ERROR_NOT_FOUND) before returning if the address list is empty. I guess the same changes should be made to the other GResolver methods too.
Created attachment 275027 [details] [review] gresolver: Document that GResolver lists are non-empty on success The documentation previously wasn’t clear about whether the GResolver methods could return an empty list and no error. On balance, this seems like a bad idea, and GResolver should commit to always return a non-empty list, or an error (which should be G_RESOLVER_ERROR_NOT_FOUND if the list would otherwise be empty).
Created attachment 275028 [details] [review] gresolver: Ensure GThreadedResolver always sets an error if resolution fails It was previously possible for GThreadedResolver to return an empty list and no error in response to a g_resolver_lookup_by_name() call, if it happened that all the addresses returned by getaddrinfo() could not be converted from native addresses to GSocketAddresses. Fix that by setting a G_RESOLVER_ERROR_NOT_FOUND if the returned list is empty.
Created attachment 275029 [details] [review] gresolver: Add postconditions to ensure lists are non-empty Add postconditions to the GResolver interface method wrappers to ensure that the returned values conform to the API documentation (i.e. that the returned list is non-empty, or an error is set). This should prevent erroneous implementations of GResolver from causing problems for users of the API.
Comment on attachment 275028 [details] [review] gresolver: Ensure GThreadedResolver always sets an error if resolution fails OK, but note that if you want to commit this to glib-2-40 too, you need to get rid of the new translated string. You could just reuse the string "No DNS record of the requested type for '%s'" from later on in the file instead. (That's just for glib-2-40; the new string is fine on master.)
Comment on attachment 275029 [details] [review] gresolver: Add postconditions to ensure lists are non-empty seems messy, and we don't do this anywhere else
Pushed to master and glib-2-40 (with the changed string). Attachment 275027 [details] pushed as 14b0c15 - gresolver: Document that GResolver lists are non-empty on success Attachment 275028 [details] pushed as 956921e - gresolver: Ensure GThreadedResolver always sets an error if resolution fails
(In reply to comment #5) > (From update of attachment 275029 [details] [review]) > seems messy, and we don't do this anywhere else I think it’s a useful way of catching bugs in the implementation of interfaces or virtual methods. If a GResolver implementation is provided by a third party, it’s still called through the g_resolver_*() functions, so it’s partially a GLib problem if results come back which don’t match the function documentation. We could stick all those assertions in the GResolver vfunc implementations themselves, but that seems a bit wasteful. Is there any other approach at the moment for verifying that an interface or vfunc implementation conforms to the interface declaration/documentation?
Dan, ping?
I think it uglifies the code too much for too little benefit. Just make Tartan verify it. :-) (I left the bug open before in case any other GLib maintainer chimed in saying they wanted this, but no one has, so...)
(In reply to comment #9) > I think it uglifies the code too much for too little benefit. Just make Tartan > verify it. :-) Fair enough. Getting Tartan to verify this would be tricky, but given a few assumptions it's probably feasible. I'll add it to the to-do list. :-)