|
Description
Guillaume Desmottes
2011-07-06 01:43:38 UTC
Here is my version http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=socket-util-38997 It's based on Morten's branch with the following changes: - rebased on top of master - I squashed the "create new utilty function" and "use it in TpStreamTube" commits together. This makes reviewing easier and we can easily see if the new function is just a factoring out of existing code. - _tp_determine_socket_address_type() and _tp_determine_access_control_type() are now statics as they weren't used outside of util.c - _tp_set_socket_address_type_and_access_control_type() now directly pass the GError from the caller rather than using its own and use g_propagate_error(). - I re-ordered the arguments of _tp_create_local_socket so the (out) args are last. - _tp_create_local_socket(): set unix_address to NULL if we create an Inet socket. - Some coding style fixes. Created attachment 48809 [details] [review] Factor out _tp_set_socket_address_type_and_access_control_type() and _tp_create_client_socket to util-internal Created attachment 48810 [details] [review] Factor out _tp_create_local_socket() to tests/lib/util Created attachment 48811 [details] [review] Factor out _tp_destroy_socket_control_list() to tests/lib/util Shouldn't you protect all gunix* with #ifdef for _tp_create_local_socket()? I think even the +#include <gio/gunixsocketaddress.h> should be protected because those headers are not installed on windows'glib. I could be wrong though... In all those functions you assume out args are not NULL, it's fine since they are internal APIs, but maybe add a g_assert on top of the methods to make it explicit? The rest seems OK. (In reply to comment #5) > Shouldn't you protect all gunix* with #ifdef for _tp_create_local_socket()? I > think even the +#include <gio/gunixsocketaddress.h> should be protected because > those headers are not installed on windows'glib. I could be wrong though... You're right. Fixed (amended in 2nd commit and added a new one). While I was at it, I fixed the stream tube test to pass if UNIX sockets are not there. > In all those functions you assume out args are not NULL, it's fine since they > are internal APIs, but maybe add a g_assert on top of the methods to make it > explicit? Good idea; done (amended) Created attachment 49001 [details] [review] Factor out _tp_set_socket_address_type_and_access_control_type() and _tp_create_client_socket to util-internal Created attachment 49002 [details] [review] Factor out _tp_create_local_socket() to tests/lib/util Created attachment 49003 [details] [review] Factor out _tp_destroy_socket_control_list() to tests/lib/util Created attachment 49004 [details] [review] tests/lib/stream-tube-chan: don't try to use UNIX socket if not supported Created attachment 49005 [details] [review] gnio-util: properly set the GError if UNIX sockets are not implemented Created attachment 49006 [details] [review] test-stream-tube: pass if UNIX sockets are not implemented I backported the fix that should be merged into stable first: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=unix-sockets Reviewing only the stable-branch fixes: + g_assert (FALSE); g_assert_not_reached, surely? The rest of the unix-sockets branch looks fine. (In reply to comment #13) > I backported the fix that should be merged into stable first: > http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=unix-sockets Merged, so http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=socket-util-38997 is the last thing needing review. Merged to master; will be in 0.15.5. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.