fix of rtp socket binding
hi, after having solved an LCR issue, i patched rtp_proxy.c the same way, because it has the same issue. 1. when next_udp_port (in rtp_socket_bind) reaches 0xfffe, the bind process will fail next time. 2. if rtp_sub_socket_bind of rtp socket succeeds and rtp_sub_socket_bind of rtcp socket fails, the rtp socket must be re-created, because it cannot not be bound again to the next higher port. 3. the port range will not wrap arround and start at 0, but at RTP_PORT_BASE. (after reaching RTP_PORT_MAX) andreas
On Mon, Aug 20, 2012 at 08:51:57AM +0200, Andreas Eversberg wrote:
hi,
Hi Andreas, my two cents below.
out_rtcp_bfd: - osmo_fd_unregister(&rs->rtcp.bfd); + if (rs->rtcp.bfd.fd > 0) { + osmo_fd_unregister(&rs->rtcp.bfd);
We had this before but I don't remember if it was with the socket code. A filedescriptor of '0' is valid. An unitiliazed bfd.fd should be set to -1.
out_rtcp_socket: - close(rs->rtcp.bfd.fd); + close(rs->rtcp.bfd.fd); + }
But more importantly... why do we need to have these extra checks? The code should only jump to the cleanup label that needs to be cleaned up. So if the rtcp.bfd has not been registered, the code should not jump to out_rtcp_bfd.
/* try to bind to a consecutive pair of ports */ - for (next_udp_port = next_udp_port % 0xffff; - next_udp_port < 0xffff; next_udp_port += 2) { + start_port = next_udp_port; + while (1) { rc = rtp_sub_socket_bind(&rs->rtp, ip, next_udp_port); if (rc != 0) - continue; + goto try_next_port;
rc = rtp_sub_socket_bind(&rs->rtcp, ip, next_udp_port+1); - if (rc == 0) + if (rc == 0) { + next_udp_port = (next_udp_port + 2 > RTP_PORT_MAX) ? + RTP_PORT_BASE : next_udp_port + 2;
code duplication, please move the calculation to a macro or inline function.
+ init_rss(&rs->rtp, rs, rc2, RTP_PRIV_RTP); + rc2 = osmo_fd_register(&rs->rtp.bfd); + if (rc2 < 0) { + close(rs->rtp.bfd.fd); + return rc2; + }
Can you think of a way to have rtp_socket_create and this somehow share the code to initialize the
+try_next_port: + next_udp_port = (next_udp_port + 2 > RTP_PORT_MAX) ? + RTP_PORT_BASE : next_udp_port + 2; + if (next_udp_port == start_port) break; + /* we must use rc2, in order to preserve rc */
Hmm, maybe I deleted too much from the patch but break is only called when rc == 0 and for all other errors you return immediately?
} if (rc < 0) { DEBUGPC(DLMUX, "failed\n");
I think this is dead code.
hi holger, i reworked the patch. Holger Hans Peter Freyther wrote:
We had this before but I don't remember if it was with the socket code. A filedescriptor of '0' is valid. An unitiliazed bfd.fd should be set to -1.
But more importantly... why do we need to have these extra checks? The code should only jump to the cleanup label that needs to be cleaned up. So if the rtcp.bfd has not been registered, the code should not jump to out_rtcp_bfd.
the checks were required, because rtp_socket_bind() might have closed a socked. i removed the checks and closed both rtp and rtcp sockets at rtp_socket_bind().
code duplication, please move the calculation to a macro or inline function.
done.
+ init_rss(&rs->rtp, rs, rc2, RTP_PRIV_RTP); + rc2 = osmo_fd_register(&rs->rtp.bfd); + if (rc2 < 0) { + close(rs->rtp.bfd.fd); + return rc2; + }
Can you think of a way to have rtp_socket_create and this somehow share the code to initialize the
alternatively the create and rtp_sub_socket_create and rtp_sub_socket_close. this makes more work, but i was lazy.
+try_next_port: + next_udp_port = (next_udp_port + 2 > RTP_PORT_MAX) ? + RTP_PORT_BASE : next_udp_port + 2; + if (next_udp_port == start_port) break; + /* we must use rc2, in order to preserve rc */
Hmm, maybe I deleted too much from the patch but break is only called when rc == 0 and for all other errors you return immediately?
} if (rc < 0) { DEBUGPC(DLMUX, "failed\n");
I think this is dead code.
The code is not dead. If bind of rtp socket failed to bind, rc is -1, else if all sockets are tried, rc is -1, because the rtcp socket failed to bind. if rc==0, both sockets are bound. regards, andreas
Hi Jolly, A couple of comments on your patch. On Wed, Aug 29, 2012 at 08:22:22AM +0200, jolly wrote:
hi holger,
i reworked the patch.
Holger Hans Peter Freyther wrote:
We had this before but I don't remember if it was with the socket code. A filedescriptor of '0' is valid. An unitiliazed bfd.fd should be set to -1.
But more importantly... why do we need to have these extra checks? The code should only jump to the cleanup label that needs to be cleaned up. So if the rtcp.bfd has not been registered, the code should not jump to out_rtcp_bfd.
the checks were required, because rtp_socket_bind() might have closed a socked. i removed the checks and closed both rtp and rtcp sockets at rtp_socket_bind().
code duplication, please move the calculation to a macro or inline function.
done.
+ init_rss(&rs->rtp, rs, rc2, RTP_PRIV_RTP); + rc2 = osmo_fd_register(&rs->rtp.bfd); + if (rc2 < 0) { + close(rs->rtp.bfd.fd); + return rc2; + }
Can you think of a way to have rtp_socket_create and this somehow share the code to initialize the
alternatively the create and rtp_sub_socket_create and rtp_sub_socket_close. this makes more work, but i was lazy.
+try_next_port: + next_udp_port = (next_udp_port + 2 > RTP_PORT_MAX) ? + RTP_PORT_BASE : next_udp_port + 2; + if (next_udp_port == start_port) break; + /* we must use rc2, in order to preserve rc */
Hmm, maybe I deleted too much from the patch but break is only called when rc == 0 and for all other errors you return immediately?
} if (rc < 0) { DEBUGPC(DLMUX, "failed\n");
I think this is dead code.
The code is not dead. If bind of rtp socket failed to bind, rc is -1, else if all sockets are tried, rc is -1, because the rtcp socket failed to bind. if rc==0, both sockets are bound.
regards,
andreas
diff --git a/openbsc/src/libtrau/rtp_proxy.c b/openbsc/src/libtrau/rtp_proxy.c index 3d34ac6..36629ae 100644 --- a/openbsc/src/libtrau/rtp_proxy.c +++ b/openbsc/src/libtrau/rtp_proxy.c @@ -555,12 +555,10 @@ struct rtp_socket *rtp_socket_create(void)
rc = rtp_socket_bind(rs, INADDR_ANY); if (rc < 0) - goto out_rtcp_bfd; + goto out_free;
return rs;
-out_rtcp_bfd: - osmo_fd_unregister(&rs->rtcp.bfd); out_rtcp_socket: close(rs->rtcp.bfd.fd); out_rtp_bfd: @@ -595,39 +593,76 @@ static int rtp_sub_socket_bind(struct rtp_sub_socket *rss, uint32_t ip, &alen); }
-#define RTP_PORT_BASE 30000 -static unsigned int next_udp_port = RTP_PORT_BASE; +#define RTP_PORT_BASE 30000 +#define RTP_PORT_MAX 39998 +#define NEXT_UDP_PORT next_udp_port = (next_udp_port + 2 > RTP_PORT_MAX) ? \ + RTP_PORT_BASE : next_udp_port + 2 +static unsigned short next_udp_port = RTP_PORT_BASE;
/* bind a RTP socket to a local address */ int rtp_socket_bind(struct rtp_socket *rs, uint32_t ip) { - int rc = -EIO; + int rc, rc2; struct in_addr ia; + unsigned short start_port;
ia.s_addr = htonl(ip); DEBUGP(DLMUX, "rtp_socket_bind(rs=%p, IP=%s): ", rs, inet_ntoa(ia));
/* try to bind to a consecutive pair of ports */ - for (next_udp_port = next_udp_port % 0xffff; - next_udp_port < 0xffff; next_udp_port += 2) { + start_port = next_udp_port; + while (1) {
Can you use 'for' instead of 'while', just for readability. The limited loop is also good in case that we forget break and loop forever (in case of bug, of course).
rc = rtp_sub_socket_bind(&rs->rtp, ip, next_udp_port); - if (rc != 0) - continue; + if (rc < 0) + goto try_next_port;
rc = rtp_sub_socket_bind(&rs->rtcp, ip, next_udp_port+1); - if (rc == 0) + if (rc == 0) { + NEXT_UDP_PORT;
Could you use a function for this instead? The use of macros in this way seems a bit evil to me. More specifically, you have to scroll up and down in the code to know what it does and the absence of parameters and return value makes this obscure.
+ next_udp_port = (next_udp_port + 2 > RTP_PORT_MAX) ? + RTP_PORT_BASE : next_udp_port + 2;
Why this call after NEXT_UDP_PORT again? Is it a leftover?
+ break; + }
+ /* reopen rtp socket and try again with next udp port */ + osmo_fd_unregister(&rs->rtp.bfd); + close(rs->rtp.bfd.fd); + rs->rtp.bfd.fd = -1;
Suggestion: from here below
+ rc2 = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); + if (rc2 < 0) { + rc = rc2; + goto out_rtp; + } + init_rss(&rs->rtp, rs, rc2, RTP_PRIV_RTP); + rc2 = osmo_fd_register(&rs->rtp.bfd); + if (rc2 < 0) { + close(rs->rtp.bfd.fd); + rc = rc2; + goto out_rtp; + }
To here, I'd move this code to a function.
+try_next_port: + NEXT_UDP_PORT; + if (next_udp_port == start_port) break; + /* we must use rc2, in order to preserve rc */ } if (rc < 0) { DEBUGPC(DLMUX, "failed\n"); - return rc; + goto out_rtcp; }
ia.s_addr = rs->rtp.sin_local.sin_addr.s_addr; DEBUGPC(DLMUX, "BOUND_IP=%s, BOUND_PORT=%u\n", inet_ntoa(ia), ntohs(rs->rtp.sin_local.sin_port)); return ntohs(rs->rtp.sin_local.sin_port); + +out_rtcp: + osmo_fd_unregister(&rs->rtcp.bfd); + close(rs->rtcp.bfd.fd); +out_rtp: + osmo_fd_unregister(&rs->rtp.bfd); + close(rs->rtp.bfd.fd); + return rc; }
static int rtp_sub_socket_connect(struct rtp_sub_socket *rss,
participants (4)
-
Andreas Eversberg -
Holger Hans Peter Freyther -
jolly
-
Pablo Neira Ayuso