fix of rtp socket binding

[Holger Hans Peter Freyther]

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.