Review of gnio, round 1

[Alexander Larsson <alexl redhat com>]

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.