Re: Review of gnio, round 2

[Dan Winship <danw gnome org>]

Alexander Larsson wrote:
> There is no GLIB_SYSDEF enum thing because there are no such thing in
> the sockets API. You use getprotobyname, which is wrapped as
> g_socket_protocol_id_lookup_by_name().

Hm... actually, it looks like this is implemented pretty lamely in the
sockets API. There's only a single name<->id mapping (defined by
getproto* and /etc/protocols), which basically means all socket types
have to use the same mapping as network sockets do. And the socket()
docs fall just shy of saying you're allowed to use the IPPROTO_* values
from netinet/in.h, though it's very strongly implied, and Google code
search shows "socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)" to be an order
of magnitude more common than the same with getprotobyname("tcp").
(Though of course, most people just pass "0".)

I'd say

typedef enum {
  G_SOCKET_PROTOCOL_DEFAULT = 0,
  G_SOCKET_PROTOCOL_TCP     = 6,
  G_SOCKET_PROTOCOL_UDP     = 17,
  G_SOCKET_PROTOCOL_SCTP    = 132
} GSocketProtocol;

with the standard "and you can pass other things too" note, and don't
bother with g_socket_protocol_id_lookup_by_name() (people can use
getprotobyname if they really want to).

(Note that there's not even any need to GLIB_SYSDEF those, since they're
IANA-registered numbers.)

>> 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()?
> 
> I don't know if its used for anything else. I don't like _finish since
> it matches the _async/_finish scheme but works differently. Maybe
> g_socket_check_connect_result() ?

Sure. (I thought about the _async/_finish thing, but couldn't decide if
that was a bug or a feature.)

> The way I see this is that GSocket is a direct mapping of the lowest
> common denominator BSD socket API and how it behaves. As such, it maps
> the weirdness of sockets directly. This particular issue is specific  to
> TCP connections for a subset of applications (most protocols don't close
> without an ack or pass explicit sizes so this is no problem), so I don't
> think we should specifically tweak the GSocket API to handle it.
> 
> However, in order to allow a method in TcpConnection like
> g_tcp_connection_set_graceful_close() we should probably expose the
> shutdown call in GSocket.

Well, I was thinking we *might* want to merge close() and shutdown()
together into one API somehow... but yeah, I think there's no reasonable
way to do that given the different use cases. So... expose shutdown()
(and possibly SO_LINGER?), make sure the higher levels are doing the
right things with it, and try to document this better than the sockets
man pages do... Once we figure it out :)

> g_socket_get/set_keepalive() should probably be removed. SO_KEEPALIVE is

> I don't think its not useful. In server implementations for protocols
> where the connection is idle unless the client sends something this is
> quite important. The canonical example is a shell server, but the same
> goes for e.g. a MUD or an echo service. 

I guess my thinking was that the server ought to be timing out idle
clients on its own eventually, but I suppose for some protocols you
might not want to.

(Amusingly, googling SO_KEEPALIVE turns up some really ancient
discussions in which people suggest that using it is bad because of the
additional network traffic caused by sending 1 extra packet every two
hours. Presumably they all had 300 baud modems or something...)

> fd_source_dispatch passes fd_source->pollfd.revents which is basically 
> the result from the poll from the time of the poll exit.
> 
> Both of these seem to me to be more or less the "current" condition,

Ah, documentation problem then. I interpreted "current condition" as
meaning "the condition you're currently waiting for". Adding "which will
probably be 0" or something might help clarify.

-- Dan