Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add net/setsockopt #1138

Merged
merged 4 commits into from
May 17, 2023
Merged

add net/setsockopt #1138

merged 4 commits into from
May 17, 2023

Conversation

zevv
Copy link
Contributor

@zevv zevv commented May 15, 2023

This PR adds the net/setsockopt function to src/core/net.c; this allows a number of common socket options to be configured to support broadcast and multicast traffic on sockets. Additional options can later be added when needed.

Example for receiving mdns packets:

(def sock (net/listen "0.0.0.0" "5353" :datagram))
(net/setsockopt sock :so_broadcast true)
(net/setsockopt sock :ip_add_membership "224.0.0.251")
(net/setsockopt sock :ip_multicast_ttl 255)
(pp (net/read sock 512))

What is the appropriate error behavior for:

  • Passing wrong options (currently panics)
  • the setsockopt() call failing (currently panics)

@sogaiu
Copy link
Contributor

sogaiu commented May 15, 2023

Looks neat!

Sorry to only have a minor comment here -- would you mind doing make format?

FWIW, the following is the diff I get currently when I do that:

diff --git a/src/core/net.c b/src/core/net.c
index 8c911f92..ac97554a 100644
--- a/src/core/net.c
+++ b/src/core/net.c
@@ -894,32 +894,32 @@ static const struct sockopt_type sockopt_type_list[] = {
 };
 
 JANET_CORE_FN(cfun_net_setsockopt,
-             "(net/setsockopt stream option value)",
-             "set socket options.\n"
-             "\n"
-             "supported options and associated value types:\n"
-             "- :SO_BROADCAST boolean\n"
-             "- :SO_REUSEADDR boolean\n"
-             "- :SO_KEEPALIVE boolean\n"
-             "- :IP_MULTICAST_TTL number\n"
-             "- :IP_ADD_MEMBERSHIP string\n"
-             "- :IP_DROP_MEMBERSHIP string\n"
-             "- :IPV6_JOIN_GROUP string\n"
-             "- :IPV6_LEAVE_GROUP string\n") {
+              "(net/setsockopt stream option value)",
+              "set socket options.\n"
+              "\n"
+              "supported options and associated value types:\n"
+              "- :SO_BROADCAST boolean\n"
+              "- :SO_REUSEADDR boolean\n"
+              "- :SO_KEEPALIVE boolean\n"
+              "- :IP_MULTICAST_TTL number\n"
+              "- :IP_ADD_MEMBERSHIP string\n"
+              "- :IP_DROP_MEMBERSHIP string\n"
+              "- :IPV6_JOIN_GROUP string\n"
+              "- :IPV6_LEAVE_GROUP string\n") {
     janet_arity(argc, 3, 3);
     JanetStream *stream = janet_getabstract(argv, 0, &janet_stream_type);
     janet_stream_flags(stream, JANET_STREAM_SOCKET);
     JanetKeyword optstr = janet_getkeyword(argv, 1);
 
     const struct sockopt_type *st = sockopt_type_list;
-    while(st->name) {
+    while (st->name) {
         if (janet_cstrcmp(optstr, st->name) == 0) {
             break;
         }
         st++;
     }
 
-    if(st->name == NULL) {
+    if (st->name == NULL) {
         janet_panicf("unknown socket option %q", argv[1]);
     }
 
@@ -932,19 +932,19 @@ JANET_CORE_FN(cfun_net_setsockopt,
     void *optval = (void *)&val;
     socklen_t optlen = 0;
 
-    if(st->type == JANET_BOOLEAN) {
+    if (st->type == JANET_BOOLEAN) {
         val.v_int = janet_getboolean(argv, 2);
         optlen = sizeof(val.v_int);
-    } else if(st->type == JANET_NUMBER) {
+    } else if (st->type == JANET_NUMBER) {
         val.v_int = janet_getinteger(argv, 2);
         optlen = sizeof(val.v_int);
-    } else if(st->optname == IP_ADD_MEMBERSHIP || st->optname == IP_DROP_MEMBERSHIP) {
+    } else if (st->optname == IP_ADD_MEMBERSHIP || st->optname == IP_DROP_MEMBERSHIP) {
         const char *addr = janet_getcstring(argv, 2);
         memset(&val.v_mreq, 0, sizeof val.v_mreq);
         val.v_mreq.imr_interface.s_addr = htonl(INADDR_ANY);
         val.v_mreq.imr_multiaddr.s_addr = inet_addr(addr);
         optlen = sizeof(val.v_mreq);
-    } else if(st->optname == IPV6_JOIN_GROUP || st->optname == IPV6_LEAVE_GROUP) {
+    } else if (st->optname == IPV6_JOIN_GROUP || st->optname == IPV6_LEAVE_GROUP) {
         const char *addr = janet_getcstring(argv, 2);
         memset(&val.v_mreq6, 0, sizeof val.v_mreq6);
         val.v_mreq6.ipv6mr_interface = 0;
@@ -957,7 +957,7 @@ JANET_CORE_FN(cfun_net_setsockopt,
     janet_assert(optlen != 0, "invalid socket option value");
 
     int r = setsockopt((JSock) stream->handle, st->level, st->optname, optval, optlen);
-    if(r == -1) {
+    if (r == -1) {
         janet_panicf("setsockopt(%q): %s", argv[1], strerror(errno));
     }
 

@zevv
Copy link
Contributor Author

zevv commented May 15, 2023

Formatting applied; seems that MacOS failed, i'll look into that.

@zevv
Copy link
Contributor Author

zevv commented May 15, 2023

Yeah, this is hard without having a mac at hand; missing some #include or magic _XOPEN_SOURCE define I guess, sigh...

@sogaiu
Copy link
Contributor

sogaiu commented May 15, 2023

No clue whether these might help, but they refer to some of the names mentioned as "undeclared identifier" in the error output:

Chromium source had some bit that seemed related at the top of this file.

@tionis
Copy link
Contributor

tionis commented May 15, 2023

I think whenever keywords are used as arguments (or parts or arguments) in the stdlib they are lowercase, so perhaps this would also make sense here for consistency?

@zevv
Copy link
Contributor Author

zevv commented May 15, 2023

I think whenever keywords are used as arguments (or parts or arguments) in the stdlib they are lowercase, so perhaps this would also make sense here for consistency?

yeah, good point. I don't really have an opinion about this; upper case seemed the natural choice because of the underlying constants, but lower case is more natural to Janet. I'd be happy to change this.

@zevv
Copy link
Contributor Author

zevv commented May 15, 2023

Indeed, magic define _DARWIN_C_SOURCE was missing, thanks @disruptek

@bakpakin
Copy link
Member

Cool, multicast was something I wanted to experiment with. Thanks for the work, @zevv and the review everyone else!

LGTM

@bakpakin bakpakin merged commit 692b6ef into janet-lang:master May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants