Created attachment 306765 [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 306766 [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 306767 [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.

Comment on attachment 306765 [details] [review]
gsocket: Split out functions to convert to and from struct msghdr

>+/* Unfortunately these have to be macros rather than inline functions due to
>+ * using alloca(). */

Blah.

I was thinking maybe it would be possible to have a function, with a wrapper macro that does the alloca()s for it, but you need most of that code just to figure out how much to alloc...

>-    /* We always set the close-on-exec flag so we don't leak file
>-     * descriptors into child processes.  Note that gunixfdmessage.c
>-     * will later call fcntl (fd, FD_CLOEXEC), but that isn't atomic.
>-     */
>-#ifdef MSG_CMSG_CLOEXEC
>-    msg.msg_flags |= MSG_CMSG_CLOEXEC;
>-#endif

You appear to have lost this

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

Here's an alternative implementation (some details in the new struct not fully sorted out yet).

Sorry, I was meaning to review/compare/comment on Philip's patches to my implementation. Will do tomorrow.

Comment on attachment 306767 [details] [review]
gsocket: Add g_socket_receive_messages()

> /**
>+ * GInputMessage:
>+ * @address: (optional) (out callee-allocates) (transfer full): return location

I don't think "callee-allocates" is correct here. caller/callee-allocates really only applies to structs that are being filled in, not to pointer types.

>+ * @bytes_received: will be set to the number of bytes that have been received

So that would be (out)? Really, I don't think you're going to be able to annotate this coherently at all. It might make more sense to split this up into multiple structs or multiple arguments to g_socket_receive_messages()? As it is, it's certainly not going to be usable from bindings. (Probably GOutputMessage should not have contained @bytes_sent either...)

>+ * @flags: (inout): collection of #GSocketMsgFlags to pass into the receive
>+ *   call for this message, and will be updated with flags outputted by the
>+ *   call

You don't actually use this, you use the flags argument to g_socket_receive_messages() instead.

>+ * If @control_messages is non-%NULL then it is set to an array of control
>+ * messages received with the message (if any), and ownership is transferred
>+ * to the caller. @num_control_messages is set to the number of elements in
>+ * this array, which may be zero.

This won't work; each received message can include 0 or more control messages; you have to have an array of (GInputVector + array of GSocketControlMessage), not an array of GInputVector and an array of array of GSocketControlMessage.

And actually, you appear to ignore control messages in the HAVE_RECVMMSG case anyway.

(Sigh. It looks like g_socket_send_messages() is broken as well; sendmmsg() lets you send 0 or more control messages with each message, but g_socket_send_messages() sends all of the control messages in the GOutputMessage with each message.)

>+static gssize
>+g_socket_receive_message_with_blocking (GSocket                 *socket,

Any reason you didn't split this out along with send_message_with_blocking() in the previous patch?

Comment on attachment 306765 [details] [review]
gsocket: Split out functions to convert to and from struct msghdr

Personally, I think this is really quite horrible. It's basically write-only code, or copy'n'paste only code in this case. I didn't do this on purpose when I wrote send_messages. Up to GLib maintainers of course.

If you go for it, the one thing that's important to me here is that the address conversion optimisations based on the previous message/hdr structure are maintained, both for send_messages() and receive_messages().

For the GInputMessage structure, I've considered a few different things, including what you have implemented. I don't think the way passing back the address and control message is done here is very nice, and I wonder if it works for bindings at all.

What my current patch implements is that each struct has flags so the caller can specify if they want address/cmsgs extracted, but since that choice is never really done on a per-message basis in practice, it seems wasteful and unelegant.

My current opinion is that passing another flags argument (in addition to the socket flags argument, since I don't think we have a guaranteed range of flags we can use for our own purposes, or do we?) to _receive_messages() where caller can pass RETRIEVE_CONTROL_MESSAGES | RETRIEVE_ADDRESS and then the fields will be set, otherwise they'll be NULL. Explicit opt-in is required IMHO since the caller will have to free the result.

Comment on attachment 306766 [details] [review]
gsocket: Factor out blocking parameter from g_socket_send_messages()

Attachment 306766 [details] pushed as 8520ae3 - gsocket: Factor out blocking parameter from g_socket_send_messages()

As for the _receive_messages() API, a timeout property is much more useful than a blocking boolean and you can easily express a blocking boolean with an unlimited timeout. The GSocket timeout is not good enough in practice.

timeout argument, not property.

Created attachment 307979 [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.

This depends on the introduction of GInputMessage as another patch in
bug #751924.

---

Re-add the missing CLOEXEC flag (whoops), fix propagation of returned flags in g_socket_receive_message_with_blocking(), and note that this depends on the not-yet-committed GInputMessage struct from another patch in this bug.

(In reply to Dan Winship from comment #6)
> Comment on attachment 306767 [details] [review] [review]
> gsocket: Add g_socket_receive_messages()

Thanks for the review. Most of the points you picked up on are those which differ between my patch and Tim’s, so let’s not go through this review in detail until we’ve decided on the best API. A few replies though:

> >+ * @bytes_received: will be set to the number of bytes that have been received
> 
> So that would be (out)? Really, I don't think you're going to be able to
> annotate this coherently at all. It might make more sense to split this up
> into multiple structs or multiple arguments to g_socket_receive_messages()?
> As it is, it's certainly not going to be usable from bindings. (Probably
> GOutputMessage should not have contained @bytes_sent either...)

Yes, that would be (out). Not sure how splitting it up into multiple structs or arguments to g_socket_receive_messages() would help — I quite like how all the information about one received message is kept in that GInputMessage instance.

I will have to look more closely at bindability. I wasn’t especially considering it before, under the assumption that bindings generally have their own socket API. Not a great assumption.

> >+ * @flags: (inout): collection of #GSocketMsgFlags to pass into the receive
> >+ *   call for this message, and will be updated with flags outputted by the
> >+ *   call
> 
> You don't actually use this, you use the flags argument to
> g_socket_receive_messages() instead.

They are per-message flags, and are copied into the struct msghdr corresponding to each GInputMessage by input_message_to_msghdr().

> >+ * If @control_messages is non-%NULL then it is set to an array of control
> >+ * messages received with the message (if any), and ownership is transferred
> >+ * to the caller. @num_control_messages is set to the number of elements in
> >+ * this array, which may be zero.
> 
> This won't work; each received message can include 0 or more control
> messages; you have to have an array of (GInputVector + array of
> GSocketControlMessage), not an array of GInputVector and an array of array
> of GSocketControlMessage.

If I’m understanding your terminology, what the patch actually has is
   array of GInputMessage
which is equivalent to:
   array of (array of GInputVector, return location for array of GSocketControlMessage, other stuff)

Which I think is consistent with my reading of `man recvmmsg`. Each message (which is an array of GInputVectors) can return zero or more control messages, hence needs a return location for an array of GSocketControlMessages.

GInputVector == struct iovec
GInputMessage == struct mmsghdr

> And actually, you appear to ignore control messages in the HAVE_RECVMMSG
> case anyway.

Like the per-message flags, these pass through the struct msghdr via input_message_[to|from]_msghdr().

> >+static gssize
> >+g_socket_receive_message_with_blocking (GSocket                 *socket,
> 
> Any reason you didn't split this out along with send_message_with_blocking()
> in the previous patch?

Rebase faff. I can do so if you want.

(In reply to Tim-Philipp Müller from comment #7)
> Comment on attachment 306765 [details] [review] [review]
> gsocket: Split out functions to convert to and from struct msghdr
> 
> Personally, I think this is really quite horrible. It's basically write-only
> code, or copy'n'paste only code in this case. I didn't do this on purpose
> when I wrote send_messages. Up to GLib maintainers of course.

I think that’s precisely the reason it’s well-suited for factoring out. It’s just converting between two data structures — there should be no need for that code to be duplicated.

> If you go for it, the one thing that's important to me here is that the
> address conversion optimisations based on the previous message/hdr structure
> are maintained, both for send_messages() and receive_messages().

The conversion optimisations are maintained.

(In reply to Tim-Philipp Müller from comment #8)
> For the GInputMessage structure, I've considered a few different things,
> including what you have implemented. I don't think the way passing back the
> address and control message is done here is very nice, and I wonder if it
> works for bindings at all.

Fair, though the use of an extra pointer indirection basically acts as an implicit flag for ‘I want the address extracted’ or ‘I want the cmsgs extracted’. I think that’s a bit more natural for the API user than having to remember to pass in a flag to enable conversion.

(In reply to Tim-Philipp Müller from comment #10)
> As for the _receive_messages() API, a timeout property is much more useful
> than a blocking boolean and you can easily express a blocking boolean with
> an unlimited timeout. The GSocket timeout is not good enough in practice.

Agreed, I like the concept of per-operation timeouts for this kind of level of API. Am I right in thinking that (timeout == 0) would be guaranteed to replicate the current (blocking == FALSE) behaviour?

(In reply to Philip Withnall from comment #13)
> (In reply to Tim-Philipp Müller from comment #8)
> > For the GInputMessage structure, I've considered a few different things,
> > including what you have implemented. I don't think the way passing back the
> > address and control message is done here is very nice, and I wonder if it
> > works for bindings at all.
> 
> Fair, though the use of an extra pointer indirection basically acts as an
> implicit flag for ‘I want the address extracted’ or ‘I want the cmsgs
> extracted’. I think that’s a bit more natural for the API user than having
> to remember to pass in a flag to enable conversion.

Although the use of an extra pointer indirection does mean that the storage locations for all the information to do with a single network message are not inside the GInputMessage, which is yucky — just as bad as if the bytes_received were stored somewhere else. It means there are multiple pointers to carry around for each message, rather than a single GInputMessage*.

So I think I’m on the fence about flags-vs-return-locations now. Dan, what do you think?

> (In reply to Tim-Philipp Müller from comment #10)
> > As for the _receive_messages() API, a timeout property is much more useful
> > than a blocking boolean and you can easily express a blocking boolean with
> > an unlimited timeout. The GSocket timeout is not good enough in practice.
> 
> Agreed, I like the concept of per-operation timeouts for this kind of level
> of API. Am I right in thinking that (timeout == 0) would be guaranteed to
> replicate the current (blocking == FALSE) behaviour?

tpm confirmed this assumption as correct on IRC. I’m all in favour of the timeout parameter.

(In reply to Philip Withnall from comment #13)
> Yes, that would be (out). Not sure how splitting it up into multiple structs
> or arguments to g_socket_receive_messages() would help — I quite like how
> all the information about one received message is kept in that GInputMessage
> instance.

g-i doesn't have a concept of per-struct-member direction, so in order to be automatically bindable, every argument to g_socket_receive_messages() has to be either entirely "in" or entirely "out" (or entirely in-out)

> I will have to look more closely at bindability. I wasn’t especially
> considering it before, under the assumption that bindings generally have
> their own socket API. Not a great assumption.

JavaScript has no native socket API, and even users of other bindings may want to be using some other API that uses GSockets, and so might want this API along with it.

(OTOH, I don't actually know how bindable g_socket_send_message()/g_socket_receive_message() are...)


> > >+ * @flags: (inout): collection of #GSocketMsgFlags to pass into the receive
> > >+ *   call for this message, and will be updated with flags outputted by the
> > >+ *   call
> > 
> > You don't actually use this, you use the flags argument to
> > g_socket_receive_messages() instead.
> 
> They are per-message flags, and are copied into the struct msghdr
> corresponding to each GInputMessage by input_message_to_msghdr().

Ah, right. That's not inout though; msg->msg_flags is out-only for recvmsg()/recvmmsg() (and provides informations about the messages, as opposed to the g_socket_receive_messages() @flags arg, which modifies how the messages are received).

Ah, this is based on g_socket_receive_message(), which similarly mixes the two concepts up... it really ought to have had an (in) flags argument and an (out) msg_flags argument...

> If I’m understanding your terminology, what the patch actually has is
>    array of GInputMessage
> which is equivalent to:
>    array of (array of GInputVector, return location for array of
> GSocketControlMessage, other stuff)
> 
> Which I think is consistent with my reading of `man recvmmsg`.

Yes, I misread it before.

> > Any reason you didn't split this out along with send_message_with_blocking()
> > in the previous patch?
> 
> Rebase faff. I can do so if you want.

Yes, please

> (In reply to Tim-Philipp Müller from comment #7)
> > Comment on attachment 306765 [details] [review] [review] [review]
> > gsocket: Split out functions to convert to and from struct msghdr
> > 
> > Personally, I think this is really quite horrible. It's basically write-only
> > code, or copy'n'paste only code in this case. I didn't do this on purpose
> > when I wrote send_messages. Up to GLib maintainers of course.
> 
> I think that’s precisely the reason it’s well-suited for factoring out. It’s
> just converting between two data structures — there should be no need for
> that code to be duplicated.

I find the fact that it has to be done as a macro to be horrible, but there's no easy way to fix that...

> (In reply to Tim-Philipp Müller from comment #10)
> > As for the _receive_messages() API, a timeout property is much more useful
> > than a blocking boolean and you can easily express a blocking boolean with
> > an unlimited timeout. The GSocket timeout is not good enough in practice.
> 
> Agreed, I like the concept of per-operation timeouts for this kind of level
> of API. Am I right in thinking that (timeout == 0) would be guaranteed to
> replicate the current (blocking == FALSE) behaviour?

Other than the error code, it seems like it would be the same...

Just to clarify (since my patch doesn't match my comments, and I think that comment was in the other bug anyway), my suggestion for the address/cmsg handling was basically:

typedef struct {
  ...
  GSocketAddress         *address;
  ...
  GSocketControlMessage **control_messages;
  gint                    num_control_messages;
  ...
} GInputMessage;

and then

gint
g_socket_receive_messages (GSocket           *socket,
                           GInputMessage     *messages,
                           guint              num_messages,
                           GInputMessageFlags input_flags,
                           gint               socket_flags,
                           gint64             timeout,
                           GCancellable      *cancellable,
                           GError           **error)

with

typedef enum {
  G_INPUT_MESSAGE_FLAG_NONE,
  G_INPUT_MESSAGE_FLAG_RECEIVE_ADDRESS,
  G_INPUT_MESSAGE_FLAG_RECEIVE_CMESGS
} GInputMessageFlags;

or somesuch.

If NONE is passed to g_socket_receive_messages(), address and control_messages would be NULL and num_control_messages 0 for each message. Otherwise they would be set to a non-NULL pointer, and the caller would have to unref/free them.

(In reply to Dan Winship from comment #15)
> (In reply to Philip Withnall from comment #13)
> > Yes, that would be (out). Not sure how splitting it up into multiple structs
> > or arguments to g_socket_receive_messages() would help — I quite like how
> > all the information about one received message is kept in that GInputMessage
> > instance.
> 
> g-i doesn't have a concept of per-struct-member direction, so in order to be
> automatically bindable, every argument to g_socket_receive_messages() has to
> be either entirely "in" or entirely "out" (or entirely in-out)
> 
> > I will have to look more closely at bindability. I wasn’t especially
> > considering it before, under the assumption that bindings generally have
> > their own socket API. Not a great assumption.
> 
> JavaScript has no native socket API, and even users of other bindings may
> want to be using some other API that uses GSockets, and so might want this
> API along with it.
> 
> (OTOH, I don't actually know how bindable
> g_socket_send_message()/g_socket_receive_message() are...)

I’ve e-mailed gir-devel about this, but had no replies yet. However, ebassi, tpm and I discussed it on IRC just now, and agreed that GInputMessage should be designed to be nice to use from C (the current suggested patch using pointers is apparently OK in that respect). If bindings want to use GInputMessage, convenience API can be added later as required.

A sketch of some convenience API:
 • g_input_message_new() → heap-allocated GInputMessage with address and control_messages set to NULL
 • g_input_message_new_with_address() → heap-allocated GInputMessage with a location for an address tacked on to the end of the allocation and pointed to by the address field
 • g_input_message_new_with_control_messages(guint n_control_messages) → heap-allocated GInputMessage with allocation for an address and the given number of control messages tacked on to the end of the allocation and pointed to appropriately
 • g_input_message_free() — frees the message and whatever was tacked on the end

Could probably also have convenience API for allocating an array of input messages.

One of the big non-bindability concerns, I think, is the fact that GInputMessage is meant to be stack-allocated in C, but bindings will probably want to heap-allocate it.

Just my two cents (as maintainer of the Haskell GI bindings):

The original approach with the in/out annotations on the the GInputMessage itself is rather difficult to bind directly (in fact I am not sure I properly understand the ownership semantics in all cases, and it is certainly not supported in the current haskell gi bindings).

On the other hand, the approach in the previous comment of having a convenience API looks easily bindable.

Ideally, GInputMessage would be a boxed object, with some convenient constructors, and ownership of the memory in the struct fields is tied to ownership of the struct itself. So, in practice, from the bindings one would use g_input_message_new_with_control_messages to build a boxed object, which is then memory managed as any other boxed object.

Created attachment 308531 [details] [review]
giotypes: Add GInputMessage struct

This complements the GOutputMessage struct. It will shortly be used for
adding a g_socket_receive_messages() function, but needs to be committed
first to allow some internal refactoring of GSocket.

Created attachment 308532 [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 308533 [details] [review]
gsocket: Factor out blocking parameter from g_socket_receive_message()

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

Created attachment 308534 [details] [review]
gsocket: Switch internal functions from blocking booleans to timeouts

In order to support per-operation timeouts on new API like
g_socket_receive_messages(), the internal GSocket API should use
timeouts rather than boolean blocking parameters.

   (timeout == 0) === (blocking == FALSE)
   (timeout == -1) === (blocking == TRUE)
   (timeout > 0) === new behaviour

Created attachment 308535 [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, g_socket_receive_messages(), and corresponding unit
tests.

Created attachment 308536 [details] [review]
gsocket: Clarify flags documentation for g_socket_receive_message()

The API design here is a bit awkward — the in/out flags argument should
actually have been an in flags argument and an out msg_flags argument.
Clarify that a bit in the documentation.

Created attachment 308537 [details] [review]
gsocket: Clarify GSocket:blocking doesn’t apply to ops with a parameter

Operations which take an explicit blocking parameter are completely
unaffected by GSocket:blocking.

Created attachment 308538 [details] [review]
gsocket: Decouple GSocket:timeout from methods taking an explicit timeout

If a method takes an explicit timeout parameter, it would be really
confusing for it to return G_IO_ERROR_TIMED_OUT even if (timeout == -1).
Rather than defining some complex semantics for how GSocket:timeout
interacts with per-operation timeouts, just defer entirely to the
method’s timeout parameter.

Updated patch set. Notable changes:
 • Added timeout support everywhere, replacing the blocking parameter. See below for information about the timeout behaviour.
 • Settled on potentially adding convenience API in future to make GInputMessage bindable.
 • Split out GSocket:timeout from the per-operation timeout parameters; just like GSocket:blocking, the per-operation parameter always takes precedence now, and GSocket:timeout is only used for operations which don’t have that parameter.
 • Marked tpm’s patch as rejected accordingly.

Timeout behaviour:
 • (timeout == 0) is non-blocking behaviour
 • (timeout < 0) is blocking behaviour
 • (timeout > 0) blocks up to the timeout, and then returns G_IO_ERROR_TIMED_OUT if no messages were received, otherwise it returns however many messages were received before timing out

Comment on attachment 308531 [details] [review]
giotypes: Add GInputMessage struct

>+ * GInputMessage:
>+ * @address: (optional) (out callee-allocates) (transfer full): return location

"callee-allocates" is wrong. just (out)

>+ * @control_messages: (array length=num_control_messages) (optional)
>+ *   (out callee-allocates) (transfer full): return location for a
>+ *   caller-allocated array of #GSocketControlMessages, or %NULL

likewise here (as noted by the documentation itself)

>+ * This structure closely mirrors `struct mmsghdr` and `struct msghdr` from
>+ * the [POSIX sockets
>+ * API](http://man7.org/linux/man-pages/man2/recvmmsg.2.html#DESCRIPTION).

drop the link; we don't generally url-link man page references, since there's no canonical site to link to that we know isn't going to go away

>+ * If @address is non-%NULL then it is set to the source address the message
>+ * was received from, and ownership is transferred to the caller.

"which the caller must free afterward". We don't usually talk about that in terms of "ownership" in the actual docs. (Likewise for @control_messages.)

(Or maybe there should just be g_input_message_clear()?)

Comment on attachment 308534 [details] [review]
gsocket: Switch internal functions from blocking booleans to timeouts

>+/* Block on a timed wait for @condition until (@start_time + @timeout).
>+ * Return %G_IO_ERROR_TIMED_OUT if the timeout is reached; otherwise %TRUE. */

nitpicky, but the "*/" should be on the next line

>+  if (timeout >= 0)

should probably "g_return_if_fail (timeout != 0)", since this would end up returning the wrong error code if timeout was 0.

>+          g_set_error_literal (error,
>+                               G_IO_ERROR, G_IO_ERROR_TIMED_OUT,
>+                                _("Socket I/O timed out"));

more nitpicks; there's one too many spaces on the last line

>-                if (!g_socket_condition_wait (socket,
>-                                              G_IO_OUT, cancellable, error))
>-                  return -1;
>+                if (!block_on_timeout (socket, G_IO_OUT, timeout, start_time,
>+                                       cancellable, error))
>+                  {
>+                    if (num_sent > 0)
>+                      {
>+                        g_clear_error (error);
>+                        break;

Hm... g_socket_send_messages()'s behavior on error-after-sending-at-least-one-message is broken, and needs more fixing than just this.

sendmmsg() says that "An error is returned only if no datagrams could be sent", and that's probably the intention with g_socket_send_messages() too (and the docs should clarify this), but it doesn't quite do that. Eg, if we're trying to send 2 messages, and the first sendmmsg() returns 1 and so then we call again, and get ECONNRESET, then we'll return an error to the caller, rather than indicating that we successfully sent one message.

>+            gint64 elapsed = g_get_monotonic_time () - start_time;
>+            wait_timeout = MAX (timeout - elapsed, 1);

(The other possibility would be to have the internal API work in terms of end_time rather than timeout, like g_cond_wait_until(). I'm not sure if that would be an overall simplification or not.)

Comment on attachment 308535 [details] [review]
gsocket: Add g_socket_receive_messages()

> /**
>+ * g_socket_receive_messages:

>+ * @timeout: the maximum time (in microseconds) to wait, 0 to not block, or -1
>+ *   to block indefinitely

I would prefer to have this API be parallel to g_socket_send_messages()... people can use the GDatagramBased API if they need the timeout arg.

>+ * Receive multiple data messages from @socket in one go.  This is the most
>+ * complicated and fully-featured version of this call. For easier use, see
>+ * g_socket_receive(), g_socket_receive_from(), and g_socket_receive_message().

g_socket_receive_message() also still claims to be the "most complicated and fully-featured version". (As does g_socket_send_message(). Oops.)


Also, it has the same issue with errors-on-the-second-syscall as g_socket_send_messages().

Comment on attachment 308538 [details] [review]
gsocket: Decouple GSocket:timeout from methods taking an explicit timeout

This patch doesn't do what it says it does.

>@@ -2585,9 +2585,6 @@ g_socket_receive_with_timeout (GSocket       *socket,
>   if (!check_socket (socket, error))
>     return -1;
> 
>-  if (!check_timeout (socket, error))
>-    return -1;

check_timeout() checks for a timeout that *already happened* and just hasn't been reported yet. Specifically, priv->timed_out only gets set if a GSocketSource times out, in which case the source is triggered, and the user's callback function runs, and calls g_socket_receive() or whatever, and that method returns G_IO_ERROR_TIMED_OUT. So... in that case it would be silly of the caller to call g_socket_receive_with_timeout() since they know that either the socket is already ready to read, or else it has already timed out. But anyway, if they did call it, then it should return G_IO_ERROR_TIMED_OUT.

@@ -4833,9 +4840,6 @@ g_socket_receive_messages (GSocket        *socket,
>+ * The value of #GSocket:timeout will be ignored by this function; @timeout is
>+ * used instead.

So this will go away anyway if you remove @timeout as I suggested, but anyway, it's wrong; GSocket:timeout (in synchronous cases) is implemented by g_socket_condition_timed_wait(), which will wait for the shorter of @timeout and :timeout.

Comment on attachment 308534 [details] [review]
gsocket: Switch internal functions from blocking booleans to timeouts

>-            if (msg_error->code == G_IO_ERROR_WOULD_BLOCK && i > 0)
>+            if (i > 0 &&
>+                (msg_error->code == G_IO_ERROR_WOULD_BLOCK ||
>+                 msg_error->code == G_IO_ERROR_TIMED_OUT))

oh, also, the existing code is wrong; it should use g_error_matches() rather than silently assuming that the domain is G_IO_ERROR

Created attachment 309410 [details] [review]
gsocket: Fix error behaviour of g_socket_send_messages()

If an error in the underlying sendmmsg() syscall occurs after
successfully sending one or more messages, g_socket_send_messages()
should return the number of messages successfully sent, rather than an
error. This mirrors the documented sendmmsg() behaviour.

This is a slight behaviour change for g_socket_send_messages(), but as
it relaxes the error reporting (reporting errors in fewer situations
than before), it should not cause problems.

Created attachment 309411 [details] [review]
gsocket: Fix documentation for g_socket_send_message()

It is no longer the most fully featured version of this function —
g_socket_send_messages() stole that dubious honour with 2.44.

Created attachment 309412 [details] [review]
fixup! giotypes: Add GInputMessage struct

Created attachment 309413 [details] [review]
fixup! gsocket: Switch internal functions from blocking booleans to timeouts

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

Created attachment 309415 [details] [review]
fixup! gsocket: Add g_socket_receive_messages()

All review comments fixed unless specified otherwise. Thanks for the reviews; sorry for my slow response — I’ve been away.

(In reply to Dan Winship from comment #28)
> Comment on attachment 308531 [details] [review] [review]
> giotypes: Add GInputMessage struct
>
> >+ * This structure closely mirrors `struct mmsghdr` and `struct msghdr` from
> >+ * the [POSIX sockets
> >+ * API](http://man7.org/linux/man-pages/man2/recvmmsg.2.html#DESCRIPTION).
> 
> drop the link; we don't generally url-link man page references, since
> there's no canonical site to link to that we know isn't going to go away

Replaced with a reference to `man 2 recvmmsg`, since I think it’s important to keep some kind of canonical link.

> >+ * If @address is non-%NULL then it is set to the source address the message
> >+ * was received from, and ownership is transferred to the caller.
> 
> "which the caller must free afterward". We don't usually talk about that in
> terms of "ownership" in the actual docs. (Likewise for @control_messages.)
> 
> (Or maybe there should just be g_input_message_clear()?)

Let’s add g_input_message_clear() later once the requirements for bindability become clearer.

(In reply to Dan Winship from comment #29)
> Comment on attachment 308534 [details] [review] [review]
> gsocket: Switch internal functions from blocking booleans to timeouts
>
> >-                if (!g_socket_condition_wait (socket,
> >-                                              G_IO_OUT, cancellable, error))
> >-                  return -1;
> >+                if (!block_on_timeout (socket, G_IO_OUT, timeout, start_time,
> >+                                       cancellable, error))
> >+                  {
> >+                    if (num_sent > 0)
> >+                      {
> >+                        g_clear_error (error);
> >+                        break;
> 
> Hm... g_socket_send_messages()'s behavior on
> error-after-sending-at-least-one-message is broken, and needs more fixing
> than just this.
> 
> sendmmsg() says that "An error is returned only if no datagrams could be
> sent", and that's probably the intention with g_socket_send_messages() too
> (and the docs should clarify this), but it doesn't quite do that. Eg, if
> we're trying to send 2 messages, and the first sendmmsg() returns 1 and so
> then we call again, and get ECONNRESET, then we'll return an error to the
> caller, rather than indicating that we successfully sent one message.

Fixed in a new commit, and g_socket_receive_messages() updated to match.

> >+            gint64 elapsed = g_get_monotonic_time () - start_time;
> >+            wait_timeout = MAX (timeout - elapsed, 1);
> 
> (The other possibility would be to have the internal API work in terms of
> end_time rather than timeout, like g_cond_wait_until(). I'm not sure if that
> would be an overall simplification or not.)

I’ll leave it as it is for now. I’m not sure it would be a simplification either.

(In reply to Dan Winship from comment #30)
> Comment on attachment 308535 [details] [review] [review]
> gsocket: Add g_socket_receive_messages()
> 
> > /**
> >+ * g_socket_receive_messages:
> 
> >+ * @timeout: the maximum time (in microseconds) to wait, 0 to not block, or -1
> >+ *   to block indefinitely
> 
> I would prefer to have this API be parallel to g_socket_send_messages()...
> people can use the GDatagramBased API if they need the timeout arg.

Fair point. Changed.

> >+ * Receive multiple data messages from @socket in one go.  This is the most
> >+ * complicated and fully-featured version of this call. For easier use, see
> >+ * g_socket_receive(), g_socket_receive_from(), and g_socket_receive_message().
> 
> g_socket_receive_message() also still claims to be the "most complicated and
> fully-featured version". (As does g_socket_send_message(). Oops.)

g_socket_receive_message() fixed in the g_socket_receive_messages() commit. g_socket_send_message() fixed in a separate commit.

(In reply to Dan Winship from comment #32)
> Comment on attachment 308534 [details] [review] [review]
> gsocket: Switch internal functions from blocking booleans to timeouts
> 
> >-            if (msg_error->code == G_IO_ERROR_WOULD_BLOCK && i > 0)
> >+            if (i > 0 &&
> >+                (msg_error->code == G_IO_ERROR_WOULD_BLOCK ||
> >+                 msg_error->code == G_IO_ERROR_TIMED_OUT))
> 
> oh, also, the existing code is wrong; it should use g_error_matches() rather
> than silently assuming that the domain is G_IO_ERROR

Fixed as part of that commit.

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

(This one belongs on bug #697907.)

Comment on attachment 309410 [details] [review]
gsocket: Fix error behaviour of g_socket_send_messages()

>This is a slight behaviour change for g_socket_send_messages(), but as
>it relaxes the error reporting (reporting errors in fewer situations
>than before), it should not cause problems.

I think it's even better than that; it may actually fix bugs, since callers are now getting correctly told about packets that were sent (and when the retry on the packets that weren't sent, they ought to get the error that time).

Not sure which patches to mark as what given the fixups, but the rest of this looks good to commit once glib has branched.

(That is, the two acn patches can be committed now to go into 2.46.1, but the rest need to wait.)

actually, it would be good if you could rebase and squash and then push the branch somewhere... I can't get the patches all to apply...

(In reply to Dan Winship from comment #43)
> (That is, the two acn patches can be committed now to go into 2.46.1, but
> the rest need to wait.)

Not sure which 2 of the 6 acn patches you mean, so I’ve pushed the 3 documentation patches which are definitely relevant to 2.46. I have not pushed the g_socket_send_messages() error behaviour fix since it has a bit more impact.

I’ll rebase and squash the remaining patches shortly, which should hopefully make things a bit easier.

Attachment 308536 [details] pushed as 8fdc670 - gsocket: Clarify flags documentation for g_socket_receive_message()
Attachment 308537 [details] pushed as 347e4a7 - gsocket: Clarify GSocket:blocking doesn’t apply to ops with a parameter
Attachment 309411 [details] pushed as 363fa18 - gsocket: Fix documentation for g_socket_send_message()

Created attachment 312288 [details] [review]
giotypes: Add GInputMessage struct

This complements the GOutputMessage struct. It will shortly be used for
adding a g_socket_receive_messages() function, but needs to be committed
first to allow some internal refactoring of GSocket.

Created attachment 312289 [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 312290 [details] [review]
gsocket: Factor out blocking parameter from g_socket_receive_message()

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

Created attachment 312291 [details] [review]
gsocket: Switch internal functions from blocking booleans to timeouts

In order to support per-operation timeouts on new API like
g_socket_receive_messages(), the internal GSocket API should use
timeouts rather than boolean blocking parameters.

   (timeout == 0) === (blocking == FALSE)
   (timeout == -1) === (blocking == TRUE)
   (timeout > 0) === new behaviour

Created attachment 312292 [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, g_socket_receive_messages(), and corresponding unit
tests.

Created attachment 312293 [details] [review]
gsocket: Fix error behaviour of g_socket_send_messages()

If an error in the underlying sendmmsg() syscall occurs after
successfully sending one or more messages, g_socket_send_messages()
should return the number of messages successfully sent, rather than an
error. This mirrors the documented sendmmsg() behaviour.

This is a slight behaviour change for g_socket_send_messages(), but as
it relaxes the error reporting (reporting errors in fewer situations
than before), it should not cause problems.

(In reply to Dan Winship from comment #44)
> actually, it would be good if you could rebase and squash and then push the
> branch somewhere... I can't get the patches all to apply...

Patches rebased and squashed, and branch pushed here: https://git.collabora.com/cgit/user/pwith/glib.git/log/?h=g-socket-interface

These patches were formerly reviewed and accepted; I haven’t re-set that status because they needed a little bit of tweaking for the socket_set_error_lazy() change:
 • Split out functions to convert to and from struct msghdr
 • Factor out blocking parameter from g_socket_receive_message()
 • Fix error behaviour of g_socket_send_message()

(In reply to Dan Winship from comment #42)
> Not sure which patches to mark as what given the fixups, but the rest of
> this looks good to commit once glib has branched.

GLib has now branched. Did you mean for *all* of the rest of the patches to be committed now?

yes

Merged, with changes to the symbol versions for 2.48 rather than 2.46.

Thanks for the patient reviews. :-)  Next step: bug #697907.

Attachment 312288 [details] pushed as 8c4c16d - giotypes: Add GInputMessage struct
Attachment 312289 [details] pushed as 5d68947 - gsocket: Split out functions to convert to and from struct msghdr
Attachment 312290 [details] pushed as 7f985b3 - gsocket: Factor out blocking parameter from g_socket_receive_message()
Attachment 312291 [details] pushed as a0cefc2 - gsocket: Switch internal functions from blocking booleans to timeouts
Attachment 312292 [details] pushed as f62cbfc - gsocket: Add g_socket_receive_messages()
Attachment 312293 [details] pushed as 1086507 - gsocket: Fix error behaviour of g_socket_send_messages()