Created attachment 241380 [details] [review]
iostream: Add API to know if a stream is datagrams

Created attachment 241381 [details] [review]
udpconnection: Add a GIOStream for UDP connections

Also add matching GUdpWrapperConnection

Created attachment 241382 [details] [review]
gtlsconnection: Add functions to support DTLS-SRTP negotiation

Created attachment 241383 [details] [review]
Add G_IO_ERROR_MESSAGE_TOO_LARGE

Created attachment 241384 [details] [review]
tlsconnection: Add status property

Comment on attachment 241382 [details] [review]
gtlsconnection: Add functions to support DTLS-SRTP negotiation

attached to wrong bug

Comment on attachment 241384 [details] [review]
tlsconnection: Add status property

attached to wrong bug

(In reply to comment #6)
> (From update of attachment 241382 [details] [review])
> attached to wrong bug

Oh, I thought the patches attached to 697908 were the same as these, but I see that you attached all the glib patches here and all the glib-networking ones there. But there shouldn't be TLS-specific stuff mixed in with the datagram-support bug. (And it's fine to put glib and glib-networking patches in the same bug. So you should re-attach them there. Although maybe you should file a separate bug for SRTP stuff since that seems separate from the base DTLS stuff.)

Comment on attachment 241381 [details] [review]
udpconnection: Add a GIOStream for UDP connections

>Also add matching GUdpWrapperConnection

You don't need that; GTcpWrapperConnection is just a workaround for a bug in GSocketClient's API. But GSocketClient only supports TCP sockets, so there'd never be a need for a UDP wrapper.

Comment on attachment 241380 [details] [review]
iostream: Add API to know if a stream is datagrams

>+/**
>+ * g_io_stream_is_datagram:
>+ * @stream: a #GIOStream.
>+ *
>+ * Returns if the #GIOStream is composed of datagrams, if it is, each
>+ * read from the #GInputStream will return at most one datagram and each
>+ * write the #GOutputStream will produce one datagram.
          ^ to

>+ *
>+ * Return: %TRUE if the stream is a stream of datagrams

Since: 2.38

(and the G_AVAILABLE_IN in the .h file is wrong; 2_36 instead of 2_38)

>+ */

So... this doesn't work very well with filter streams in the general case. But after thinking about it, I guess that doesn't really matter? If you want to work with datagrams, then you need to only work with filter streams that are explicitly aware of datagrams (like GTlsConnection).

Well, I was assuming that if a filter isn't datagram aware, it will transform the underlying datagram stream into a non-datagram stream.. which will be reflected by having is_datagram() return FALSE.

Created attachment 241397 [details] [review]
iostream: Add API to know if a stream is datagrams

Updated the version number

Created attachment 241398 [details] [review]
udpconnection: Add a GIOStream for UDP connections

Remove the GUdpWrapperConnection

