Re: Review of gnio, round 1

Alexander Larsson wrote:
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.

For all the reasons that you list, I agree.  I'll do this.

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.

Not sure I agree. See shutdown() syscall. The fact that this call exists means that the designers of [unix or tcp or whatever] went out of their way because they disagreed with you.

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.

OK. So now I see that you propose to have a close() on the toplevel stream but also the ability to individually close only one of the substreams? Is that an accurate assessment?

Doesn't seem used, and has parts of gunixfdmessage.c in it

WIP.  Never got around to implementing this. :)

Going forward from this point, you argue that there should be a massive reduction in the number of classes. I think that your decision-making here is influenced by the fact that we're working in C. I tried to think out a logical class structure that I would do as if I had a language like Vala to do the dirty work for me -- that is, I favoured subclassing where it makes sense for conceptual clarity rather than in order to avoid an explosion of classes.

The result, of course, was an explosion of classes. I don't think that that is a bad thing, per se.

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

GSocketConnection *g_socket_connection_connect_to (GSocketConnectable
		  				   GCancellable        *cancellable,
                    				   GError             **error);
GSocketConnection *g_socket_connection_connect_to_host (const gchar
                                                        const gchar
							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.

We had a lot of discussions about this (in #vala, of all places) back in the day. We have the client code for 3 reasons:

 - the reason you cite about creating the right type of connection

 - in order to be able to specify connection parameters (like the local
   address to connect from, or socket options) without having gigantic
   connect() calls with 20 arguments.  also, if we decide to add more
   options or flags later (by user request) then it's just a matter of
   adding new functions -- not changing existing api.

 - there's no pattern for async IO without a gobject on which to perform
   the IO (ie: the source_object for the async result and callback).

If we want to solve this properly, then I propose that we come up with two new generic interfaces:

 - asynchronous object creation interface

 - potentially failing object creation interface

The async interface would essentially look like a complete() and complete_async() call that you do right after g_object_new(). The object isn't considered to be fully constructed until that returns.

The potentially-failing object creation interface would be a complete(GError *err) call that you do after _new(). If it fails, then the object must be immediately destroyed and not used.

We might even add some g_object_new() type API to GIO (g_object_async_new ?!) that did the checking and async handling.

If we developed those as proper interfaces that other people could use then I'd be comfortable with setting a new precedent for using them in GNIO, but that still solves only 1 of the above 3 problems.

GSocketConnection, GTcpConnection, GUnixConnection

GTcpConnection is empty, so just remove.

Again, this is just WIP. The intention is to land things like TCP connection options here (like CORK, etc).

                                         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.
Also, credentials.

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.

This wouldn't be terrible, although I have a preference to the way it's currently done. It's worth noting that send_fd() is actually slightly evil in the sense that it sends a single zero byte (you can't send control messages without sending at least one byte). This "sending a zero byte" seems to be a fairly well-established thing to do, but it's certainly not the most generic way that this call could work.

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.
I find your argument here fairly convincing. The only real reason to have the listener in this particular use case is to create the correct type of connection object.

In other use cases, though, the listener object itself -is- directly useful. Some people might want the accept_async() style rather than the signal callback style. Vala's 'yields' functions and davidz's fibres would work particularly well with this. That's the use case for separate listener objects.

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).

The 4-over-6 functionality (and even the setsockopt to disable it) is specified in some RFC somewhere.



ok, I'm not really that lazy :), section 5.3.

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.

I'm fine with that, but I think there is still cause for keeping the listener.

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.


I also think that we could use some sort of 'localsocket' abstraction that does unix sockets on unix and maybe some sort of named pipe stuff on windows. We should then encourage people to use this instead of unix sockets (unless they really need unix sockets for fd passing or something like that that won't work on windows anyway).

Thanks for the review.

Looking forward to your comments :)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]