<p style="white-space: pre-wrap; word-wrap: break-word;">Tx for the review, will fix that up.</p><p><a href="https://gerrit.osmocom.org/13242">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13242/1//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1//COMMIT_MSG@10">Patch Set #1, Line 10:</a> <code style="font-family:monospace,monospace">Signed-off-by: Sylvain Munaut <tnt@246tNt.com></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">No signed-off-by in osmocom repos.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/msc_vty.c">File src/libmsc/msc_vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/msc_vty.c@1072">Patch Set #1, Line 1072:</a> <code style="font-family:monospace,monospace"> uint16_t port = 0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">no initialization needed here afaiu.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/silent_call.c">File src/libmsc/silent_call.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/silent_call.c@40">Patch Set #1, Line 40:</a> <code style="font-family:monospace,monospace"> char traffic_ip[INET_ADDRSTRLEN];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">worth using INET6_ADDRSTRLEN here in case we want to support ipv6? Not sure if it makes sense in thi […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/silent_call.c@91">Patch Set #1, Line 91:</a> <code style="font-family:monospace,monospace"> osmo_timer_schedule(&scd->timer, 0, 0);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why not calling timer_cb directly around line 120? You are immediatelly firing this cb, and only on […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I tried like 4 different approaches to solve this and this is the lest ugly/most self contained approach.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/silent_call.c@99">Patch Set #1, Line 99:</a> <code style="font-family:monospace,monospace"> } rtp_addr;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is this conversion from sockaddr_in to sockaddr_storage through union valid? Never seen it before.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why wouldn't it be ?</p><p style="white-space: pre-wrap; word-wrap: break-word;">sockaddr_storage is just 'something big enough'. unions members are guaranteed to start at the beginning ... so I don't see an issue here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/silent_call.c@107">Patch Set #1, Line 107:</a> <code style="font-family:monospace,monospace"> gsm0808_speech_codec_from_chan_type(&scl.codec[i], scd->ct.perm_spch[i]);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Would it make sense to re-use enc_speech_codec_list() from a_iface. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/silent_call.c@120">Patch Set #1, Line 120:</a> <code style="font-family:monospace,monospace"> break;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Would be clearer using a return 0 here, otherwise it's confusing scheduling scd timer then having a […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13242/1/src/libmsc/silent_call.c@187">Patch Set #1, Line 187:</a> <code style="font-family:monospace,monospace"> struct gsm0808_channel_type *ct,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ct is not changed in this function - why not make it const similar to traffic_dst_ip?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/13242">change 13242</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/13242"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I82645708dd27864cf33ea9cc993ead0983415602 </div>
<div style="display:none"> Gerrit-Change-Number: 13242 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: tnt <tnt@246tNt.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: tnt <tnt@246tNt.com> </div>
<div style="display:none"> Gerrit-CC: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 13 Mar 2019 20:13:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>