GNOME Bugzilla – Bug 623187
provide some support for arbitrary setsockopt()s?
Last modified: 2012-12-12 14:22:20 UTC
Hi all, there is a problem sending broadcast messages using GSocket, because I need to set SO_BROADCAST flag in setsockopt() function (sys/socket.h) but it's lower level than GIO API, and GIO must allow to send broadcast packets via the API. This is an example code I made as an example: #include <stdio.h> #include <glib-object.h> #include <gio/gio.h> #include <sys/socket.h> int main(int argc, char *argv[]) { GSocket *socket; GSocketAddress *address; GInetAddress *inetaddress; GString *string; g_type_init(); socket = g_socket_new(G_SOCKET_FAMILY_IPV4, G_SOCKET_TYPE_DATAGRAM, G_SOCKET_PROTOCOL_UDP, NULL); if(socket == NULL) { printf("Could not create socket.\n"); return 0; } inetaddress = g_inet_address_new_from_string("255.255.255.255"); if(inetaddress == NULL) { printf("Could not create internet address.\n"); return 0; } address = g_inet_socket_address_new(inetaddress, 4000); string = g_string_new(NULL); g_string_printf(string, "This is a test"); GError *error = NULL; GOutputVector vector; vector.buffer = string->str; vector.size = string->len; //Some hack!!! //======================================== int fd = g_socket_get_fd(socket); int value = 1; if(setsockopt(fd, SOL_SOCKET, SO_BROADCAST, &value, sizeof(value)) != 0) { printf("Error making the hack!!\n"); return 0; } //======================================== //CANT SEND PACKET TO BROADCAST ADDRESS (WITHOUT THE HACK) :S if(g_socket_send_message(socket, address, &vector, 1, NULL, 0, 0, NULL, &error) == -1) { printf("Error sending packet: %s\n", error->message); return 0; } g_string_free(string, TRUE); g_object_unref(socket); return 0; } When I don't use the hack code the application throws this: "Error sending packet: Error sending message: Permission denied" Is very "unclean" to do that fix on every program I do and the idea I have is to modify the GSocketPrivate structure inside the GSocket structure and add an "gint broadcast" and make this functions and add to the GIO Socket API: - g_socket_set_broadcast(GSocket *socket, gboolean broadcast); - gboolean g_socket_get_broadcast(GSocket *socket); This problem is for making a patch to the GIO library. I can fix the code if you like, but answer me if you like to fix this. Thanks, Steven.
hi Steven, This seems like an appropriate feature. You should prepare a patch against the current version of glib in git. I recommend looking at the existing g_socket_{get,set}_keepalive functions and copying what they do (ie: add g_socket_{get,set}_broadcast). Please add the appropriate documentation comments to the function, too. Be sure to include the "Since: 2.26" tag at the bottom. Also, make sure you add your new functions to the appropriate places in gio/gio.symbols and docs/reference/gio/gio-sections.txt. After all of that, please post the patch here and we will review it. Thanks for the offer :)
Created attachment 164943 [details] [review] Added support for sending broadcast messages in the Socket Layer (GSockets) I made a patch adding the new functions of the GLIB to support broadcasting ;) Thanks for all, Steven.
Review of attachment 164943 [details] [review]: other than one tiny problem, this looks pretty good. i'd add a little more detail for the documentation in the _set() call. maybe mention that this is only meaningful with datagram-type sockets or something (is that true?)... i'll let dan have the final say on this one, though. ::: gio/gsocket.c @@ +1025,3 @@ + * g_socket_set_broadcast: + * @socket: a #GSocket. + * @keepalive: Value for the broadcast flag @keepalive -> @broadcast here, i guess
Created attachment 164945 [details] [review] Added support for sending broadcast messages in the Socket Layer (GSockets) Replace over the patch I posted before.
(In reply to comment #3) > Review of attachment 164943 [details] [review]: > > other than one tiny problem, this looks pretty good. > > i'd add a little more detail for the documentation in the _set() call. maybe > mention that this is only meaningful with datagram-type sockets or something > (is that true?)... > > i'll let dan have the final say on this one, though. > > ::: gio/gsocket.c > @@ +1025,3 @@ > + * g_socket_set_broadcast: > + * @socket: a #GSocket. > + * @keepalive: Value for the broadcast flag > > @keepalive -> @broadcast here, i guess I posted a new patch because broadcasting is for IPv4 only. Broadcasting is not only for the UDP protocol I guess... Thanks for the correction, Steven.
my feeling on this is that SO_BROADCAST is a very "fiddly" option. It is only meaningful for IPv4 UDP sockets, and there's no way to hide that or try to include it as part of a more abstract interface that also covers other things. And then once we add SO_BROADCAST, someone is going to need to set SO_LINGER, or SO_RCVLOWAT, or IP_OPTIONS, etc, etc. So I think we should either (a) say the code above using g_socket_get_fd() is not a hack, it's just the way you're supposed to do it, or (b) add g_socket_get/set_option() (using GValue or GVariant rather than varargs).
(In reply to comment #6) > my feeling on this is that SO_BROADCAST is a very "fiddly" option. It is only > meaningful for IPv4 UDP sockets, and there's no way to hide that or try to > include it as part of a more abstract interface that also covers other things. > > And then once we add SO_BROADCAST, someone is going to need to set SO_LINGER, > or SO_RCVLOWAT, or IP_OPTIONS, etc, etc. > > So I think we should either (a) say the code above using g_socket_get_fd() is > not a hack, it's just the way you're supposed to do it, or (b) add > g_socket_get/set_option() (using GValue or GVariant rather than varargs). Ok, we can do a: gboolean g_socket_set_option(GSocket *socket, GSocketOption option, gpointer value); and a: gpointer g_socket_get_option(GSocket *socket, GSocketOption option); GSocketOption is an enum containing the setsockopt possible flags. I can implement it ;) Di you like this? Option b is the best, if you like I can make a patch for adding this 2 new functions. Thanks, Steven.
The problem with the gpointer way is that it's not usable by language bindings (python, javascript, etc). That's why I suggested using GValue or GVariant. Either of those would let you make it type safe while still allowing values of arbitrary types. So, eg gboolean g_socket_set_option_with_variant (GSocket *socket, int level, int optname, GVariant value); And it ought to be pretty easy (as I understand it) to add a C convenience wrapper like: gboolean g_socket_set_option (GSocket *socket, int level, int optname, const char *signature, ...); and then you'd call it like g_socket_set_option (sock, SOL_SOCKET, SO_BROADCAST, "i", 1); or g_socket_set_option (sock, SOL_SOCKET, SO_LINGER, "(ii)", 1, 3); (except I probably got the signature syntax wrong on that second one, but you get the idea). (And then we'd add a "Rename-to: g_socket_set_option" annotation to g_socket_set_option_with_variant, and non-C languages ought to do the right thing with it, I think.) Anyway, Ryan is the gvariant guy, so he should have some comment on the feasability of this and pointers to documentation. (In particular, can we make the "automatically convert to/from struct linger" thing work in the second example above?) Oh, the other thing is that I recommend we keep the "int level, int optname" from setsockopt() rather than trying to wrap them into a single enum, because people might want to use non-SOL_SOCKET sockopts (like the IPPROTO_IP ones), and because people might want to use arbitrary new/non-portable sockopts and we don't want to have to add a new enum value for each one. I realize this is quite a bit more complicated than your original patch... :-}
looks just about right to me except that i'd call "signature" "format_string". ...it's very strange to see GVariant abused for these sorts of uses... ...but good, I think :)
(In reply to comment #9) > looks just about right to me except that i'd call "signature" "format_string". > > ...it's very strange to see GVariant abused for these sorts of uses... > > ...but good, I think :) So the funtcions to implement are: gboolean g_socket_set_option(GSocket *socket, gint level, gint optname, GVariant value); and: GValue g_socket_get_option(GSocket *socket, gint level, gint optname); Are these methods right? Another thing... I watched in the source code of SDL_net the broadcast "hack" for adding broadcast function in an UDP socket: This is the piece of code I found: #ifdef SO_BROADCAST /* Allow LAN broadcasts with the socket */ { int yes = 1; setsockopt(sock->channel, SOL_SOCKET, SO_BROADCAST, (char*)&yes, sizeof(yes)); } #endif This code is in the socket creation function... But already... If we need to use the g_socket_get/set functions we must add in our program header this: #ifdef G_OS_WIN32 #include <winsock2.h> #else #include <sys/socket.h> #endif And I think is some "embarrasing" to put platform specific code in a GLIB-based application... :S Tell me what do you think about this ;) Thanks, Steven.
hi Dan, (In reply to comment #8) > Oh, the other thing is that I recommend we keep the "int level, int optname" > from setsockopt() rather than trying to wrap them into a single enum, because > people might want to use non-SOL_SOCKET sockopts (like the IPPROTO_IP ones), > and because people might want to use arbitrary new/non-portable sockopts and we > don't want to have to add a new enum value for each one. We won't need to add an enum value but probably we will need to have some knowledge of how to map the GVariant (or GValue) for it into the type that the system call is expecting to see (hopefully including support for type-checking). How do you propose to deal with that? This is particularly difficult for the getsockopt case where we don't start out with a value but, rather, have to construct one from the system call result.
(In reply to comment #11) > We won't need to add an enum value but probably we will need to have some > knowledge of how to map the GVariant (or GValue) for it into the type that the > system call is expecting to see (hopefully including support for > type-checking). How do you propose to deal with that? well, I was proposing to NOT do type checking, essentially, just like setsockopt. Assume that our caller got the type right, serialize the passed-in data, and then pass it to setsockopt (and the inverse for getsockopt). But looking around a little more, there are lots of sockopts with complicated struct types containing typedef'ed integer types of uncertain sizes, so really there's no way this can possibly work. So now I'm back to leaning towards "just use g_socket_get_fd() and setsockopt()". An intermediate possibility would be to have #define g_socket_set_option(sock, level, optname, optval, optlen, error) ... that calls g_socket_get_fd() and setsockopt(), and if setsockopt() returns -1, it translates the error into a GError. I'm not sure that would be useful though; you might need to see the precise errno value to figure out what went wrong in some cases. (In reply to comment #10) > If we need to > use the g_socket_get/set functions we must add in our program header this: > > #ifdef G_OS_WIN32 > #include <winsock2.h> > #else > #include <sys/socket.h> > #endif > > And I think is some "embarrasing" to put platform specific code in a GLIB-based > application... :S Yes. Internally we have gnetworkingprivate.h to solve that problem. We should either provide a public equivalent of that, or perhaps just move the platform-specific networking includes into gsocket.h.
*** Bug 612926 has been marked as a duplicate of this bug. ***
Created attachment 176183 [details] [review] Add gnetworking.h OK, this adds a public gnetworking.h header, so that you can at least It doesn't make a whole lot of sense to make g_socket_set_option, etc, since the error codes would be incompatible, etc Does this work? And is this the right way to generate gnetworking.h?
Review of attachment 176183 [details] [review]: ::: gio/gnetworking.h.in @@ +41,3 @@ +#include <sys/types.h> +#include <sys/socket.h> +#undef __USE_GNU I'd be careful with this; __USE_GNU is considered glibc private I believe. It's tricky but you probably have to detect whether _GNU_SOURCE is defined, and if it isn't, define it. To safely undefine it, you'd need to define a different define...here let me just write it =) #ifdef __linux__ #ifndef _GNU_SOURCE #define __G_TEMPORARY_GNU_SOURCE #define _GNU_SOURCE #endif #include <sys/types.h> #include <sys/socket.h> #ifdef __G_TEMPORARY_GNU_SOURCE #undef __GNU_SOURCE #endif #endif
(In reply to comment #15) > I'd be careful with this; __USE_GNU is considered glibc private I believe. > > It's tricky but you probably have to detect whether _GNU_SOURCE is defined, and > if it isn't, define it. The problem isn't that we wanted to avoid polluting the user's defines, it's that you have to define _GNU_SOURCE before including any other header, because the _GNU_SOURCE -> __USE_GNU conversion only happens in <features.h>, which is protected by #ifndef _FEATURES_H, so once you include *any* header without defining _GNU_SOURCE, defining it later has no effect. However, even with this patch there's still a problem; if you've already included <sys/types> without __USE_GNU, then _SYS_TYPES_H will be defined and it won't include it again, blah blah blah. So really, we should just require gnetworking.h #includers to set _GNU_SOURCE themselves if they need it.
Created attachment 176194 [details] [review] Add gnetworking.h Install a public "gnetworking.h" header that can be used to include the relevant OS-dependent networking headers. This does not really abstract away unix-vs-windows however; error codes, in particular, are incompatible. Use gnetworking.h as appropriate within gio, use _GNU_SOURCE correctly instead of cheating with __USE_GNU, and remove some unnecessary _GNU_SOURCE defines.
commit ffb5f8b10191ddf51ccd021c1e4dbba4eafbc370 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Jan 13 13:01:35 2012 +0100 GSocket: Add function to set/get the broadcast setting on a socket https://bugzilla.gnome.org/show_bug.cgi?id=623187 What should happen with the patch attached to this bug?
retitling since this is no longer really about SO_BROADCAST
Created attachment 228976 [details] [review] gio: move resolver utils from gresolver.c to gthreadedresolver.c Since there is only one resolver implementation now, we can move the resolver utility functions from gresolver.c into gthreadedresolver.c, and remove the prototypes from gnetworkingprivate.h.
Created attachment 228977 [details] [review] Add gnetworking.h Install a public "gnetworking.h" header that can be used to include the relevant OS-dependent networking headers. This does not really abstract away unix-vs-windows however; error codes, in particular, are incompatible. gnetworkingprivate.h now contains just a few internal URI-related functions Also add a g_networking_init() function to gnetworking.h, which can be used to explicitly initialize OS-level networking, rather than having that happen as a side-effect of registering GInetAddress.
Created attachment 228978 [details] [review] gsocket: add getsockopt/setsockopt wrappers Add g_socket_get_option() and g_socket_set_option(), wrapping getsockopt/setsockopt for the case of integer-valued options. Update code to use these instead of the underlying calls.
*** Bug 687981 has been marked as a duplicate of this bug. ***
*** Bug 667858 has been marked as a duplicate of this bug. ***
pushed with a minor win32 compile fix Attachment 228976 [details] pushed as 9e90575 - gio: move resolver utils from gresolver.c to gthreadedresolver.c Attachment 228977 [details] pushed as b377e69 - Add gnetworking.h Attachment 228978 [details] pushed as 211ed17 - gsocket: add getsockopt/setsockopt wrappers