Re: Review of gnio, round 2

[Dan Winship <danw gnome org>]

more comments, now that it's all committed... :-}

g_socket_new()'s documentation and implementation don't match wrt
"protocol". (In particular, the docs refer to the "protocol_id" argument
as "protocol", and talk about %NULL instead of 0.) Also, is there some
reason this is an int rather than another GLIB_SYSDEF_*'ed enum? Also,
it should be consistent throughout whether sockets have a "protocol" or
a "protocol_id". (The property is currently "protocol", but everything
else is "protocol_id".)

"gchar buffer[256]" as a generic sockaddr is wrong and you should feel
bad for hardcoding a magic number. Use "struct sockaddr_storage", which
is defined to be "big enough".

When g_socket_get_property() is asked for PROP_LOCAL_ADDRESS, it should
call g_socket_get_local_address() rather than assuming it is already
set, because you might be querying it to find the local address of a
non-bind()ed socket after you connect() it. (Which also means the "This
is only useful if the socket has been bound to a local address." in
g_socket_get_local_address() is wrong/misleading.) Also, g_socket_bind()
currently ends with

    socket->priv->local_address = g_object_ref (address);

but that's wrong too, because @address may be "incomplete" (eg, a
GInetSocketAddress with port 0), and so the actual local address will
contain additional information that wasn't in @address, so you should
just leave priv->local_address unset here and figure it out the hard way
(via getsockname) if the caller then asks for later it via
g_socket_get_local_address() or the property.

The g_socket_listen() docs should refer to
g_socket_set_listen_backlog(), and g_socket_set_listen_backlog() should
note that it needs to be called *before* g_socket_listen() and has no
effect if it's called afterward. Perhaps it should g_return_if_fail() in
that case. (And I agree that backlog shouldn't be an argument to
g_socket_listen(), since you almost never really have any clue what you
want to set it to...)

I dislike g_socket_check_pending_error() as a name. Is it ever used for
anything except the connect() result? (Google seems to suggest, no, not
really.) Maybe g_socket_connect_finish()?

GInputVector and GOutputVector are missing from the docs.

g_socket_close() may need to be revisited. In particular, if you close()
a socket when there is unread data on it, it may cause the other end of
the connection to receive an error and drop the connection before it
finishes reading all of the data which you sent it. (This has not been a
problem for libsoup because in HTTP it's always clear whose turn it is
to talk, and so you won't generally end up closing a socket with unread
data on it.) I am still figuring out the details here. qv
http://74.125.45.132/search?q=cache:7iHzJ3hxy80J:cs.baylor.edu/~donahoo/practical/CSockets/TCPRST.pdf
http://blog.netherlabs.nl/articles/2009/01/18/the-ultimate-so_linger-page-or-why-is-my-tcp-not-reliable

g_socket_create_source() says "cancellable if not NULL can be used to
cancel the source, which will cause the source to trigger, reporting the
current condition." First, this does not appear to be true; it looks
like it will cause the source to trigger with condition==0. Second,
shouldn't it trigger with G_IO_ERR anyway for consistency with other
GCancellable usage? Maybe not...

g_socket_get/set_keepalive() should probably be removed. SO_KEEPALIVE is
apparently not all that useful. It only sends the probes every 2 hours
(by default, controllable by sysctl), and these days, it's pretty
uncommon to have a server that will keep a completely idle connection
open for even that long. And people can use setsockopt() if they really
need it...

(OTOH, TCP_NODELAY, SO_RCVTIMEO, and SO_SNDTIMEO may be more worthwhile,
though TCP_NODELAY might want to be merged into a single API with
TCP_CORK/MSG_MORE. More to think about...)

-- Dan