(In reply to comment #11)
> Well, I was assuming that if a filter isn't datagram aware, it will transform
> the underlying datagram stream into a non-datagram stream.. which will be
> reflected by having is_datagram() return FALSE.

It's more complicated than that though. With a datagram input stream you can actually lose data if you wrap a filter around it (if the filter stream makes reads of the base stream that are smaller than the incoming datagrams).

I guess that's true whether or not we have an is_datagram() method though...

I guess something like a GBufferedInput/OutputStream would help in that case, but I admit I haven't fully invetigated.

Another option is to return G_IO_ERROR_MESSAGE_TOO_LARGE if one tries to read less than one buffer.

I have some reservations about a stream being used as a datagram... I guess since we have seekable streams they're already not strictly streams...

To put a bit more weight behind my statements: I don't think we should have connectionless sockets using any name like 'SocketConnection' or being returned by an API with _connect() in the name and I don't think we should have datagrams ever being represented by any class named *Stream.

BSD sockets makes this mistake but it's actually very strange... you end up with an fd that doesn't work like a normal fd.  Would have been much better if you were forced to use sendmsg() and recvmsg() with UDP connections.

That said, why not just use GSocket?  It even has a poorly-named (in the BSD sense I lament here) _connect() call on it...  You can get a source in order to be able to poll on it and everything...

I guess I'm generally against what you propose here unless you have very convincing answers to the above.

A more interesting question would be if we were asked to support SCTP... it's more of a proper connection, but not a stream.  I guess we cross that bridge when we get there...

(In reply to comment #18)
> I don't think we should have
> connectionless sockets using any name like 'SocketConnection'

Sure, and it seems unlikely that anyone would want to use GIOStream with a connectionless socket. But not all UDP sockets are connectionless.

> I don't think we should have
> datagrams ever being represented by any class named *Stream.

There are plenty of UDP-based protocols that are talked about in terms of streaming. "RTP stream" gets 2 million google hits.

(Though I don't know for sure that *our* stream APIs make sense for UDP streams.)

> That said, why not just use GSocket?

The underlying motivation for this bug is that using GIOStreams for UDP makes DTLS (Datagram TLS) support nearly free (bug 697908).

Would it be possible to implement DTLS on top of GSocket instead?  I think I'd really prefer that....

Afaik, GSocket isn't really abstract/subclassable in it's current implementation. And I need to be able to put DTLS on top of something more complex than a simple socket (explicitly libnice), I have a pair of GStreamer elements that act as decoder/encoder around a GIOStream. Although I guess if GSocket was made abstract, we could make libnice implement a GSocket subclass. But we also need a way to bypass DTLS after the connection is established to use SRTP to encrypt the actual data packets.

Comment on attachment 241397 [details] [review]
iostream: Add API to know if a stream is datagrams

OK, let's get this in...

I've been thinking about whether it would be better to have a GIODatagramStream interface rather than a GIOStream method... it feels like in some ways it might be, but in the end, it makes things a bunch more complicated, since you'd end up having to create two new subclasses in glib-networking (GTlsDatagramClientConnectionGnutls and GTlsDatagramServerConnectionGnutls).

The other thing I don't like about the current patch is that only the iostream is marked; the input and output streams can't be identified as being datagram-based when they're apart from their iostream. So I think we want API on GInputStream and GOutputStream too.

But GInputStream and GOutputStream have fewer spare vmethod slots than GIOStream, so I'm a littl reluctant to use one up.

It seems like this could just as easily be a read-only property; add a "datagram-based" property to GIOStream, GInputStream, and GOutputStream, which is always FALSE by default, and an _is_datagram_based() method on each class that just reads the property. And then subclasses that are datagram-based (or sometimes-datagram-based) can override the property to return their own value.

(I think I like "g_io_stream_is_datagram_based()" rather than "g_io_stream_is_datagram()", since it's not the stream itself that's a datagram.)

Comment on attachment 241383 [details] [review]
Add G_IO_ERROR_MESSAGE_TOO_LARGE

this is fine, but should there be an addition to g_io_error_from_errno() to go with it?

Comment on attachment 241398 [details] [review]
udpconnection: Add a GIOStream for UDP connections

>+ * Copyright © 2008, 2009 Codethink Limited

>+ * Authors: Ryan Lortie <desrt@desrt.ca>

given Ryan's opinion of this bug, you should probably fix that :)

Although, if there's no API on GUdpConnection, we don't really need it... you could just register the datagram types as using GSocketConnection directly, right?

(In reply to comment #24)
> (From update of attachment 241398 [details] [review])
> >+ * Copyright © 2008, 2009 Codethink Limited
> 
> >+ * Authors: Ryan Lortie <desrt@desrt.ca>
> 
> given Ryan's opinion of this bug, you should probably fix that :)

Well, that file is s/tcp/udp/ from Ryan's code, so I can't really claim to have authored it.

I still don't like this.  At the very least, I'd like to make sure that Stef thinks that this is absolutely necessary in order to avoid substantial problems.

Created attachment 245687 [details] [review]
Add G_IO_ERROR_MESSAGE_TOO_LARGE

With added g_io_error_from_errno()

Created attachment 245688 [details] [review]
iostream: Add read-only property indicating if a stream is datagram based

Also add APIs to GInputStream and GOutputStream to indicate the same

This is now done using properties as requested

(In reply to comment #26)
> I still don't like this.  At the very least, I'd like to make sure that Stef
> thinks that this is absolutely necessary in order to avoid substantial
> problems.

OK, cc:ing him, but note that I'm not just claiming that this is "necessary", I'm claiming it's actually correct and desirable.

I agree this isn't architecturally beautiful. Even Java doesn't do datagram streams, but rather datagram sockets. 

It feels rather hacky to have datagrams as streams. The whole point of streams is their composability and abstract nature. Much of our/others stream stuff (things like GDataXxxStream, GBufferedXxxStream, GFilterXxxStream). won't work with datagram based streams, or will have to be special cased for it.

I'm surprised that a GStreamer based GInputStream/GOutputStream can just work with datagrams without significant special casing, but that may be because it's already sending composed messages/frames through the stream.

On the other hand, Dan, you're the one who's intimate with how this will be implemented...  Are you sure that DTLS won't be different enough to require it's own glib-networking implementation classes?

Ryan, as far as BSD-socket-isms leaking ... well that's obviously already the case, so not a strong argument for or against this.

(In reply to comment #30)
> It feels rather hacky to have datagrams as streams. The whole point of streams
> is their composability and abstract nature.

Which is exactly what Olivier needs here; he wants to use libnice to create a tunnel through a NAT, speak DTLS over the tunnelled connection, and hand the result to gstreamer.

We could create a non-stream-based abstraction to allow for this, but it would look *exactly* like GInputStream/GOutputStream other than the name.

> Much of our/others stream stuff
> (things like GDataXxxStream, GBufferedXxxStream, GFilterXxxStream). won't work
> with datagram based streams, or will have to be special cased for it.

Yes. So this is the big problem (which will apply to SCTP too).

Maybe we should have GFilterXxxStream refuse to wrap a datagram stream, unless the filter stream class sets some property declaring its ability to deal with datagrams. (We don't want to just say that the filter itself has to also be a datagram stream, because you want to allow the filter to abstract away the datagram-ness, as the gstreamer streams apparently do.)

(And "refuse to wrap" would have to actually mean "wrap but then return an error on any read or write", since GFilterXxxStream is not GInitable.)

> On the other hand, Dan, you're the one who's intimate with how this will be
> implemented...  Are you sure that DTLS won't be different enough to require
> it's own glib-networking implementation classes?

Bug 697908. Specifically, attachment 245691 [details] [review]. It's shockingly trivial, assuming we reuse GTlsConnection.

(In reply to comment #31)
> We could create a non-stream-based abstraction to allow for this, but it would
> look *exactly* like GInputStream/GOutputStream other than the name.

Would it also have g_input_stream_read_all()?


It seems that we're dealing with an attempt to take a set of APIs not designed to deal with datagrams and generalise them to a case where they support datagrams.

For TLS, for example, it seems clear to me that we want to support 3 types of API calls:

 - TLS support stuff (negotiation, etc.  Most of the GTlsConnection API)
 - (optionally) a GIOStream implementation for stream-based TLS
 - (optionally) a new datagram thing for datagram-based TLS

I don't know if "optionally" would be a has-a or is-a depending on what type of TLS you were doing....

btw: I would expect the datagram calls to be named send() and receive(), not read() and write().

> Yes. So this is the big problem (which will apply to SCTP too).
> 
> Maybe we should have GFilterXxxStream refuse to wrap a datagram stream, unless
> the filter stream class sets some property declaring its ability to deal with
> datagrams. (We don't want to just say that the filter itself has to also be a
> datagram stream, because you want to allow the filter to abstract away the
> datagram-ness, as the gstreamer streams apparently do.)
> 
> (And "refuse to wrap" would have to actually mean "wrap but then return an
> error on any read or write", since GFilterXxxStream is not GInitable.)

This ugliness is a big problem for me.

> > On the other hand, Dan, you're the one who's intimate with how this will be
> > implemented...  Are you sure that DTLS won't be different enough to require
> > it's own glib-networking implementation classes?
> 
> Bug 697908. Specifically, attachment 245691 [details] [review]. It's shockingly trivial, assuming
> we reuse GTlsConnection.

I've agreed all along that doing this would make things really easy for GTlsConnection, but I maintain that this is only because of the current form of GTlsConnection.  If datagrams had been considered from the start, probably we would have something else.

Back-compatibility is obviously a legitimate concern, though.  We have to figure out a way to get the best API given what we already have.

I just want to make sure that this datagram/stream confusion is useful on a general basis, and not just for a couple of cases like GTlsConnection being easy to implement internally or one particular streaming format working transparently with GStreamer.

(In reply to comment #32)
> > We could create a non-stream-based abstraction to allow for this, but it would
> > look *exactly* like GInputStream/GOutputStream other than the name.
> 
> Would it also have g_input_stream_read_all()?

Ha. g_input_stream_read_all() is generally only useful for streams wrapping ordinary files (and not for streams wrapping device files, pipes, bidirectional network connections, etc). So you're just highlighting the fact that the streaming APIs already have special cases for certain stream types. :)

> It seems that we're dealing with an attempt to take a set of APIs not designed
> to deal with datagrams and generalise them to a case where they support
> datagrams.

Yes. The situation is basically equivalent to where we would be if we had built seek() and tell() directly into GInputStream and GOutputStream, and then later on realized that we needed to also support streams that couldn't implement seeking.

> btw: I would expect the datagram calls to be named send() and receive(), not
> read() and write().

The only thing the word "datagram" means in this API is that the stream is inherently quantized. It does not necessarily mean that the stream wraps a network connection.

(In reply to comment #33)
> Ha. g_input_stream_read_all() is generally only useful for streams wrapping
> ordinary files

Not true.  It takes a 'count' parameter and implements the familiar

while (count)
  {
    s = read();
    count -= s;
  }

loop.... a pattern which is utter nonsense on a datagram-based socket and really goes to the heart of "things you can assume make sense to do on streams".

oh, indeed. was thinking it meant "read until end of stream".

Alright, so if GIOStream is out, we should probably have a different abstraction, something like a GDatagramSocket interface that could be implemented among by GSocket (as well as by libnice and the DTLS stuff), I would include the following functions

g_socket_receive_from
g_socket_receive_message
g_socket_send_to
g_socket_send_message
g_socket_condition*
g_socket_create_source
and maybe g_socket_bind()
and something (currently missing from GSocket) to deal with MTU and Path MTU discovery

Then we can create a GDTlsConnection which would be exactly like a GTlsConnection, expect it would implement GDatagramSocket and take a GDatagramSocket as it's base object. I'd probably remove auto-connecting from there (and force manual manual calls to handshake), making the thing a lot more simple. And GDTlsClientConnection and GDTlsServerConnection to match.

So we have a distinction between (let's call it) "associated" and "unassociated" datagram sending.  The _to() and _from() calls are really for the unassociated case...

Am I correct in assuming that DTLS would always be associated?  If this is the case then we could probably limit it to a simple send() and receive() API.

Hmm, DTLS streams would always be associated yes.. but you could have multiple DTLs streams over the same local socket to different peers, but I guess we need some association API. Also, libnice also creates an association (a datagram-oriented connection).. So maybe we need some kind of "connection" layer between GSocket and GDTlsConnection? That would split packets from different sources..

(In reply to comment #36)
> g_socket_receive_from
> g_socket_receive_message
> g_socket_send_to
> g_socket_send_message

For the purposes of higher level abstractions, the fact that a GSocket has an innate blocking/non-blocking-ness turned out to be annoying, and resulted in us needing to add g_socket_receive_with_blocking() / g_socket_send_with_blocking() (which take an explicit "gboolean blocking") to get GSocketConnection to work right.

I'm not sure this will necessarily apply to the more datagram/DTLS-y use cases, but I don't think it will hurt either, so we might want to make the "GDatagramSocket" APIs all have the "gboolean blocking" arg, unlike the existing GSocket APIs listed above.

(Also, the interface itself only needs to implement receive_message() and send_message(); receive_from() and send_to() can just be simple wrapper functions like they are now in GSocket.)

> and maybe g_socket_bind()

And connect()? It would seem weird to me to include one but not the other...

Also, maybe g_socket_get_available_bytes()?

> Then we can create a GDTlsConnection which would be exactly like a
> GTlsConnection, expect it would implement GDatagramSocket and take a
> GDatagramSocket as it's base object. I'd probably remove auto-connecting from
> there (and force manual manual calls to handshake), making the thing a lot more
> simple. And GDTlsClientConnection and GDTlsServerConnection to match.

Of course, GDatagramSocket ends up looking a lot like the combination of GPollableInputStream and GPollableOutputStream, so it ought to be possible to reorganize the code in such a way that most of it stays shared between the TLS and DTLS objects.

(In reply to comment #39)
> > and maybe g_socket_bind()
> 
> And connect()? It would seem weird to me to include one but not the other...

Bind is needed to select the port to receive packets on.. But connect only selects the default destination, any reason to use that instead of just having _send_message() or _send_to(), there may be a performance advantage to connect()...
 
> Also, maybe g_socket_get_available_bytes()?

Yes, although I'm not sure the current abstraction is useful on Windows & OSX for datagrams (as it may returns the total number of bytes in the queue instead of just the next packet's size).

> Of course, GDatagramSocket ends up looking a lot like the combination of
> GPollableInputStream and GPollableOutputStream, so it ought to be possible to
> reorganize the code in such a way that most of it stays shared between the TLS
> and DTLS objects.

Yes, absolutely.

(In reply to comment #40)
> > Also, maybe g_socket_get_available_bytes()?
> 
> Yes, although I'm not sure the current abstraction is useful on Windows & OSX
> for datagrams (as it may returns the total number of bytes in the queue instead
> of just the next packet's size).

Yes, that's bug 686786. It can be fixed on both platforms, it's just that no one has.

What's the status/plan for this now? Would be nice to somehow get DTLS support into GSocket but from reading this comments it seems like no plan to go forward was decided as they all seem rather ugly?

imho this is a WONTFIX for a lot of reasons, but I think Dan and Stef have the last say here.

but in summary, I think it would be a mistake to turn this entire (very widely used) API into something that it's not on account of making one use of it more convenient -- even if it really does make that one use extremely convenient.

oh, and either way: not before 2.40.

(In reply to comment #43)
> imho this is a WONTFIX for a lot of reasons, but I think Dan and Stef have the
> last say here.
> 
> but in summary, I think it would be a mistake to turn this entire (very widely
> used) API into something that it's not on account of making one use of it more
> convenient -- even if it really does make that one use extremely convenient.

You mean the approach of Olivier's initial patches that make GIOStreams of datagram sockets possible? Yes, I agree with that. Just the combination of "stream" and "datagram" in the same sentence is already hurting me head ;)

However in the later comments other approaches were discussed to make an implementation of DTLS possible in GIO. And I think having DTLS support in GIO is something that would be really good to have.

I agree.  That's bug 697909, though.

I thought bug #697909 is only for the SRTP specific additions. I was thinking of RFC 6347 here, of which RFC 5764 (the SRTP bits from that other bug) is an extension.

My new plan is to instead add a GSocket interface that would be implemented by GSocket, but that we could also implement over DTLS, or over libnice or over a socks proxy.

Sounds like a good plan, yes. Are you working on that already?

It's currently only at the plan state, I haven't had any time to work on it yet.

Created attachment 301129 [details] [review]
Add a socket interface and implement it in GSocket

I implemented a basic interface to have some starting point.
Some functions might be missing and i am also a bit unsure about the naming.

The source is attached as patch file and also available on
github: https://github.com/sdamm/glib .

Created attachment 302965 [details] [review]
Here’s a WIP patch I’ve been playing with. The interface is called GCommunicable for the moment, since GSocket is taken, GSocketInterface leads to craziness like GSocketInterfaceIface, and I can’t think of anything else reasonable. I considered:

• GSocketlike — too unconventional
 • GEndpoint — not obviously socket-like enough
 • GSock — too likely to be confused with GSocket

This also implements a lot more of the GSocket methods as vfuncs. I haven’t yet worked out the best balance of what needs vfunc-ing and what doesn’t. There are still some TODOs.

Basically, this is ready for a bit of preliminary feedback by anyone who is interested, but is not worth reviewing in any kind of detail yet.

---

gio: Add GCommunicable interface and rebase GSocket on it

GCommunicable is an interface abstracting the Berkeley sockets API, so
that socket-like communications can be done through any object, not just
a concrete GSocket (which wraps socket()).

This adds the GCommunicable interface, and implements it in GSocket.

(In reply to Philip Withnall from comment #52)
> Created attachment 302965 [details] [review] [review]
> Here’s a WIP patch I’ve been playing with. The interface is called
> GCommunicable for the moment, since GSocket is taken, GSocketInterface leads
> to craziness like GSocketInterfaceIface, and I can’t think of anything else
> reasonable. I considered:
> 
> • GSocketlike — too unconventional
>  • GEndpoint — not obviously socket-like enough
>  • GSock — too likely to be confused with GSocket
> 
> This also implements a lot more of the GSocket methods as vfuncs. I haven’t
> yet worked out the best balance of what needs vfunc-ing and what doesn’t.
> There are still some TODOs.

One thing I’m considering is to modify the [send|receive]_message() vfuncs to be like sendmmsg() and recvmmsg(), rather than sendmsg() and recvmsg(). That would make them completely future-proof. But that requires defining some kind of GNetworkMessage struct akin to mmsghdr and msghdr. Probably worthwhile. I still need to think about it some more.

> One thing I’m considering is to modify the [send|receive]_message() vfuncs
> to be like sendmmsg() and recvmmsg(), rather than sendmsg() and recvmsg().
> That would make them completely future-proof. But that requires defining
> some kind of GNetworkMessage struct akin to mmsghdr and msghdr.

There's GOutputMessage for sendmmsg()

(In reply to Tim-Philipp Müller from comment #54)
> > One thing I’m considering is to modify the [send|receive]_message() vfuncs
> > to be like sendmmsg() and recvmmsg(), rather than sendmsg() and recvmsg().
> > That would make them completely future-proof. But that requires defining
> > some kind of GNetworkMessage struct akin to mmsghdr and msghdr.
> 
> There's GOutputMessage for sendmmsg()

Ah, I’d totally missed that. That makes things a lot easier, thanks.

Created attachment 306607 [details] [review]
gsocket: Split out functions to convert to and from struct msghdr

As new methods are added to GSocket, we don’t want to duplicate this
code, so factor it out.

Created attachment 306608 [details] [review]
gsocket: Factor out blocking parameter from g_socket_send_messages()

This will make future API additions easier. The factored version is
internal for the time being.

Created attachment 306609 [details] [review]
gsocket: Add g_socket_receive_messages()

Add support for receiving multiple messages with a single system call,
using recvmmsg() if available. Otherwise, fall back to looping over
g_socket_receive_message().

This adds new API:
 • GInputMessage
 • g_socket_receive_messages()
and corresponding unit tests.

Created attachment 306610 [details] [review]
gio: Add GCommunicable interface and rebase GSocket on it

GCommunicable is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GCommunicable allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GCommunicable interface, and implements it in GSocket.

Here’s my latest WIP, still in need of some high-level review (but nothing detailed yet).

I’m torn at the moment between two approaches:
 1. An interface which abstracts BSD socket semantics — i.e. it provides sendmsg() and recvmsg(), but they can be used for datagram-like *or* stream-like communications.
 2. An interface which just abstracts datagram-like communications.

If I go with option #1, GCommunicable can be implemented by GIOStream, and integrating it with existing code becomes a whole lot easier. It can also get vfuncs for things like bind(), which might be useful. Or they might be unnecessarily specific. In my experience so far, none of the implementations of GCommunicable I’ve written have needed bind(), connect(), listen() or anything like that. And if they have, the connection method has not matched the standard connect() API anyway.

If I go with option #2, we get the nice semantic split between stream-like and datagram-like APIs, which might be more future-proof. But we end up having to put “if (datagram-like) g_communicable_receive_messages(…) else g_io_stream_read(…)” everywhere which currently uses BSD semantics. One big example is the TLS code, where switching between datagram and stream semantics is as simple as passing a flag to GnuTLS (see bug #697908).

The choice between these two options also affects the interface name. For option #1, the best I’ve been able to come up with is GCommunicable. For option #2, we could go with something more obvious, like GDatagramSocket as suggested previously.

Created attachment 306611 [details] [review]
gsocket: add recvmmsg-based g_socket_receive_messages() (alternative version)

Here's an alternative implementation which IMO is better in some respects, e.g. it maintains the timeout argument that recvmmsg has (only that it works properly here).

Let’s split the recvmmsg()/g_socket_receive_messages() discussion out into bug #751924 to avoid getting sidetracked.

This bug can remain for discussion of GCommunicable.

Created attachment 307242 [details] [review]
gio: Add GCommunicable interface and rebase GSocket on it

GCommunicable is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GCommunicable allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GCommunicable interface, and implements it in GSocket.

---

Updated to add close_async() and close_finish(), and add a GCancellable to close().

I’ve split out the discussion for DTLS itself in GIO to bug #752240. Implementing DTLS in glib-networking’s GnuTLS backend is still bug #697908. This bug is about adding an interface for socket-like things to GIO (which I am still calling GCommunicable for the time being).

Comment on attachment 307242 [details] [review]
gio: Add GCommunicable interface and rebase GSocket on it

>GCommunicable is...

a terrible terrible name :). The word "communicable" is basically never followed by any word except "diseases". Google agrees with me.

I'm fine with GSocketLike.

>+ * Copyright © 2015 Collabora Ltd.

Just "Copyright". No symbol required.

>+ * A #GCommunicable is a networking interface for representing datagram-based
>+ * communications. It is a more or less direct mapping of the BSD socket API in

"of the core parts of the BSD socket API"

>+ * As with #GSocket, #GCommunicables can be either connection oriented or
>+ * datagram based.

"connection-oriented" and "datagram-based" are not contrasting types and you said above that it's only for datagram-based communications. "can be either connection-oriented or connectionless".

>+   * GCommunicable:type:
>+   *
>+   * The type of the communicable. This defines the semantics of sending and
>+   * receiving â whether communications are datagram- or stream-based.

OK, need to make up our minds on this...

>+ * even if the close returns with no error. Similarly, it does not do a closing
>+ * handshake, so stateful protocols (such as TCP) must implement this
>+ * themselves.

That makes it sound like it doesn't even do a FIN...

>+gboolean
>+g_communicable_close (GCommunicable  *communicable,
>+                      GCancellable   *cancellable,
>+                      GError        **error)
>+{
>+  g_return_val_if_fail (G_IS_COMMUNICABLE (communicable), FALSE);
>+  g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
>+
>+  return g_communicable_shutdown (communicable, TRUE, TRUE, cancellable, error);
>+}

That's not the same as close() for a socket. It still leaves the fd open. (And, eg, g_socket_is_closed() will still return FALSE after this.)

>+ * g_communicable_close_async:

At this API level, it would be more idiomatic to add a @blocking parameter to g_communicable_close(). (Likewise for shutdown_async.)

>+ * Note that on Windows, this function is rather inefficient in the UDP case,
>+ * and so if you know any plausible upper bound on the size of the incoming
>+ * packet, it is better to just do a g_socket_receive() with a buffer of that

"just do a g_communicable_receive_messages()". But maybe this warning doesn't belong in the GCommunicable docs anyway...

>+typedef struct _GCommunicableIface GCommunicableIface;

New types should use "Interface", not "Iface"

(In reply to Dan Winship from comment #65)
> Comment on attachment 307242 [details] [review] [review]
> gio: Add GCommunicable interface and rebase GSocket on it
> 
> >GCommunicable is...
> 
> a terrible terrible name :). The word "communicable" is basically never
> followed by any word except "diseases". Google agrees with me.

Hah. Could be paired with GSickit?

> I'm fine with GSocketLike.

But then we end up with methods like g_socket_like_is_closed() which is a bit weird.

Could go with GSocketlike (lower case ‘l’) which would give g_socketlike_is_closed(), which I think is marginally better. Still feels ugly though.

Emmanuele suggested GDatagramChannel which I think is the best suggestion yet.

> >+ * Copyright © 2015 Collabora Ltd.
> 
> Just "Copyright". No symbol required.

OK, though I cargo-culted the header from gsocket.c.

> >+ * A #GCommunicable is a networking interface for representing datagram-based
> >+ * communications. It is a more or less direct mapping of the BSD socket API in
> 
> "of the core parts of the BSD socket API"

Fixed locally.

> >+ * As with #GSocket, #GCommunicables can be either connection oriented or
> >+ * datagram based.
> 
> "connection-oriented" and "datagram-based" are not contrasting types and you
> said above that it's only for datagram-based communications. "can be either
> connection-oriented or connectionless".

Whoops, thinko. Fixed locally.

> >+   * GCommunicable:type:
> >+   *
> >+   * The type of the communicable. This defines the semantics of sending and
> >+   * receiving â whether communications are datagram- or stream-based.
> 
> OK, need to make up our minds on this...

Make up our mind on whether GCommunicable uses only datagram semantics, or both datagram and stream semantics? Yeah, this is a bit of a pain. I originally wrote the interface as a direct abstraction of GSocket (with both semantics), but with the recvmmsg stuff it’s morphed towards being datagram-only. Having done the DTLS stuff now, I think it makes sense to make this interface explicitly datagram-only, and have GIOStream/GInputStream/GOutputStream as stream-only counterparts — or leave the door open for introducing a counterpart streaming interface in future (GStreamChannel?). That would implement send() and recv() rather than sendmmsg() and recvmmsg().

I’ll remove :type locally.

> >+ * even if the close returns with no error. Similarly, it does not do a closing
> >+ * handshake, so stateful protocols (such as TCP) must implement this
> >+ * themselves.
> 
> That makes it sound like it doesn't even do a FIN...

I think this is also a victim of the interface changing from socket-like to datagram-like over time. I’ve changed it locally to say that all buffered-but-unsent data is sent.

> >+gboolean
> >+g_communicable_close (GCommunicable  *communicable,
> >+                      GCancellable   *cancellable,
> >+                      GError        **error)
> >+{
> >+  g_return_val_if_fail (G_IS_COMMUNICABLE (communicable), FALSE);
> >+  g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
> >+
> >+  return g_communicable_shutdown (communicable, TRUE, TRUE, cancellable, error);
> >+}
> 
> That's not the same as close() for a socket. It still leaves the fd open.
> (And, eg, g_socket_is_closed() will still return FALSE after this.)

Hmm, this is a bit of a pain. I read the man pages multiple times to try and figure out the difference between shutdown(SHUT_RDWR) and close(), but missed that.

In what cases is it useful to shutdown(SHUT_RDWR) but keep the FD open? Is that a case we should be supporting in this interface? Other implementations of the interface might not be based on FDs, so I’m wary of getting too tied up in their semantics.

> >+ * g_communicable_close_async:
> 
> At this API level, it would be more idiomatic to add a @blocking parameter
> to g_communicable_close(). (Likewise for shutdown_async.)

Good point. I’ll add a @timeout parameter instead (as per discussion on bug #697907).

> >+ * Note that on Windows, this function is rather inefficient in the UDP case,
> >+ * and so if you know any plausible upper bound on the size of the incoming
> >+ * packet, it is better to just do a g_socket_receive() with a buffer of that
> 
> "just do a g_communicable_receive_messages()". But maybe this warning
> doesn't belong in the GCommunicable docs anyway...

Dropped locally.

> >+typedef struct _GCommunicableIface GCommunicableIface;
> 
> New types should use "Interface", not "Iface"

Fixed locally.

(In reply to Philip Withnall from comment #66)
> Could go with GSocketlike (lower case ‘l’) which would give
> g_socketlike_is_closed(), which I think is marginally better. Still feels
> ugly though.

Sure

> Emmanuele suggested GDatagramChannel which I think is the best suggestion
> yet.

Or GDatagramBased, to match GFileDescriptorBased...

> In what cases is it useful to shutdown(SHUT_RDWR) but keep the FD open? Is
> that a case we should be supporting in this interface? Other implementations
> of the interface might not be based on FDs, so I’m wary of getting too tied
> up in their semantics.

I'm not sure there are any cases where it's *useful*, but I don't remember the exact semantics.

It seems strange anyway to say that close() must be equivalent to shutdown(RDWR), but then still expose them as separate APIs.

(In reply to Dan Winship from comment #67)
> > Emmanuele suggested GDatagramChannel which I think is the best suggestion
> > yet.
> 
> Or GDatagramBased, to match GFileDescriptorBased...

GDatagramBased works for me; I’ll rename things and submit an updated patch.

> > In what cases is it useful to shutdown(SHUT_RDWR) but keep the FD open? Is
> > that a case we should be supporting in this interface? Other implementations
> > of the interface might not be based on FDs, so I’m wary of getting too tied
> > up in their semantics.
> 
> I'm not sure there are any cases where it's *useful*, but I don't remember
> the exact semantics.
> 
> It seems strange anyway to say that close() must be equivalent to
> shutdown(RDWR), but then still expose them as separate APIs.

close() is a convenience API.

Created attachment 307997 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

GDatagramBased is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GDatagramBased allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GDatagramBased interface, and implements it in GSocket.

---

Updated to:
 • Rename from GCommunicable to GDatagramBased
 • Fix review comments with documentation etc. so far
 • Add a timeout parameter to close() and shutdown()
 • Close the GSocket FD on shutdown(SHUT_RDWR)

This still needs the semantics of close() vs. shutdown() clarifying, and the [send|receive]_messages() methods’ parameters updating once bug #751924 is settled.

If we are bikeshedding what about gmessagebased? It is a bit more generic than datagram.

Disclaimer: I am reading from a phone on holiday and I only added myself to CC since Dan mentioned this interface in the context of websockets

(In reply to Paolo Borelli from comment #70)
> If we are bikeshedding what about gmessagebased? It is a bit more generic
> than datagram.

Bikeshed over. The name is GDatagramBased unless someone comes up with a really compelling argument for me to rename and rebase all the patches again.

(GMessageBased seems a bit too easily confused with non-network-related stuff to me.)

Created attachment 308539 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

GDatagramBased is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GDatagramBased allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GDatagramBased interface, and implements it in GSocket.

Updated patch to use the (potentially final) g_socket_receive_messages() interface from the latest patches in bug #751924:
 • Use per-operation timeouts rather than per-operation blocking parameters
 • Shutdown-vs-close semantics are unchanged

Comment on attachment 308539 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

"a #GDatagramBased" and @datagram_based are awkward... I can't think of any good simplification without renaming again though. (Eg, if it was GDatagramChannel, then you could refer to instances of it as "channels" and name your variables @channel or @chan...)

>+ * Receive multiple data messages from @datagram_based in one go.

"one or more", not "multiple"; while the corresponding GSocket API is likely only going to be used by people who actually want to receive multiple messages, this is the only API available at the GDatagramBased level, so it would be used by people who only wanted a single message as well...

>+ * If @timeout is negative the call will block until @num_messages have been
>+ * received, or the connection is closed remotely (EOS).

(or @cancellable is cancelled, or some other error occurs...)

>+  g_return_val_if_fail (num_messages == 0 || messages != NULL, -1);

is there any reason to allow num_messages to be 0?

>+ * g_datagram_based_send_messages()

same comments as above

>+ * If @timeout is non-negative, and shutting down blocks for longer than that,
>+ * %G_IO_ERROR_TIMED_OUT is returned and any remaining unsent data is lost.
>+ * Pass -1 to @timeout for blocking behaviour.

The documentation up to this point implies that shutting down is an instantaneous operation and doesn't mention anything about "unsent data" (which should probably be "unreceived data" in the @shutdown_read case anyway).

>+ * Closing a #GDatagramBased waits for all buffered but untransmitted data to be
>+ * sent before it returns.

Ah, this presumably belongs in the shutdown() docs.

>+ * g_datagram_based_create_source:
>+ * @datagram_based: a #GDatagramBased
>+ * @condition: a #GIOCondition mask to monitor
>+ * @cancellable: (nullable): a #GCancellable

should this take a timeout too?

>+ * g_datagram_based_condition_timed_wait:

drop "timed" from the name; everything in this API takes a timeout, so you don't need to call attention to it here

(arguably, g_datagram_based_condition_check() could go away and be replaced by g_datagram_based_condition_wait() with @timeout=0...)

>+ * If you donât want a timeout, use g_datagram_based_condition_wait().
>+ * (Alternatively, pass -1 for @timeout.)

bad copy+paste; that function doesn't (currently) exist

>+  /* Close the underlying FD on success. */
>+  if (retval && shutdown_read && shutdown_write)
>+    retval = g_socket_close (G_SOCKET (self), error);

That will leave the socket open if you close the read and write ends in separate shutdown calls.

Created attachment 309418 [details] [review]
gsocket: Fix connected state if shutting down in two steps

The value of g_socket_is_connected() gets stuck high if the GSocket is
shut down in two steps:
   g_socket_shutdown (socket, TRUE, FALSE, NULL);
   g_socket_shutdown (socket, FALSE, TRUE, NULL);
rather than one:
   g_socket_shutdown (socket, TRUE, TRUE, NULL);

Fix that by tracking the connected status for the read half and the
write half of the connection separately.

Created attachment 309419 [details] [review]
fixup! gio: Add GDatagramBased interface and rebase GSocket on it

For the changes from reviews in bug #751924.

Created attachment 309420 [details] [review]
fixup! gio: Add GDatagramBased interface and rebase GSocket on it

For the changes from reviews in bug #697907.

(In reply to Dan Winship from comment #74)
> Comment on attachment 308539 [details] [review] [review]
> gio: Add GDatagramBased interface and rebase GSocket on it
> 
> "a #GDatagramBased" and @datagram_based are awkward... I can't think of any
> good simplification without renaming again though. (Eg, if it was
> GDatagramChannel, then you could refer to instances of it as "channels" and
> name your variables @channel or @chan...)

The only simplification I can think of is s/@datagram_based/@socket/ which is not strictly correct, but should be understandable in most situations.

I really don’t want to rename again. I’ve been through that pain twice now.

> >+ * Receive multiple data messages from @datagram_based in one go.
> 
> "one or more", not "multiple"; while the corresponding GSocket API is likely
> only going to be used by people who actually want to receive multiple
> messages, this is the only API available at the GDatagramBased level, so it
> would be used by people who only wanted a single message as well...

Good point. Fixed.

> >+ * If @timeout is negative the call will block until @num_messages have been
> >+ * received, or the connection is closed remotely (EOS).
> 
> (or @cancellable is cancelled, or some other error occurs...)

Fixed.

> >+  g_return_val_if_fail (num_messages == 0 || messages != NULL, -1);
> 
> is there any reason to allow num_messages to be 0?

There’s no good reason not to? I’ve left this as-is for now.

> >+ * g_datagram_based_send_messages()
> 
> same comments as above

Fixed.

> >+ * If @timeout is non-negative, and shutting down blocks for longer than that,
> >+ * %G_IO_ERROR_TIMED_OUT is returned and any remaining unsent data is lost.
> >+ * Pass -1 to @timeout for blocking behaviour.
> 
> The documentation up to this point implies that shutting down is an
> instantaneous operation and doesn't mention anything about "unsent data"
> (which should probably be "unreceived data" in the @shutdown_read case
> anyway).

Fixed.

> >+ * Closing a #GDatagramBased waits for all buffered but untransmitted data to be
> >+ * sent before it returns.
> 
> Ah, this presumably belongs in the shutdown() docs.

Fixed, as above.

> >+ * g_datagram_based_create_source:
> >+ * @datagram_based: a #GDatagramBased
> >+ * @condition: a #GIOCondition mask to monitor
> >+ * @cancellable: (nullable): a #GCancellable
> 
> should this take a timeout too?

What semantics would you have for it? A recurring timeout, or once-off? I’m tempted to suggest using g_source_set_ready_time(), but it says that it’s for GSource implementations only.

> >+ * g_datagram_based_condition_timed_wait:
> 
> drop "timed" from the name; everything in this API takes a timeout, so you
> don't need to call attention to it here

Good point, fixed.

> (arguably, g_datagram_based_condition_check() could go away and be replaced
> by g_datagram_based_condition_wait() with @timeout=0...)

I thought about that, but they’re subtly different because g_datagram_based_condition_check() can additionally return G_IO_HUP, whereas g_datagram_based_condition_wait() will return TRUE with no further indication of which condition was met.

I guess the vfuncs could be merged; and g_datagram_based_condition_wait() could be changed to return a GIOCondition instead of a gboolean. That would make it less consistent with other g_*_wait() APIs though. Thoughts?

> >+ * If you donât want a timeout, use g_datagram_based_condition_wait().
> >+ * (Alternatively, pass -1 for @timeout.)
> 
> bad copy+paste; that function doesn't (currently) exist

Oops. Fixed.

> >+  /* Close the underlying FD on success. */
> >+  if (retval && shutdown_read && shutdown_write)
> >+    retval = g_socket_close (G_SOCKET (self), error);
> 
> That will leave the socket open if you close the read and write ends in
> separate shutdown calls.

Fixed, plus an additional patch (to be applied before the GDatagramBased patch) which fixes the same bug in g_socket_shutdown().

Created attachment 309452 [details] [review]
fixup! gio: Add GDatagramBased interface and rebase GSocket on it

For the changes from reviews in bug #697907.

Created attachment 309453 [details] [review]
fixup! gio: Add GDatagramBased interface and rebase GSocket on it

Error out of GDatagramBased method implementations in GSocket if
GSocket:type is not datagram-based.

I've been trying to use this to implement some things and the recv() style API where the caller passes a buffer is quite problematic. The common use-case is for each layer to add a header. For the sending side, it works fine as you can add more elements to the GOutputVector array. But for the receive side, it means each layer needs to do a memcpy into the smaller buffer provided by the previous layer and you get annoying problems liks MSG_TRUNC. Instead I suggest adding a GBytes to the GInputMessage instead of an array of GInputVector, the GSocket would allocate them (and pool them to avoid an allocation for each buffer). And then intermediary layers can creates subsets with g_bytes_new_from_bytes(). Ideally, we'd have a g_bytes_new_take_bytes() that eats the GBytes paramters and returns the same one but with a smaller size so we can avoid allocations entirely.

Another possibility is to allow the callee to modify the content of the GInputVector struct to create subbuffers by moving the data pointer down and/or reducing the size. That would solve many cases, but still wouldn't help the case where you have a demultiplexer.

Unfortunately in the case of a (D)TLS connection sitting on top of *anything*, that’s not going to work without memcpy()s, because GnuTLS controls its own buffer allocation, and as far as I can see, there is no way to control which buffer gets passed to the pull_func (see glib-networking/tls/gnutls/gtlsconnection-gnutls.c).

So even if all the lower layers use GBytes allocated by the GSocket at the very bottom, there has to be at least one memcpy() from (a sub-region of) one of those GBytes to the buffer provided by GnuTLS.

I think it’s possible to achieve what you want using the current API already: when a higher layer is forwarding a receive_messages() call to a lower layer, it needs to reallocate the array of GInputVectors in the GInputMessage to prepend a new one (for the header it’s expecting). This is actually more flexible than using GBytes, because the array could be modified in arbitrary ways when it’s reallocated, rather than just adding a vector at one or both ends of it — though I’m not sure whether this flexibility would be useful in practice.

The ideal which I was aiming for with this API is for user code to provide a buffer which is passed all the way down to the kernel, filled, and passed all the way back up with zero memcpy()s on the payload. Obviously, this isn’t possible if intermediate elements in the stream are doing buffering (I’m looking at you, libnice’s pseudo-TCP socket, and GnuTLS).

For reference, Olivier benchmarked the alternative of using G_SOCKET_MSG_PEEK to (for example) work out which conversation an input to a demuxer should be outputted on, and it’s an order of magnitude slower than doing extra buffering and memcpy()s, due to the extra syscall.

I did a diagram to help my thinking. It might be useful or might be completely unintelligible: https://tecnocode.co.uk/misc/g-datagram-based_buffer-management_2015-09-11.pdf

Created attachment 312294 [details] [review]
gsocket: Fix connected state if shutting down in two steps

The value of g_socket_is_connected() gets stuck high if the GSocket is
shut down in two steps:
   g_socket_shutdown (socket, TRUE, FALSE, NULL);
   g_socket_shutdown (socket, FALSE, TRUE, NULL);
rather than one:
   g_socket_shutdown (socket, TRUE, TRUE, NULL);

Fix that by tracking the connected status for the read half and the
write half of the connection separately.

Created attachment 312296 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

GDatagramBased is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GDatagramBased allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GDatagramBased interface, and implements it in GSocket.

Rebased. Branch at https://git.collabora.com/cgit/user/pwith/glib.git/log/?h=g-socket-interface.

Created attachment 312480 [details] [review]
gsocket: Fix connected state if shutting down in two steps

The value of g_socket_is_connected() gets stuck high if the GSocket is
shut down in two steps:
   g_socket_shutdown (socket, TRUE, FALSE, NULL);
   g_socket_shutdown (socket, FALSE, TRUE, NULL);
rather than one:
   g_socket_shutdown (socket, TRUE, TRUE, NULL);

Fix that by tracking the connected status for the read half and the
write half of the connection separately.

---

Rebased on master now that we’ve branched for 2.48.

Created attachment 312481 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

GDatagramBased is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GDatagramBased allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GDatagramBased interface, and implements it in GSocket.

---

 • Rebased on master now that we’ve branched for 2.48.
 • Changed the version numbers for APIs to 2.48.

Created attachment 312482 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

GDatagramBased is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GDatagramBased allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GDatagramBased interface, and implements it in GSocket.

---

 • Rebased on master now that we’ve branched for 2.48.
 • Changed the versions for new APIs to 2.48.

(In reply to Philip Withnall from comment #86)
> Rebased. Branch at
> https://git.collabora.com/cgit/user/pwith/glib.git/log/?h=g-socket-interface.

That branch has one unrelated commit on it ("gobject: Add a boxed type for GAsyncQueue")

Comment on attachment 312480 [details] [review]
gsocket: Fix connected state if shutting down in two steps

It's probably a bug that connected gets set FALSE after shutdown()... either that or it's a bug that connected *doesn't* get set FALSE after recv() returns 0 or send() returns ECONNRESET/EPIPE...

>  * Check whether the socket is connected. This is only useful for
>  * connection-oriented sockets.
>  *
>+ * If using g_socket_shutdown(), this function will return %TRUE until the
>+ * socket has been shut down for reading and writing.
>+ *
>  * Returns: %TRUE if socket is connected, %FALSE otherwise.

while we're clarifying things here...

"If you do a non-blocking connect, this function will not return %TRUE until after you call g_socket_check_connect_result()."

Comment on attachment 312482 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

>+ * #GSocket, which wraps the UNIX socket API and winsock2 on Windows.

"which wraps the UNIX socket API on UNIX and winsock2 on Windows"

>+ * error. A non-blocking will return immediately with a %G_IO_ERROR_WOULD_BLOCK

"A non-blocking operation"

>+ * g_datagram_based_receive_messages:

>+ * @flags: (inout): an int containing #GSocketMsgFlags flags

no longer (inout)

>+ * are passed in as-is, so you can pass in system-specific flags too. The flags
>+ * for specific messages can also be set in each #GInputMessage.

Not true, drop that sentence.

>+ * messages successfully received before the error will be returned. If
>+ * @cancellable is cancelled, -1 is returned and @error is set to
>+ * %G_IO_ERROR_CANCELLED; any pending received messages are discarded.

It's unclear if "pending" here means "messages that g_datagram_based_receive_messages() has read but not yet returned to you" or "messages that the system has received but g_datagram_based_receive_messages() has not yet read". At any rate, neither reading of the statement seems to be true of the GSocket implementation; cancellation is treated the same way as any other error.

>+  g_return_val_if_fail (timeout > 0 ||
>+                        !g_error_matches (child_error, G_IO_ERROR,
>+                                          G_IO_ERROR_TIMED_OUT), -1);

This doesn't always hold with the GSocket implementation; if GSocket:timeout is set, then the operation may time out even when no timeout is specified here. (I think you want to split out the core of g_socket_condition_timed_wait() into an internal function so that g_socket_condition_timed_wait() can call that function considering the socket's timeout, but block_on_timeout() can ignore it.)

(Note that g_datagram_based_create_source() has the same issue; a source returned from g_socket_create_source() will eventually time out if GSocket:timeout is set. Maybe the right fix is to just make check_datagram_based() refuse to work with sockets with timeouts...)

>+ * If @timeout is negative the call will block until there is space for
>+ * @num_messages in the underlying operating system transmit queue, @cancellable
>+ * is cancelled, or an error occurs.

"until there is space for" makes it sound like it does g_datagram_based_condition_wait() and then returns (without sending anything).

>+ * If @timeout is positive the call will block on the same conditions as if
>+ * @timeout were negative. If the timeout is reached before any messages are
>+ * sent, %G_IO_ERROR_TIMED_OUT is returned, otherwise up to @num_messages are
>+ * returned.

"otherwise it will return the number of messages sent before timing out"

>+ * To be notified when space is available, wait for the %G_IO_OUT condition.

"To be notified when messages can be sent".

>+ * successfully sent before the error will be returned. If @cancellable is
>+ * cancelled, -1 is returned and @error is set to %G_IO_ERROR_CANCELLED; any
>+ * unsent messages are discarded.

Again, that does not seem to be true of the GSocket implementation.

>+ * Returns: number of messages sent, or -1 on error. Note that the number of
>+ *     messages received may be smaller than @num_messages if @timeout is zero

s/received/sent/

>+ * Shut down part of a full-duplex connection, waiting for all buffered but
>+ * untransmitted data to be sent before returning.

"Shut down part or all"

And the second half of that belongs in the "If @shutdown_write is %TRUE" section; presumably we don't actually do that when doing just a shutdown_read. Although in fact, we don't do it in the shutdown_write case either; shutdown() only blocks if you've set SO_LINGER on the socket.

And if you're doing the shutdown_write, read, shutdown_read dance, then it doesn't actually matter to you exactly when the buffered data has been sent, and if you're not doing that then there's no guarantee the peer received your data even if you did wait for it all to get flushed. So I think we should just remove the claims about waiting.

>+ * It is allowed for both @shutdown_read and @shutdown_write to be TRUE â this
>+ * is equivalent to calling g_datagram_based_close().
>+ *
>+ * One example where this is used is graceful disconnect for stateful

"this" seems like it refers to the immediately-preceding sentence, when it actually refers to the opposite.

"One example where it is useful to shut down only one side of a connection..."

>+ * If @timeout is non-negative, and shutting down blocks for longer than that,
>+ * %G_IO_ERROR_TIMED_OUT is returned and any remaining unsent or unreceived data
>+ * is lost. Pass -1 to @timeout for blocking behaviour.

It is awkward that this is a potentially blocking operation (eg, in the DTLS case) but we provide no way to poll for its readiness.

>+ * %G_IO_OUT will be set if it is expect that at least one byte can be sent

expected

>+ * Note that although @timeout is in microseconds for consistency with other
>+ * GLib APIs, this function actually only has millisecond resolution, and the
>+ * behavior is undefined if @timeout is not an exact number of milliseconds.

That is true of the GSocket implementation, but not necessarily all GDatagramBased implementations.

>+ * Get the amount of data pending in the underlying receive buffer. This is the
>+ * amount of data which can be received in a single #GInputMessage passed to
>+ * g_datagram_based_receive_messages(). This is the size of a single datagram.

Needs a "without blocking" in there.

Also, the statements seem contradictory. If there are two datagrams in the receive buffer, you can receive them both in a single GInputMessage...

Comment on attachment 312480 [details] [review]
gsocket: Fix connected state if shutting down in two steps

Pushed with the suggested documentation fix from comment #91.

Attachment 312480 [details] pushed as 237fec7 - gsocket: Fix connected state if shutting down in two steps

Created attachment 312624 [details] [review]
gio: Add GDatagramBased interface and rebase GSocket on it

GDatagramBased is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GDatagramBased allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GDatagramBased interface, and implements it in GSocket.

(All comments fixed unless otherwise specified.)

I have pushed commit fc59c20e97a196a261d9caeb573ce411a98b8c32 to apply some of these docs improvements to the GSocket documentation.

(In reply to Dan Winship from comment #92)
> Comment on attachment 312482 [details] [review] [review]
> gio: Add GDatagramBased interface and rebase GSocket on it
>
> >+ * messages successfully received before the error will be returned. If
> >+ * @cancellable is cancelled, -1 is returned and @error is set to
> >+ * %G_IO_ERROR_CANCELLED; any pending received messages are discarded.
> 
> It's unclear if "pending" here means "messages that
> g_datagram_based_receive_messages() has read but not yet returned to you" or
> "messages that the system has received but
> g_datagram_based_receive_messages() has not yet read". At any rate, neither
> reading of the statement seems to be true of the GSocket implementation;
> cancellation is treated the same way as any other error.

Reworded to show that cancellation is just another error, and is not treated differently (as it is in some APIs).

> >+  g_return_val_if_fail (timeout > 0 ||
> >+                        !g_error_matches (child_error, G_IO_ERROR,
> >+                                          G_IO_ERROR_TIMED_OUT), -1);
> 
> This doesn't always hold with the GSocket implementation; if GSocket:timeout
> is set, then the operation may time out even when no timeout is specified
> here. (I think you want to split out the core of
> g_socket_condition_timed_wait() into an internal function so that
> g_socket_condition_timed_wait() can call that function considering the
> socket's timeout, but block_on_timeout() can ignore it.)
> 
> (Note that g_datagram_based_create_source() has the same issue; a source
> returned from g_socket_create_source() will eventually time out if
> GSocket:timeout is set. Maybe the right fix is to just make
> check_datagram_based() refuse to work with sockets with timeouts...)

I agree that the right fix (well, the only fix without making things hideously complex and having different timeout semantics for a given GSocket instance depending on whether you call g_socket_*() or g_datagram_based_*()) is to ban #GSocket:timeout in check_datagram_based(). Done.

> >+ * Shut down part of a full-duplex connection, waiting for all buffered but
> >+ * untransmitted data to be sent before returning.
> 
> "Shut down part or all"
> 
> And the second half of that belongs in the "If @shutdown_write is %TRUE"
> section; presumably we don't actually do that when doing just a
> shutdown_read. Although in fact, we don't do it in the shutdown_write case
> either; shutdown() only blocks if you've set SO_LINGER on the socket.
> 
> And if you're doing the shutdown_write, read, shutdown_read dance, then it
> doesn't actually matter to you exactly when the buffered data has been sent,
> and if you're not doing that then there's no guarantee the peer received
> your data even if you did wait for it all to get flushed. So I think we
> should just remove the claims about waiting.

Claims removed.

> >+ * If @timeout is non-negative, and shutting down blocks for longer than that,
> >+ * %G_IO_ERROR_TIMED_OUT is returned and any remaining unsent or unreceived data
> >+ * is lost. Pass -1 to @timeout for blocking behaviour.
> 
> It is awkward that this is a potentially blocking operation (eg, in the DTLS
> case) but we provide no way to poll for its readiness.

Yeah. I'm not sure whether 'readiness' is a well-defined concept for DTLS shutdown, though, since it's a two-way handshake of CloseNotify messages, and we can't know the readiness of the peer even if we know that *we* are ready to send *our* CloseNotify.

I see three options:
 • (a) allow polling simply for G_IO_OUT, which means we know we can send the initial part of a shutdown handshake for any protocol, even if we end up later blocking on a subsequent part of the protocol; or
 • (b) turn shutdown() into a proper GAsyncResult function; or
 • (c) a combination of (a) and (b) where any implementation of #GDatagramBased which _can_ poll for readiness for the entire shutdown() operation does so as a G_IO_OUT poll, and any implementation which can't must provide its own GAsyncResult-based shutdown_async() method, not as part of its #GDatagramBased implementation. So DTLS would provide a g_dtls_connection_shutdown_async() method.

(a) means we could still block without knowing we would in advance, but keeps things simple. (b) covers all possibilities, but doesn't sit well with the rest of the (non-GAsyncResult-based) #GSocket API. (c) is the best and worst of both?

I'm not really sure of the best approach here.

> >+ * Note that although @timeout is in microseconds for consistency with other
> >+ * GLib APIs, this function actually only has millisecond resolution, and the
> >+ * behavior is undefined if @timeout is not an exact number of milliseconds.
> 
> That is true of the GSocket implementation, but not necessarily all
> GDatagramBased implementations.

Removed. The GSocket behaviour is not actually undefined — it just divides by 1000 and hence may drop up to 999 microseconds from the timeout period.

> >+ * Get the amount of data pending in the underlying receive buffer. This is the
> >+ * amount of data which can be received in a single #GInputMessage passed to
> >+ * g_datagram_based_receive_messages(). This is the size of a single datagram.
> 
> Needs a "without blocking" in there.

Added.

> Also, the statements seem contradictory. If there are two datagrams in the
> receive buffer, you can receive them both in a single GInputMessage...

Nope, you will receive one of them in one GInputMessage and the second in another GInputMessage (if passed to receive_messages()).

This does make me question the utility of this method. It is definitely useful for the case of checking 'is there _any_ data available to read', but isn't so useful for the case of allocating correctly-sized GInputVectors for reading the available data. Then again, if your code is tailoring and allocating buffers for each pending datagram, you've lost already, and you might as well receive the datagrams one at a time (one GInputMessage per receive_messages() call) and call get_available_bytes() each time.

The other solutions: (a) returning the total number of bytes available across all pending datagrams; or (b) returning an array of the sizes of each pending datagram; are not so good either. (a) means you don't know the size of each individual datagram. (b) means you have to deal with an array of sizes. Also, I'm not sure how to get that data out of the OS anyway. SO_NREAD is not helpful here, and neither is FIONREAD.

So I guess the current API is reasonable enough, and hopefully good enough now the documentation isn't a pack of lies.

(In reply to Philip Withnall from comment #95)
> > It is awkward that this is a potentially blocking operation (eg, in the DTLS
> > case) but we provide no way to poll for its readiness.
> 
> Yeah. I'm not sure whether 'readiness' is a well-defined concept for DTLS
> shutdown, though, since it's a two-way handshake of CloseNotify messages,
> and we can't know the readiness of the peer even if we know that *we* are
> ready to send *our* CloseNotify.
> 
> I see three options:
>  • (a) allow polling simply for G_IO_OUT, which means we know we can send
> the initial part of a shutdown handshake for any protocol, even if we end up
> later blocking on a subsequent part of the protocol; or

If close() is going to wait to receive a CloseNotify as well, then this doesn't really solve the problem, since you could be waiting arbitrarily long for that.

We'd need to do something like add G_IO_CLOSE or something...

> (a) means we could still block without knowing we would in advance, but
> keeps things simple. (b) covers all possibilities, but doesn't sit well with
> the rest of the (non-GAsyncResult-based) #GSocket API. (c) is the best and
> worst of both?
> 
> I'm not really sure of the best approach here.

Two more ideas:

  (d) Like (c), but be consistent; say that shutdown()/close(), like
      connect(), is not GDatagramBased's problem, and you always need to
      use underlying APIs to shutdown/close. This seems like it might be
      problematic for some use cases, but no worse than (c)...

  (e) Be like connect(), and say that shutdown() and close() don't block
      if timeout is 0, but they may return G_IO_ERROR_PENDING. And then
      add an additional function or functions to get the result later.
      (Probably also need a function to return the GIOCondition that you
      need to wait for before calling the get-result method, since it's not
      going to consistently be G_IO_OUT like in the connect() case.)

> > Also, the statements seem contradictory. If there are two datagrams in the
> > receive buffer, you can receive them both in a single GInputMessage...
> 
> Nope, you will receive one of them in one GInputMessage and the second in
> another GInputMessage (if passed to receive_messages()).

Ah, right. I guess I meant "in a single array-of-GInputMessage", not "in a single GInputMessage".

> This does make me question the utility of this method.

Yeah... are you actually using it? If not, we could get rid of it now, and add it later if someone needs it.

(In reply to Dan Winship from comment #96)
> Two more ideas:
> 
>   (d) Like (c), but be consistent; say that shutdown()/close(), like
>       connect(), is not GDatagramBased's problem, and you always need to
>       use underlying APIs to shutdown/close. This seems like it might be
>       problematic for some use cases, but no worse than (c)...
> 
>   (e) Be like connect(), and say that shutdown() and close() don't block
>       if timeout is 0, but they may return G_IO_ERROR_PENDING. And then
>       add an additional function or functions to get the result later.
>       (Probably also need a function to return the GIOCondition that you
>       need to wait for before calling the get-result method, since it's not
>       going to consistently be G_IO_OUT like in the connect() case.)

Some background: I've got an implementation of GDatagramBased in libnice, which exposes an ICE connection. ICE connections can either be unreliable (ICE-UDP, datagram semantics) or reliable (ICE-TCP, stream semantics). The plan is for libnice to provide:
 • nice_agent_get_socket(), returns a GDatagramBased if the connection is ICE-UDP-based, NULL otherwise
 • nice_agent_get_io_stream(), returns a GIOStream if the connection is ICE-TCP-based, NULL otherwise

So users of libnice need to handle a GDatagramBased or a GIOStream object roughly equivalently. Of course, if they want a particular set of semantics (say, always stream-based), they could error out if the nice_agent_get_io_stream() method returns NULL.

So, on one hand I think GDatagramBased should expose a shutdown()/close() method (in some shape) for symmetry with GIOStream. On the other hand, we could quite easily require the user to call nice_agent_remove_stream() to remove the ICE stream regardless of whether it's ICE-UDP or ICE-TCP.

Looking at another part of libnice, we have a PseudoTcpSocket which takes a GDatagramBased base socket and implements pseudo-TCP on top of it. PseudoTcpSocket itself is a GIOStream. Currently, calling g_io_stream_close() on the PseudoTcpSocket closes the underlying GDatagramBased base socket, which is reasonably intuitive. However, GIOStream specifically allows for implementations to *not* free underlying resources when g_io_stream_close() is called, so this could be changed and it would still be consistent with the rest of the platform.

For GDtlsConnection, I think it would be quite reasonable to add a g_dtls_connection_close_async() method — and we could probably bundle it up with GDtlsConnection:require-close-notify. There's no need to follow the precedent of GTlsConnection (which uses g_io_stream_close()) because GDtlsConnection is definitely not a GIOStream.

---

So after all that rambling, I am leaning towards eliminating shutdown()/close()/is_closed() from GDatagramBased, and leaving those to implementations to handle (option (d)). After all, they're nothing to do with datagrams.

Yes? No?

> > This does make me question the utility of this method.
> 
> Yeah... are you actually using it? If not, we could get rid of it now, and
> add it later if someone needs it.

I think I was previously, but now am not. Let's drop it. I'll produce another iteration of the patch once we've resolved the shutdown()/close() question.

If dropping shutdown and close works for you then let's do it. As with get_available(), we can always add it later if it turns out to be necessary.

Created attachment 312823 [details] [review]
Removed close(), shutdown(), is_closed(), get_available_bytes() from GDatagramBased

gio: Add GDatagramBased interface and rebase GSocket on it

GDatagramBased is an interface abstracting datagram-based communications
in the style of the Berkeley sockets API. It may be contrasted to (for
example) GIOStream, which supports only streaming I/O.

GDatagramBased allows socket-like communications to be done through any
object, not just a concrete GSocket (which wraps socket()).

This adds the GDatagramBased interface, and implements it in GSocket.

Fixed! Thanks Dan. :-)