This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Pablo Neira Ayuso pablo at gnumonks.orgHi 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,