Re: Review of gnio, round 1

[Alexander Larsson <alexl redhat com>]

On Mon, 2009-04-27 at 13:22 -0400, Dan Winship wrote:

> There are also other flags we might want to think about. In libsoup, all
> sockets get marked FD_CLOEXEC because of a long-ago bug. (The Red Carpet
> daemon used librpm to do stuff, and librpm would spawn postinstall
> scripts without doing the necessary cleanup like closing fds, and so
> sometimes you'd try to restart rcd, and it would fail to start up again,
> because some process like cupsd which had previously been restarted by
> rcd now was holding a leaked copy of the old rcd process's listening
> socket, and so the new rcd process couldn't re-bind that address.)
> Anyway, ideally people would use spawning APIs that cleaned up fds
> properly, but the fact is, unless you're writing inetd, you generally
> don't intend to hand off your sockets to child processes anyway, so
> setting FD_CLOEXEC unconditionally (or using SOCK_CLOEXEC where
> available) generally makes sense.

There is also SO_KEEPALIVE which seems to see some use. And possibly
SO_LINGER, although that seems fraught with problems.

> > Win32 issues:
> 
> I'd filed a bug about some other win32 issues over the weekend.
> (http://bugzilla.gnome.org/show_bug.cgi?id=580236)

More todo... Need to get some win32 people involved to test this stuff.

> > * Need to check errors from WSAStartup
> 
> Would it be better to have only one place in gio that calls WSAStartup?
> (Currently that's g_inet_address_get_type().)

Yes, i forgot that. I think thats a good place. I'll fix that.

> > 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 imagine the theory is that G_SOCKET_TYPE_STREAM,
> G_SOCKET_TYPE_DATAGRAM, and G_SOCKET_TYPE_SEQPACKET invariably also
> imply tcp, udp, and sctp, respectively.

Well, you can use sctp in the one-to-one mode with SOCK_STREAM too. I'll
add the protocol argument so the user can explicitly pick one.

> > 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.
> 
> libsoup's solution here is to implement "close" as
> shutdown(fd,SHUT_RDWR), and only actually call close(fd) at finalization
> time.

Whats the advantage here over close() and then setting some is_closed
bit?

> > GSocketConnection *g_socket_connection_connect_to_host (const gchar
> > *host,
> >                                                         const gchar
> > *default_port,
> > 							GCancellable        *cancellable,
> >   							GError             **error);
> 
> IMHO, ports should always be numbers, not strings. getservbyname() is
> useless, because you want your app to work on last year's distro that
> doesn't know about this year's protocols. So apps should just provide a
> #define for their port number and use that, rather than hoping that the
> service's name will be in /etc/services.

I agree.

> > 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.
> 
> My vision of how this works is something like:
> 
>     - GUnixSocket{Connection,Service} - does unix sockets. Has APIs for
>       using abstract names, but they return G_IO_ERROR_NOT_SUPPORTED on
>       non-Linux.
> 
>     - GWin32NamedPipe{Connection,Service} - a Windows named pipe.
>       Implements GIOStream, but is not related to any of the socket
>       types.
> 
>     - GNamedBidirectionalIPC{Connection,Service} - Takes an
>       application name and a channel name, and a flag for "session" or
>       "system" level, and then "it does something", such that if one
>       app makes the server and another app makes a client with the
>       same parameters, then they can talk to each other. On Windows,
>       this would use a named pipe, on Linux it would use abstract
>       sockets, on other unixes, it would use a unix socket in /tmp.
>       Or maybe it passes fds over D-Bus. And maybe on OS X it uses
>       Mach ports.

I like the basic idea, but.... soo ... many ... classes... :)