GNOME Bugzilla – Bug 769994
GSocket control messages API is not ideal
Last modified: 2018-05-24 19:02:49 UTC
Currently control messages are implemented as full GObjects and are globally registered. This has a couple of problems: - there could be multiple implementations for the same control message if you e.g. use two libraries that want to use the same one and implement them locally - the boilerplate code required to add support for a control message is immense - you require yet another quite expensive allocation per UDP packet, which can become a real problem if you have a few 10k packets per second It would seem like a more flexible approach that would also cover the above, to register per socket (or even per receive/send) the interest in specific control messages together with functions to serialize/parse them, and have them passed as simple C struct pointers to the API. That way one could e.g. also implement support for a specific control message by just having the content of it memcpy'd into stack allocated memory during receiving. This should be possible to implement in GSocket, etc in a backwards compatible way even at this point and allows to deprecate the existing API. It would have to come with new g_socket_receive/send_messages() variants with new names though. Any opinions? Other ideas how this can be implemented in a better way to solve the above problems?
(In reply to Sebastian Dröge (slomo) from comment #0) > This should be possible to implement in GSocket, etc in a backwards > compatible way even at this point and allows to deprecate the existing API. > It would have to come with new g_socket_receive/send_messages() variants > with new names though. Ugh. We *just* added that API. We already have 4 receive variants and 4 send variants, and this would add another pair. Maybe if you have "register[ed] per socket the interest in specific control messages", then send/receive_message[s] could just ignore the GSocketControlMessage parts of the API, and use the registered callbacks instead. Eg g_socket_set_cmsg_handler (sock, SOL_SOCKET, SCM_CREDENTIALS, parse_scm_creds, &creds); nread = g_socket_receive (sock, buf, sizeof (buf), cancellable, &error); g_socket_set_cmsg_handler (sock, SOL_SOCKET, SCM_CREDENTIALS, NULL, NULL); This could also help with bug 618556...
(In reply to Dan Winship from comment #1) > (In reply to Sebastian Dröge (slomo) from comment #0) > > This should be possible to implement in GSocket, etc in a backwards > > compatible way even at this point and allows to deprecate the existing API. > > It would have to come with new g_socket_receive/send_messages() variants > > with new names though. > > Ugh. We *just* added that API. We already have 4 receive variants and 4 send > variants, and this would add another pair. Sure, it's a bit annoying that I only noticed that now :) I didn't need it anywhere else before. Do you know who is using it and for what? It seems to me like the current API is rather suboptimal for all low-latency and/or high-packet-rate use cases, which should be the majority of what people are using UDP for. > Maybe if you have "register[ed] per socket the interest in specific control > messages", then send/receive_message[s] could just ignore the > GSocketControlMessage parts of the API, and use the registered callbacks > instead. Eg > > g_socket_set_cmsg_handler (sock, SOL_SOCKET, SCM_CREDENTIALS, > parse_scm_creds, &creds); > nread = g_socket_receive (sock, buf, sizeof (buf), cancellable, &error); > g_socket_set_cmsg_handler (sock, SOL_SOCKET, SCM_CREDENTIALS, NULL, NULL); > > This could also help with bug 618556... That would work for me, but having to add one handler per message type might not be ideal either. Maybe just pass all to all handlers and let them filter themselves?
(In reply to Sebastian Dröge (slomo) from comment #2) > Do you know who is using it and for what? It seems to me like the current > API is rather suboptimal for all low-latency and/or high-packet-rate use > cases, which should be the majority of what people are using UDP for. The only use case that was really considered with the control message API was unix sockets (ie, credentials and fd passing). > That would work for me, but having to add one handler per message type might > not be ideal either. Maybe just pass all to all handlers and let them filter > themselves? sure
Created attachment 340034 [details] [review] Patch to add the control message callback function Would this be something you guys have in your mind?
Created attachment 340422 [details] [review] Patch to add the control message callback function Here is an update of the patch: - Re-applied to master. - Now also supports receive_messages(). The callback got an extra parameter pkt_index, which indicates which packet it is in the sequence if called through receive_messages()
Review of attachment 340422 [details] [review]: Generally looks good to me ::: gio/gsocket.c @@ +4219,3 @@ + + /* capture the flags */ + message->flags = msg->msg_flags; This seems inconsistent. Shouldn't they be just passed to the callback too? @@ +5622,3 @@ + * Returns: success or failure. + * + */ Since: 2.52 ::: gio/gsocket.h @@ +64,3 @@ + * @user_data - user data provided to g_socket_set_cmsg_handler() is passed via this argument + */ +typedef void (*GSocketCmsgCallback) (int level, int type, const gpointer data, gsize size, gint pkt_index, gpointer user_data); Why not a non-void typed pointer for the data, like a const guint8 * @@ +322,3 @@ +gboolean g_socket_set_cmsg_handler (GSocket *socket, + GSocketCmsgCallback cmsg_fn, + gpointer cmsg_data); Should probably be called user_data to make gobject-introspection more happy. And you also want a GDestroyNotify for the user_data.
(In reply to Sebastian Dröge (slomo) from comment #6) > Review of attachment 340422 [details] [review] [review]: Thanks for the review. > ::: gio/gsocket.c > @@ +4219,3 @@ > + > + /* capture the flags */ > + message->flags = msg->msg_flags; > > This seems inconsistent. Shouldn't they be just passed to the callback too? My reasoning behind this was to maintain the original API compatibility. And I still think it makes sense: flags like MSG_EOR, MSG_TRUNC can be still useful to the caller as they are related to the message data and not to the control message data. The question is whether the callback needs the flags. I tend to think it doesn't. The reason is that the processing loop is outside of the callback, so the callback gets just individual messages. If MSG_CTRUNC (which seems to be currently the only flag related to cmsgs) is set it should not be handled on the level of individual cmsgs (i.e. in the callback) but on level of the entire message (i.e. in the recmvsg caller). Does that make sense? > @@ +5622,3 @@ > + * Returns: success or failure. > + * > + */ > > Since: 2.52 Added. > ::: gio/gsocket.h > @@ +64,3 @@ > + * @user_data - user data provided to g_socket_set_cmsg_handler() is passed > via this argument > + */ > +typedef void (*GSocketCmsgCallback) (int level, int type, const gpointer > data, gsize size, gint pkt_index, gpointer user_data); > > Why not a non-void typed pointer for the data, like a const guint8 * The const makes sense, but how is the guint8 * better? I used void * since it's IMHO cleaner and doesn't need an explicit typecast. > @@ +322,3 @@ > +gboolean g_socket_set_cmsg_handler (GSocket > *socket, > + GSocketCmsgCallback cmsg_fn, > + gpointer cmsg_data); > > Should probably be called user_data to make gobject-introspection more > happy. Makes sense, renamed. > And you also want a GDestroyNotify for the user_data. I'm thinking of the following flow: * prepare user_data * register callback * call recvmsg -> calls the callback, which extracts the relevant cmsg data into user_data * unregister callback * process the message with the help of user-data * dispose user_data In this case the caller of recvmsg is responsible for unref-ing the user_data. Even though the callback can be registered once for ever, I don't see it to be very practical. Because there is no mechanism of pairing the cmsgs with the actual message.
(In reply to Petr Kulhavy from comment #7) > (In reply to Sebastian Dröge (slomo) from comment #6) > > Review of attachment 340422 [details] [review] [review] [review]: > > Thanks for the review. > > > ::: gio/gsocket.c > > @@ +4219,3 @@ > > + > > + /* capture the flags */ > > + message->flags = msg->msg_flags; > > > > This seems inconsistent. Shouldn't they be just passed to the callback too? > > My reasoning behind this was to maintain the original API compatibility. And > I still think it makes sense: flags like MSG_EOR, MSG_TRUNC can be still > useful to the caller as they are related to the message data and not to the > control message data. > > The question is whether the callback needs the flags. I tend to think it > doesn't. The reason is that the processing loop is outside of the callback, > so the callback gets just individual messages. If MSG_CTRUNC (which seems to > be currently the only flag related to cmsgs) is set it should not be handled > on the level of individual cmsgs (i.e. in the callback) but on level of the > entire message (i.e. in the recmvsg caller). Does that make sense? Makes sense indeed > > ::: gio/gsocket.h > > @@ +64,3 @@ > > + * @user_data - user data provided to g_socket_set_cmsg_handler() is passed > > via this argument > > + */ > > +typedef void (*GSocketCmsgCallback) (int level, int type, const gpointer > > data, gsize size, gint pkt_index, gpointer user_data); > > > > Why not a non-void typed pointer for the data, like a const guint8 * > > The const makes sense, but how is the guint8 * better? I used void * since > it's IMHO cleaner and doesn't need an explicit typecast. Yes you're right, these are casted to the actual control message structs from the system headers anyway so a byte array is actually worse. > > And you also want a GDestroyNotify for the user_data. > > I'm thinking of the following flow: > > * prepare user_data > * register callback > * call recvmsg > -> calls the callback, which extracts the relevant cmsg data into > user_data > * unregister callback > * process the message with the help of user-data > * dispose user_data > > In this case the caller of recvmsg is responsible for unref-ing the > user_data. > > Even though the callback can be registered once for ever, I don't see it to > be very practical. Because there is no mechanism of pairing the cmsgs with > the actual message. That's a bit awkward. It's easy to forget unsetting the callback and then at a later point it would be called again, possibly accessing invalid memory that was pointing to the stack before. Maybe it should be yet another receive function instead that takes the callback? Or maybe making it a push/pop-style API makes it more explicit that you should really pop this callback again if it happens to use anything from the stack. For extensibility I would (if not part of a new receive function) keep the destroy notify there. You never know how someone is going to use it in the future.
(In reply to Sebastian Dröge (slomo) from comment #8) > > That's a bit awkward. It's easy to forget unsetting the callback and then at > a later point it would be called again, possibly accessing invalid memory > that was pointing to the stack before. > > Maybe it should be yet another receive function instead that takes the > callback? Or maybe making it a push/pop-style API makes it more explicit > that you should really pop this callback again if it happens to use anything > from the stack. I know... But I don't like the idea of another API function too much either and in that sense agree with Dan. Another option could be that the call unsets it automatically (kind of one time use). > For extensibility I would (if not part of a new receive function) keep the > destroy notify there. You never know how someone is going to use it in the > future. That makes sense, I will add it. How is it exactly used? I mean, who calls the destroy notify when?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1192.