<p>Patch set 9:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/13137">View Change</a></p><p>22 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_bsc_ho.msc">File doc/sequence_charts/inter_bsc_ho.msc:</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/13137/9/doc/sequence_charts/inter_bsc_ho.msc@27">Patch Set #9, Line 27:</a> <code style="font-family:monospace,monospace">     </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_msc_ho.msc">File doc/sequence_charts/inter_msc_ho.msc:</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/13137/9/doc/sequence_charts/inter_msc_ho.msc@47">Patch Set #9, Line 47:</a> <code style="font-family:monospace,monospace">      </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/doc/sequence_charts/inter_msc_ho.msc@74">Patch Set #9, Line 74:</a> <code style="font-family:monospace,monospace">      </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/9/include/osmocom/msc/Makefile.am">File include/osmocom/msc/Makefile.am:</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/13137/9/include/osmocom/msc/Makefile.am@9">Patch Set #9, Line 9:</a> <code style="font-family:monospace,monospace">gsm_04_11.h \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like a meaningless change.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c">File src/libmsc/call_leg.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/13137/9/src/libmsc/call_leg.c@34">Patch Set #9, Line 34:</a> <code style="font-family:monospace,monospace">LOG_CALL_LEG</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">LOGPFSML can handle fi=NULL, so we probably don't need this macro.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@78">Patch Set #9, Line 78:</a> <code style="font-family:monospace,monospace">fi</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">AFAICS, you're using 'fi' as the talloc-parent in order to deallocate it automatically when the 'fi' is terminated and freed. If it's the only reason, then I don't like this approach. You could just call talloc_free(fi->priv) in the FSM's cleanup() handler. Thus there will be no need to init dummy call_leg_fsm on line #59.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@78">Patch Set #9, Line 78:</a> <code style="font-family:monospace,monospace">talloc_zero</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since you're doing *cl = (struct call_leg) { ... } below, zero-initialization doesn't make sense I think.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@101">Patch Set #9, Line 101:</a> <code style="font-family:monospace,monospace">cl->fi</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like a bug to me. We need to change the parent talloc context of 'cl' itself, right? Here we keeping 'cl' as a child of the old 'osmo_fsm_inst', and changing the talloc-parent of 'new_parent_fi' to 'new_parent_fi' o_O.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@142">Patch Set #9, Line 142:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@145">Patch Set #9, Line 145:</a> <code style="font-family:monospace,monospace">for (i = 0; i < ARRAY_SIZE(cl->rtp); i++) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So here we check whether all RTP connections of this call leg are established, right? Would be great to have a comment.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, is it possible to have more than 2 RTP connections for a call leg? If no, this code can be simplified:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if (!rtp_stream_is_established(cl->rtp[0]))<br>    break;<br>  if (!rtp_stream_is_established(cl->rtp[1]))<br>    break;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">And should we print some debug / notice message if not all RTP connections are established? Currently we silently suppress CALL_LEG_EV_RTP_STREAM_ESTABLISHED in such case.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@153">Patch Set #9, Line 153:</a> <code style="font-family:monospace,monospace">if (cl->fi->state != CALL_LEG_ST_ESTABLISHED)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This check could be done before iterating over 'cl->rtp':</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if (cl->fi->state == CALL_LEG_ST_ESTABLISHED)<br>    break;</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@158">Patch Set #9, Line 158:</a> <code style="font-family:monospace,monospace">rtps = data;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This assignment looks useless to me. You could pass 'data' directly to osmo_fsm_inst_dispatch().</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@178">Patch Set #9, Line 178:</a> <code style="font-family:monospace,monospace">struct call_leg *cl = fi->priv;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here, just pass 'fi->priv' directly to osmo_fsm_inst_dispatch(). Casting from (void *) to (struct call_leg *) and then back to (void *) doesn't make sense.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@192">Patch Set #9, Line 192:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@202">Patch Set #9, Line 202:</a> <code style="font-family:monospace,monospace">break</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should we OSMO_ASSERT() here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@211">Patch Set #9, Line 211:</a> <code style="font-family:monospace,monospace">{}</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I know that you prefer this way of array-termination, but in general we use { 0, NULL }. Not critical, but let's please be consistent.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@229">Patch Set #9, Line 229:</a> <code style="font-family:monospace,monospace">call_leg_fsm_establishing_established</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The function name looks confusing... Establishing established?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@244">Patch Set #9, Line 244:</a> <code style="font-family:monospace,monospace">/* same action function as above */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ok, now I see. But still, it looks odd that we have two different states with the same set of events, state transitions and the same event handler...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@248">Patch Set #9, Line 248:</a> <code style="font-family:monospace,monospace">in_event_mask</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since we terminate the FSM instance as soon as we enter this state, is there a chance that it would receive any events?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@253">Patch Set #9, Line 253:</a> <code style="font-family:monospace,monospace">/* same action function as above */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Copy-pasted ;)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@257">Patch Set #9, Line 257:</a> <code style="font-family:monospace,monospace">static struct osmo_fsm call_leg_fsm</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This symbol was already defined at line #59.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/call_leg.c@290">Patch Set #9, Line 290:</a> <code style="font-family:monospace,monospace">call_leg_ensure_rtp_alloc</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Basically, this function just calls call_leg_rtp_alloc() and checks if the allocation was successful. Looks redundant to me.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/13137">change 13137</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/13137"/><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: I27e4988e0371808b512c757d2b52ada1615067bd </div>
<div style="display:none"> Gerrit-Change-Number: 13137 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-CC: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-CC: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 07 May 2019 21:11:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>