<p style="white-space: pre-wrap; word-wrap: break-word;">In general, looks good. Just a few cosmetic issues and more or less important note about SS/USSD.</p><p>Patch set 32:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Code-Review +1</span></p><p><a href="https://gerrit.osmocom.org/11642">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/11642/32/src/libmsc/gsm_09_11.c">File src/libmsc/gsm_09_11.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/11642/32/src/libmsc/gsm_09_11.c@347">Patch Set #32, Line 347:</a> <code style="font-family:monospace,monospace">SGSAP_SERV_IND_SMS</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hmm, according to TS 129.118, section 9.4.17, CS call indicator is used for all the CS services except the SMS. So, I think we should use SGSAP_SERV_IND_CS_CALL here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/gsm_subscriber.c">File src/libmsc/gsm_subscriber.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/11642/32/src/libmsc/gsm_subscriber.c@134">Patch Set #32, Line 134:</a> <code style="font-family:monospace,monospace">LOGP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This change seems to be unrelated.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c">File src/libmsc/sgs_iface.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/11642/32/src/libmsc/sgs_iface.c@66">Patch Set #32, Line 66:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct sgs_state;<br>struct sgs_connection;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You're referencing both structures a few lines above, so it looks like we don't need this forward declaration? Otherwise it should have been placed *before* the first reference(s).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c@113">Patch Set #32, Line 113:</a> <code style="font-family:monospace,monospace">complete_layer3_type</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Alternatively:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  conn->complete_layer3_type = mt ?<br>    COMPLETE_LAYER3_PAGING_RESP : COMPLETE_LAYER3_CM_SERVICE_REQ;<br>  ran_conn_update_id(conn);</pre><p style="white-space: pre-wrap; word-wrap: break-word;">or you could move ran_conn_update_id() outside the 'if'.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c@335">Patch Set #32, Line 335:</a> <code style="font-family:monospace,monospace">vlr_subscr_put</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Where is the corresponding vlr_subscr_get()? Why do we need this call?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c@519">Patch Set #32, Line 519:</a> <code style="font-family:monospace,monospace">osmo_hexdump(err_msg,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(cosmetic) could be moved to the next line, so there would be no need to add such long spacing (\t\t\t\t\t\t...).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_server.c">File src/libmsc/sgs_server.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/11642/32/src/libmsc/sgs_server.c@68">Patch Set #32, Line 68:</a> <code style="font-family:monospace,monospace">/* do we have to notify the SGs code about this? */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should be marked with TODO or FIXME.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/32/src/libvlr/vlr_sgs_fsm.c">File src/libvlr/vlr_sgs_fsm.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/11642/32/src/libvlr/vlr_sgs_fsm.c@279">Patch Set #32, Line 279:</a> <code style="font-family:monospace,monospace">S(SGS_UE_E_RX_LU_FROM_MME) | S(SGS_UE_E_TX_PAGING)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it would be better to have each state / event on a separate line, like we do in osmo-bsc. This would improve readability and make the git diff cleaner when adding new states / events...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Example: https://git.osmocom.org/osmo-bsc/tree/src/osmo-bsc/lchan_fsm.c#n1125</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11642">change 11642</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/11642"/><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: I73359925fc1ca72b33a1466e6ac41307f2f0b11d </div>
<div style="display:none"> Gerrit-Change-Number: 11642 </div>
<div style="display:none"> Gerrit-PatchSet: 32 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </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-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 22 Jan 2019 10:52:47 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>