With gnome 2.26 out and the GResolver branch landed it is time to start look at merging the gnio network code into gio. I'm posting this here, plus CC:ing the involved people instead of on bugzilla in order to get feedback from others who may be interested but unaware of this work.
This review is based on the gnio module in git.gnome.org, as of today, which in turn is based on the current glib from master. gnio has a two level design, the lowlevel GSocket is (in combination with the already landed stuff) a lowlevel wrapper of the system socket library, removing all system specific code etc from the higher levels. Then there is a set of higher level utility classes. I'll start with a detailed review of the GSocket code, because I think that part is closest to landing. Then I'll give my thoughts on the current highlevel stuff, which I think will need some restructuring. Maybe we should even land the GSocket code before the highlevel code. Some general comments on the code: Almost no API docs, this is a blocker. There is a lot of G_UNLIKELY() calls in places that really are not in any way performance sensitive. I'm not sure I like this, since it makes the code harder to read for very little gain. Like, it probably makes sense for the inlined version of g_once_init_enter() to use G_LIKELY, but I don't think it makes sense for e.g. error checking when creating a socket, which is not gonna happen a lot. GSocket.c: ========== The error handling in GSocket is pretty bad. There is a "sticky" error on it that you can read with a g_socket_has_error() call. This error is set on construction errors, but also when i/o operations on the socket fail. If this is set all operations just fail with this error. This is a total fail, consider for instance a server accepting incomming connections on a socket, if one accept call fails due to some transient network problem then all further connections will be ignored. This is clearly bogus, but I don't think we can totally remove it, because construction errors can still happen, for instance you might request an ipv6 socket but the OS doesn't support ipv6. So, I propose that we turn the g_socket_has_error error into a pure construction failed check. Also, we don't want to return that specific error from other calls as the exact error may be confusing comming from some method. Instead other calls should return FAILED with the string being something like "socket creation failed due to: %s". I don't really like the name g_socket_has_error() for checking for construction failure though, as it seems a bit generic. Is there a commonly used naming pattern for gobject construction failure checks? Every time I've done socket code I've enabled SO_REUSEADDR, as without this testing is a total pain in the ass. So, I think we should probably turn this on by default. There is a very minor problem with it where a old connection to the old closed socket can be confused with the new socket, but this is extremely unlikely, see: http://www.unixguide.net/network/socketfaq/4.5.shtml However, it turns out that SO_REUSEADDR means something different on windows. There if you enable this someone else can bind to the same address even if your app is still actively bound to it. See: http://cygwin.com/ml/cygwin/2004-10/msg00257.html This is a much worse security issue, and I don't want to allow that by default. So, the pragmatic approach is IMHO to enable it by default on unix and disable on win32. Win32 issues: * g_socket_details_from_fd() is totally disabled for win32, which is not right, but it needs some fixes for win32. * Need to check errors from WSAStartup * sendmsg is WSASendMsg on win32, needs special handling * winsock doesn't set errno, need to call WSAGetLastError "backlog" property needs getter and setter methods. Also, should probably be called "listen_backlog" to describe it better. We don't currently allow passing in the protocol to the socket. This means we can't for instance use SCTP (the upcomming TCP replacement) with GSocket. I think we should expose this via an optional char *protocol argument that we look up via getprotobyname(). g_socket_finalize closes socket even if g_socket_close() is already closed, need to add an is_closed variable. We should also check this in all operations so that we don't accidentally call stuff on an fd that may have been reused by now. close needs to check for errors g_socket_bind sinks the address, but socket addresses are not floating anymore. g_socket_send/recieve_message passes "flags" as is to the underlying socket lib. This isn't exactly easy to use, but I can't really think of a better approach. Any ideas? g_socket_send_message should handle num_messages == -1 by looking for a NULL termination in the array. (Similar to other glib apis) set_blocking/set_reuse() etc functions don't have any return value atm. Is that really right? What if they fail? Is it possible in practice for them to fail for other than due to programmer errors? There is some weird g_warnings about "ABI compatibility" that don't really make sense to show the users. There is IMHO some overuse of g_alloca. I mean, maybe its a useful optimization in the case of a dynamic array, but there is some use of it for constant length data instead of just using a local variable... Also, even in the other cases there is a risk of using alloca where we don't control the size as it can blow the stack. GIOStream.c: ------------ This is currently an interface, while the related GInputStream and GOutputStreams are base classes. This isn't necessarily wrong by itself, but looks a bit weird. There are also other reasons (see below) why it would be nice if it was a class, so I think we should change it. Having e.g. g_io_stream_get_output_stream() fallback on the property in case no vfunc is specified in the interface is IMHO pretty weird. A much more natural way would be to have the property implemented in the common code, based on the get_output_stream vfunc. This would be more efficient and need less code in inheriting classes. However, this is only possible if we change GIOStream to a baseclass instead of an interface. "if G_UNLIKELY (g_once_init_enter (&my_type))" g_once_init_enter is already inline and has LIKELY annotations. When GIOStream is used for a two-way stream it really represent a single object that has two directions for i/o. As such the individual streams should not be closed by themselves, for instance it makes no sense to close the input stream on a network socket or on a readwrite local file fd. So, we need to add close and close_async operations on GIOStream. We also want to add an is_closed() on the GIOStream. We also need to define what closing a substream means. This is a bit tricky, and may actually be implementation specific, as closing one direction may make sense (for instance using shutdown() on a TCP socket. I think in most cases we should just chain to the default GInput/OutputStream close methods, which will set the "closed" state and which will cause all operations to fail on that stream, then implement the real close in GIOStream, which should also close the individual streams to allow special handling there. gnioerror.h: ------------ None of these are used anymore GSocketInput/OutputStream: -------------------------- Need to check is_cancelled before doing i/o gunixcredentialsmessage.c: -------------------------- Doesn't seem used, and has parts of gunixfdmessage.c in it Highlevel API ============= Then we come to the highlevel stuff. My basic problem here is that exploding the socket types into different types for each of the highlevel objects (TcpClient, UnxiClient, WhateverClient, etc) is just wrong. First of all it hugely increases the apparent complexity of the API creating lots of classes you have to use (adding casting etc) that are more or less useless. Many are empty, and some just have a few helper functions. And secondly, it doesn't really work like the lowlevel abstraction, we just have a socket type, no unixsocket, tcpsocket, sctpsocket, etc. So, why should we have that at the higher level. I think we should just have: GSocketConnection, GSocketService and possibly GSocketListener. Lets go over each set of classes: GSocketClient, GTcpClient, GUnixClient ====================================== The base class is basically a useless temporary class you have to create such that you can call the connect() or connect_async() call on it. The subclasses are needed in order to create GSocketConnections of the right subclass, and they have a few helper function that lets you connect without having to first create a socket address. If we drop the GSocketConnection subclasses the type specific creation is not necessary. For the helper functions, I think we should drop all but the hostname one, as this is the only really common one and the others can easily do with the generic calls. So, I propose we should move the connect calls to GSocketConnection functions: GSocketConnection *g_socket_connection_connect_to (GSocketConnectable *connectable, GCancellable *cancellable, GError **error); GSocketConnection *g_socket_connection_connect_to_host (const gchar *host, const gchar *default_port, GCancellable *cancellable, GError **error); Plus g_socket_connection_connect_to_async, and g_socket_connection_connect_to_host_async variants. This should replace all the g*client.[ch] code. GSocketConnection, GTcpConnection, GUnixConnection ================================================== GTcpConnection is empty, so just remove. GUnixConnection has helper functions for sending and recieving a file descriptor. This is very specific to unix sockets, and is something that probably should be unix-only (i.e. in the gio-unix-2.0.pc file). As such we'd like to avoid having it directly in a shared header. This is sort of hard to solve in a way that is as easy to use as g_unix_connection_send_fd(). However, I don't think its worth making the rest of the API vastly larger and more complex just to get this one call. However, we should still make it pretty easy to pass fds. I think we should add g_socket_connection_send_control_message(connection, scm) to GSocketConnection. This is a general function that is useful for other socket types too. Then we push all the fd specific stuff to GUnixFdMessage. This makes passing fds a few more lines of code, but this is imho not a huge problem, as this is not a common operation. GSocketListener, GSocketService, GTcpListener, GUnixListener ============================================================ A socket listener is basically a wrapper around a GSocket that is bound to a specified address and implements the async accept functionallity. The subclasses has constructors that let you type less. Also, the tcp listener has some magic to pick the right kind of socket (ipv4 or ipv6). Then you add all the listeners to a GSocketService that lets you listen to multiple sockets for incomming connections. I'm not sure that the listener object is really needed. Wouldn't it make more sense to just add socketaddress objects to the socket service instead. The listener is one-to-one with a both the socket and the socket-address anyway, so I don't see how it buys us something. Furthermore, its actually a problem in the ipv4 vs ipv6 magic case. The current tcplistener code first tries to do an ipv6 socket and only if that fails it tries an ipv4 socket. This makes sense on linux, were an ipv6 socket also can accept ipv4 connections. However, this is not true on many other unixes, where you need two sockets to handle both ipv4 and ipv6. So in this case the listener object actually gets in the way, as we'd need to create two listener objects to handle this (or make the listener have two sockets). I think we should change g_socket_service_add_listener() to g_socket_service_add_address(address, socket_type, protocol) and then add a helper function g_socket_service_add_inet_port() that does the ipv6/ipv4 ANY magic code. Other random stuff ================== The unix socket code really should support linux-style abstract socketnames. Ideally in some way that makes it easy to fall back if this is not supported. _______________________________________________ gtk-devel-list mailing list gtk-devel-list@gnome.org http://mail.gnome.org/mailman/listinfo/gtk-devel-list