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) {
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;
+ next_udp_port = (next_udp_port + 2 > RTP_PORT_MAX) ?
+ RTP_PORT_BASE : next_udp_port + 2;
+ 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;
+ 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;
+ }
+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,
