20 Aug
2012
20 Aug
'12
9:51 a.m.
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
22 Aug
22 Aug
12:04 p.m.
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.
29 Aug
29 Aug
9:22 a.m.
hi holger,
i reworked the patch.
Holger Hans Peter Freyther wrote:
alternatively the create and rtp_sub_socket_create and
rtp_sub_socket_close. this makes more work, but i was lazy.
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
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
+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.
30 Aug
30 Aug
10:45 p.m.
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:
alternatively the create and rtp_sub_socket_create and
rtp_sub_socket_close. this makes more work, but i was lazy.
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
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
+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.
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,
4294
days inactive
4304
days old
3 comments
4 participants
participants (4)
-
Andreas Eversberg -
Holger Hans Peter Freyther -
jolly
-
Pablo Neira Ayuso