fix of rtp socket binding

[jolly]

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rtp-bind-fix2.patch
URL: <http://lists.osmocom.org/pipermail/openbsc/attachments/20120829/6272e630/attachment.ksh>