[erlang-questions] prim_inet:accept() bug with {fd, N} socket option?

[Per Hedeland]

Serge Aleynikov <saleyn@REDACTED> wrote:
>
>gen_tcp:connect/2, gen_tcp:listen/2 functions expose an option {fd, 
>FileDescriptor} that allows to take an elsewhere opened file descriptor 
>and make it usable in the gen_tcp module.  This is a very convenient 
>option, however, what I discovered was that the default options that are 
>being set on a socket by prim_inet:accept_opts/2 call involved in 
>gen_tcp:accept/1 try to get/set a 'priority' option that may not be 
>supported

Well, it's rather dubious to pass in a non-inet socket to the inet
modules/driver, but anyway the problem you're running into isn't really
that 'priority' isn't supported (SO_PRIORITY is a Linux-specific thing
AFAIK, but it's a socket-level option and so works for any type of
socket), but rather that the driver does some pretty ugly things in an
attempt to make 'priority' and 'tos' appear to be independent, even
though they aren't at the OS level (it's a mistake to do this I think,
but let's ignore that for the moment).

This has the effect that it ends up trying to set the IP_TOS option on
your unix-domain socket, which doesn't work of course, and then it gives
up on the whole thing even though it was actually SO_PRIORITY that was
requested (from the prim_inet code you quoted).

Below is a patch that addresses this - it's against R10B-10 though, and
won't apply against current versions since even though the code is
essentially the same there, it has moved around a bit. But maybe you can
convert it - or just comment out the whole setopt_prio_tos_trick() thing
there.:-)

I have subsequently hacked prim_inet and inet_drv to handle arbitrary
non-inet sockets "properly", i.e. allow for passing in 'unspec' (as in
AF_UNSPEC) as the address family, and in that case refrain from doing
*any* AF_INET/AF_INET6-specific get/setsockopts, as well as
getsockname() and getpeername() (since the address format is unknown).
It's perhaps still a bit dubious to "abuse" the inet code this way
though, but you sure get a lot of stuff for free that way...

--Per Hedeland

Index: otp/erts/emulator/drivers/common/inet_drv.c
===================================================================
--- otp/erts/emulator/drivers/common/inet_drv.c	(revision 11053)
+++ otp/erts/emulator/drivers/common/inet_drv.c	(revision 11062)
@@ -3856,6 +3856,15 @@
 	default:
 	    return -1;
 	}
+
+/* Per H @ Tail-f: The original code here had problems that possibly
+   only occur if you abuse it for non-INET sockets, but anyway:
+   a) If the getsockopt for SO_PRIORITY or IP_TOS failed, the actual
+      requested setsockopt was never even attempted.
+   b) If {get,set}sockopt for one of IP_TOS and SO_PRIORITY failed,
+      but ditto for the other worked and that was actually the requested
+      option, failure was still reported to erlang.                  */
+
 #if  defined(IP_TOS) && defined(SOL_IP) && defined(SO_PRIORITY)
 	{
 	    /* The relations between SO_PRIORITY, TOS and other options
@@ -3868,34 +3877,36 @@
 	       user feeling socket options are independent. /PaN */
 	    int tmp_ival_prio;
 	    int tmp_arg_sz_prio = sizeof(tmp_ival_prio);
+	    int res_prio;
 	    int tmp_ival_tos;
 	    int tmp_arg_sz_tos = sizeof(tmp_ival_tos);
+	    int res_tos;
 
-	    res = sock_getopt(desc->s, SOL_SOCKET, SO_PRIORITY,
-			      (char *) &tmp_ival_prio, &tmp_arg_sz_prio);
+	    res_prio = sock_getopt(desc->s, SOL_SOCKET, SO_PRIORITY,
+				   (char *) &tmp_ival_prio, &tmp_arg_sz_prio);
+	    res_tos = sock_getopt(desc->s, SOL_IP, IP_TOS, 
+				  (char *) &tmp_ival_tos, &tmp_arg_sz_tos);
+	    res = sock_setopt(desc->s, proto, type, arg_ptr, arg_sz);
 	    if (res == 0) {
-		res = sock_getopt(desc->s, SOL_IP, IP_TOS, 
-			      (char *) &tmp_ival_tos, &tmp_arg_sz_tos);
-		if (res == 0) {
-		    res = sock_setopt(desc->s, proto, type, arg_ptr, arg_sz);
-		    if (res == 0) {
-			if (type != SO_PRIORITY) {
-			    if (type != IP_TOS) {
-				res = sock_setopt(desc->s, 
-						  SOL_IP, 
-						  IP_TOS,
-						  (char *) &tmp_ival_tos, 
-						  tmp_arg_sz_tos);
-			    }
-			    if (res == 0) {
-				res =  sock_setopt(desc->s, 
-						   SOL_SOCKET, 
-						   SO_PRIORITY,
-						   (char *) &tmp_ival_prio, 
-						   tmp_arg_sz_prio);
-			    }
-			}
+		if (type != SO_PRIORITY) {
+		    if (type != IP_TOS && res_tos == 0) {
+			res_tos = sock_setopt(desc->s, 
+					      SOL_IP, 
+					      IP_TOS,
+					      (char *) &tmp_ival_tos, 
+					      tmp_arg_sz_tos);
+			if (propagate)
+			    res = res_tos;
 		    }
+		    if (res == 0 && res_prio == 0) {
+			res_prio = sock_setopt(desc->s, 
+					       SOL_SOCKET, 
+					       SO_PRIORITY,
+					       (char *) &tmp_ival_prio, 
+					       tmp_arg_sz_prio);
+			if (propagate)
+			    res = res_prio;
+		    }
 		}
 	    }
 	}