Change in osmo-msc[master]: libmsc: Allow different channel types to be requested as silent calls
gerrit-no-reply at lists.osmocom.org
Wed Mar 13 20:13:51 UTC 2019
tnt has posted comments on this change. ( https://gerrit.osmocom.org/13242 )
Change subject: libmsc: Allow different channel types to be requested as silent calls
Patch Set 1:
Tx for the review, will fix that up.
PS1, Line 10: Signed-off-by: Sylvain Munaut <tnt at 246tNt.com>
> No signed-off-by in osmocom repos.
PS1, Line 1072: uint16_t port = 0;
> no initialization needed here afaiu.
PS1, Line 40: char traffic_ip[INET_ADDRSTRLEN];
> worth using INET6_ADDRSTRLEN here in case we want to support ipv6? Not sure if it makes sense in thi […]
Everywhere else in the code we use INET_ADDRSTRLEN. So I'd rather keep it consistent. If at some point we want ipv6 the grep will turn up this code and hopefully it will be reviewed for ipv6 compat along with the rest. Using INET6_ADDRSTRLEN I think might make it look like it was tested or is v6 compatible which ... it's probably not at all and would be untested because the rest isn't v6 compatible.
PS1, Line 91: osmo_timer_schedule(&scd->timer, 0, 0);
> Why not calling timer_cb directly around line 120? You are immediatelly firing this cb, and only on […]
Because at this point in the code, I'm in the callback of a state machine which isn't in the right state to accept ran_conn_communicating() ... it will only be in the right state when the rest of the processing of the callback list is done. And I can't change the order of the callbacks because ... well the entire rest of the code base expects stuff to go in that exact order.
I tried like 4 different approaches to solve this and this is the lest ugly/most self contained approach.
PS1, Line 99: } rtp_addr;
> Is this conversion from sockaddr_in to sockaddr_storage through union valid? Never seen it before.
Why wouldn't it be ?
sockaddr_storage is just 'something big enough'. unions members are guaranteed to start at the beginning ... so I don't see an issue here.
PS1, Line 107: gsm0808_speech_codec_from_chan_type(&scl.codec[i], scd->ct.perm_spch[i]);
> Would it make sense to re-use enc_speech_codec_list() from a_iface. […]
I'm not convinced ... it literally just saves 1 for loop, the bulk of the work is done by gsm0808_speech_codec_from_chan_type. If people insist I can do it, but I'm not even sure where to put/declare it ... I guess it would belong to be in libosmocore in gsm0808_utils thus introducing a cross repo version dependency, seems a bit much.
PS1, Line 120: break;
> Would be clearer using a return 0 here, otherwise it's confusing scheduling scd timer then having a […]
PS1, Line 187: struct gsm0808_channel_type *ct,
> ct is not changed in this function - why not make it const similar to traffic_dst_ip?
To view, visit https://gerrit.osmocom.org/13242
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Owner: tnt <tnt at 246tNt.com>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: tnt <tnt at 246tNt.com>
Gerrit-CC: Max <msuraev at sysmocom.de>
Gerrit-CC: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 13 Mar 2019 20:13:51 +0000
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the gerrit-log