<p style="white-space: pre-wrap; word-wrap: break-word;">I am now through with the review, I hope nothing slipped through, the scrolling in gerrit is very broken at the moment...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Your comments from 01.11 match up with my understanding but I am asking myself now what happens if the subscriber attaches first to 2G/3G (periodic LU timer is running) and then goes to LTE, the SGs LU does not stop such timers but I have the feeling that we need to do exactly that.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also I think we don't have any TTCN3 test that verifies the periodic LU for 2G. If we do not have that yet we should implement it.</p><p><a href="https://gerrit.osmocom.org/11642">View Change</a></p><p>96 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/3/include/osmocom/msc/gsm29118.h">File include/osmocom/msc/gsm29118.h:</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/3/include/osmocom/msc/gsm29118.h@9">Patch Set #3, Line 9:</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;"><br><br><br><br><br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">do we really want to have all of those members dynamically allocated on the heap?  I think our gener […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/ran_conn.h">File include/osmocom/msc/ran_conn.h:</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/29/include/osmocom/msc/ran_conn.h@216">Patch Set #29, Line 216:</a> <code style="font-family:monospace,monospace">RAN_CONN_USE_UNTRACKED =</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(yes, it is nicer to keep the last comma, so later additions are single-line patches)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/ran_conn.h@216">Patch Set #29, Line 216:</a> <code style="font-family:monospace,monospace">RAN_CONN_USE_UNTRACKED =</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Missing comma.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/ran_conn.h">File include/osmocom/msc/ran_conn.h:</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/8/include/osmocom/msc/ran_conn.h@118">Patch Set #8, Line 118:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ws</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/ran_conn.h@213">Patch Set #8, Line 213:</a> <code style="font-family:monospace,monospace">ran_conn_sgs_re</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It looks like this symbols should have been updated after […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_iface.h">File include/osmocom/msc/sgs_iface.h:</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/29/include/osmocom/msc/sgs_iface.h@29">Patch Set #29, Line 29:</a> <code style="font-family:monospace,monospace">     /* Socket name from osmo_sock_get_name() */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Here, I guess talloc ownership is clear and straightforward, so it would be ok to keep this as-is, […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/3/include/osmocom/msc/sgs_iface.h">File include/osmocom/msc/sgs_iface.h:</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/3/include/osmocom/msc/sgs_iface.h@2">Patch Set #3, Line 2:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we should add coypyright headers to all new files, sorry for not having one in my original patch.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Do we really place copyright notices into header files? I thought the policy here was not to do this? However I checked some header files, some of them have copyright notices some of them do not. I have now added the copyright headers and I would vote to adding copyright notices to all files, regardless of what it is.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h">File include/osmocom/msc/sgs_server.h:</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/29/include/osmocom/msc/sgs_server.h@1">Patch Set #29, Line 1:</a> <code style="font-family:monospace,monospace">#pragma once</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">closing brace (C), and I guess it should contain 2019</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h@23">Patch Set #29, Line 23:</a> <code style="font-family:monospace,monospace">          unsigned int timer[_NUM_SGS_STATE_TIMERS];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Maybe this name could be "*_SGS_SERVER_*"?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/sgs_server.h">File include/osmocom/msc/sgs_server.h:</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/8/include/osmocom/msc/sgs_server.h@34">Patch Set #8, Line 34:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">const?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is (should be) set by the VTY.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Unfortunately it was not set before. I have fixed this now It is populated with a default value at first and can be changed using the VTY now. Same is true for the port.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_vty.h">File include/osmocom/msc/sgs_vty.h:</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/29/include/osmocom/msc/sgs_vty.h@1">Patch Set #29, Line 1:</a> <code style="font-family:monospace,monospace">#pragma once</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think normally we don't have (C) in . […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/vlr.h">File include/osmocom/msc/vlr.h:</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/29/include/osmocom/msc/vlr.h@114">Patch Set #29, Line 114:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why do we need this? I don't see any references to this struct.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/vlr.h@114">Patch Set #29, Line 114:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">the struct is defined 7 lines further down; indeed drop this</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/vlr.h">File include/osmocom/msc/vlr.h:</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/8/include/osmocom/msc/vlr.h@196">Patch Set #8, Line 196:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please mark it with a FIXME label.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have removed this. The spec says that the association shall be marked with the cause code on certain events but I could not find any suggestion on how the cause code can be useful later on. Presumably this is intended for logging or diagnosing etc... Should we ever need to keep the cause code we can add it again at any time.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/3/src/libmsc/gsm29118.c">File src/libmsc/gsm29118.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/3/src/libmsc/gsm29118.c@65">Patch Set #3, Line 65:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">could be simplified by msgb_push_u8(msg, msg_type)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/gsm29118.c">File src/libmsc/gsm29118.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/5/src/libmsc/gsm29118.c@39">Patch Set #5, Line 39:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Could you add a comment to explain why len is overridden in this specific case? Is this safe?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have checked this back, its required by the spec. See note in the code.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c">File src/libmsc/gsm_04_08.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/29/src/libmsc/gsm_04_08.c@453">Patch Set #29, Line 453:</a> <code style="font-family:monospace,monospace">    conn->vsub->classmark.classmark1 = lu->classmark1;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">In principle, we put the bare msg composition stuff in libosmocore. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it makes sense to pull this apart, we already have that for BSSMAP and for SGs as well but I think this is in scope for another patch and we should indeed not block this patch with that. Apart from not much is messed up here. There are a lot of other gsm48_create... function in this file.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c@455">Patch Set #29, Line 455:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ws</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_04_08_annotated.c">File src/libmsc/gsm_04_08_annotated.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/8/src/libmsc/gsm_04_08_annotated.c@89">Patch Set #8, Line 89:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">There is no such symbol anymore. Why do we need this file BTW? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am sorry, this file shouln't be here. It was just there to keep some notes in the source code without having to mess around in the original file.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/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/8/src/libmsc/gsm_09_11.c@347">Patch Set #8, Line 347:</a> <code style="font-family:monospace,monospace">RV_IND_SMS);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">It seems your editor is doing this alignment automatically. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am not sure, I have now made it look like other function calls in the file. So it should be consistent now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c@350">Patch Set #8, Line 350:</a> <code style="font-family:monospace,monospace">ee(trans);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">USSD != SMS</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">See 3GPP TS 29.118 version 15.2.0, 9.4.17 Service indicator. There is only either "CS call indicator" or "SMS indicator" possible. I wonder if USSD is entirely handled by the MME if the mobile is on 4G? and therefore this code path is never executed on SGs Associated?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/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/8/src/libmsc/gsm_subscriber.c@58">Patch Set #8, Line 58:</a> <code style="font-family:monospace,monospace">int subscr_paging_dispatch(unsigned int hooknum, unsigned int event,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Unrelated change.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@71">Patch Set #8, Line 71:</a> <code style="font-family:monospace,monospace">        LOGP(DPAG, LOGL_DEBUG, "Paging %s for %s (event=%d)\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Unrelated change.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@74">Patch Set #8, Line 74:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Unrelated change.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@80">Patch Set #8, Line 80:</a> <code style="font-family:monospace,monospace">       }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Looks better, but anyway unrelated.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@88">Patch Set #8, Line 88:</a> <code style="font-family:monospace,monospace"> /* Inform parts of the system we don't know */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@106">Patch Set #8, Line 106:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Good idea for a separate change.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have put this in a separate patch.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@144">Patch Set #8, Line 144:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@153">Patch Set #8, Line 153:</a> <code style="font-family:monospace,monospace">                                         gsm_cbfn *cbfn, void *param,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Unrelated.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@156">Patch Set #8, Line 156:</a> <code style="font-family:monospace,monospace">  int rc;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Unrelated.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@165">Patch Set #8, Line 165:</a> <code style="font-family:monospace,monospace">          if (rc <= 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Unrelated.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/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/29/src/libmsc/gsm_subscriber.c@119">Patch Set #29, Line 119:</a> <code style="font-family:monospace,monospace">SCCP</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Agree, return val info should go to the "API doc" string. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is a static function, so its not part of the API and there shouldn't be a doc-string.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@119">Patch Set #29, Line 119:</a> <code style="font-family:monospace,monospace">SCCP</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this note should precede the function definition.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@130">Patch Set #29, Line 130:</a> <code style="font-family:monospace,monospace">             return sgs_iface_tx_paging(vsub, serv_ind);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">meta question: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">When the MME performs an LU on the SGs the VLR knows that the subscriber exists and in case of paging it will page through the SGs interface, the MME then does the real paging on the EUTRAN.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Paging is indeed managed on the LTE side, but if an SMS arrives, it will arrive at the MSC, then the MSC needs to know where to deliver the paging. The SGs-Association tells the MSC/VLR that the subscriber is attached on LTE and then sends the paging through the SGs interface to the MME. Those pagings are more or less a command to the MME to trigger the actual paging on LTE.</p><p style="white-space: pre-wrap; word-wrap: break-word;">When the LU happens through the SGs interface, vlr_sgs_fsm.c instead of vlr_lu_fsm.c does the work. I do not start a timer, so I think periodic LU should be disabled. If I am wrong here and the timer is started anyway, then we have a bug. I set the attached flag to be sure that the MSC is acting as if the subscriber were attached normally, so that paging, sms and csfb calls can be done as if the subscriber were on a 2G/3G network.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@151">Patch Set #29, Line 151:</a> <code style="font-family:monospace,monospace"> */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">end with dot</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/3/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/11642/3/src/libmsc/msc_vty.c@615">Patch Set #3, Line 615:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">unrelated notice: Might be worth introducing a function like omso_fsm_inst_state_name(vsup->sgs_fsm) […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">There is already a function with that name. I am using it now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/3/src/libmsc/msc_vty.c@619">Patch Set #3, Line 619:</a> <code style="font-family:monospace,monospace">             trans->vsub ? vlr_subscr_name(trans->vsub) : "-",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">no need to break the line here for VTY_NEWLINE</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/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/11642/8/src/libmsc/msc_vty.c@1429">Patch Set #8, Line 1429:</a> <code style="font-family:monospace,monospace"> vty_out(vty, " default-codec tch-h %s%s",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think it was already declared in 'sgs_vty.c'.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/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/11642/29/src/libmsc/msc_vty.c@705">Patch Set #29, Line 705:</a> <code style="font-family:monospace,monospace">                     VTY_NEWLINE);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">maybe […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">SGS_UE_ST_NULL is a condition that is interesting to see. SGS_UE_ST_NULL does not imply that the SGs interface is disabled.</p><p style="white-space: pre-wrap; word-wrap: break-word;">A subscriber may go to SGS_UE_ST_NULL and leave it several times throughout its lifetime. Not showing the info when SGS_UE_ST_NULL would be very confusing when debugging things.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/ran_conn.c">File src/libmsc/ran_conn.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/8/src/libmsc/ran_conn.c@394">Patch Set #8, Line 394:</a> <code style="font-family:monospace,monospace">functions (see above), </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Clarify please, *what* was already done?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/5/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/5/src/libmsc/sgs_iface.c@95">Patch Set #5, Line 95:</a> <code style="font-family:monospace,monospace">           LOGSGC(sgc, LOGL_ERROR, "(sub %s) Connection allocation failed\n", vlr_subscr_msisdn_or_name(vsub));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">spelling: "Cannot"</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@219">Patch Set #5, Line 219:</a> <code style="font-family:monospace,monospace">              }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Could check for sgc->mme->fqdn[0] == '\0' to avoid string traversal in case the string is not empty.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@307">Patch Set #5, Line 307:</a> <code style="font-family:monospace,monospace">     }</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">spelling: context-dependent</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@308">Patch Set #5, Line 308:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This function never fails. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">That is intended, when the function has a return code it becomes much easier to use. No {} + separate return statement needed. The returned 0 will propagate downwards and tell the main handler that the error has been handled.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@449">Patch Set #5, Line 449:</a> <code style="font-family:monospace,monospace">       /* FIXME: If we are in SGS_UE_ST_NULL while sub->conf_by_radio_contact_ind == false,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Check return value? This function works like snprintf(): […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">the IMSI is parsed in a central location since it is used so often. I have changed this now. See also sgs_iface_rx()</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@540">Patch Set #5, Line 540:</a> <code style="font-family:monospace,monospace">   enum vlr_lu_type type;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This function never fails. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">All rx handler functions must have a return code (see sgs_tx_status() above)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@567">Patch Set #5, Line 567:</a> <code style="font-family:monospace,monospace">  if (lu_type_ie[0] == 0x01)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This function never fails either. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(see above)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@611">Patch Set #5, Line 611:</a> <code style="font-family:monospace,monospace">                break;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Return an error code?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The error is logged, we could return a status to the MME indicating that something is wrong, but technically I think that it is ok just to leave the UDUP out and do nothing since UDUP is, as far as I understood it so far, just the beeping you hear when someone rejects the 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/5/src/libmsc/sgs_iface.c@782">Patch Set #5, Line 782:</a> <code style="font-family:monospace,monospace">/* SGsAP-UPLINK-UNITDATA 3GPP TS 29.118, chapter 8.22 */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Could check for imsi[0] == '\0' to avoid string traversal in case string is not empty.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think that we are that performance critical here, but I added the check. It would help a lot to know the minimum allowed length for an IMSI. The 6 is just a guess.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/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/8/src/libmsc/sgs_iface.c@1023">Patch Set #8, Line 1023:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I have never seen such odd alignment o_O. Looks like an alt statement […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/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/29/src/libmsc/sgs_iface.c@3">Patch Set #29, Line 3:</a> <code style="font-family:monospace,monospace">/* (C) 2018-2019 by sysmocom s.f.m.c. GmbH</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">) 19</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@98">Patch Set #29, Line 98:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is a bad thing to do in general. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@110">Patch Set #29, Line 110:</a> <code style="font-family:monospace,monospace">           conn->complete_layer3_type = COMPLETE_LAYER3_CM_SERVICE_REQ;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this is not the HLR ... the vlr_subscr_find_by_imsi() does not contact the HLR. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think in this case its just a typo. It should be VLR instead of HLR. When the subscriber is not in the VLR, we can not allocate a connection. An SGs LU has to be done before. Also the callers already checked that the subscriber is indeed there. Its probably missleading that there is an imsi parameter here. Using vsub directly makes more sense.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@121">Patch Set #29, Line 121:</a> <code style="font-family:monospace,monospace">/* Check if there are connections associated with a given subscriber. If yes,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">instead of another get() here, keep the get() use count from vlr_subscr_find_by_imsi(). […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@277">Patch Set #29, Line 277:</a> <code style="font-family:monospace,monospace">                      sgsap_msg_type_name(msg_type), imsi);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">In another VTY related patch I did argue that what you're doing here is safe. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@329">Patch Set #29, Line 329:</a> <code style="font-family:monospace,monospace">                  sgsap_sgs_cause_name(cause));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this is not so nice, since you're obtaining log context info, even though you don't know whether som […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@348">Patch Set #29, Line 348:</a> <code style="font-family:monospace,monospace">        resp = gsm29118_create_status(imsi, cause, msg);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">you are *always* logging an error now. I guess the initial 'if (sgsap_iei < 0)' should have a { .. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The errors are handeled, maybe ERROR is to strong and we should log those messages as NOTICE? The return code is fine. 0 just means that the error is handled.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@354">Patch Set #29, Line 354:</a> <code style="font-family:monospace,monospace"> * reject, depending on the outcome of the location update. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">[after reading other code] ... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thats correct. "Status" means "Error" in SGs</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@414">Patch Set #29, Line 414:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(when we're sending MM Info data, is that really a "Request"?)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, at least SGs calls it a "Request", see 3GPP TS 29.118 8.12. Maybe because the VLR is requesting the MME to send the MM information to the UE...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@454">Patch Set #29, Line 454:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Of course our VLR does not even have a VLR subscriber stored if it is not attached. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@597">Patch Set #29, Line 597:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(a switch() would be nicer here)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@624">Patch Set #29, Line 624:</a> <code style="font-family:monospace,monospace">/* SGsAP-EPS-DETACH-INDICATION 3GPP TS 29.118, chapter 8.6 */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(a switch() would be nicer here)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@650">Patch Set #29, Line 650:</a> <code style="font-family:monospace,monospace">     vlr_sgs_eps_detach(gsm_network->vlr, imsi, type);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">VLR != HLR</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@655">Patch Set #29, Line 655:</a> <code style="font-family:monospace,monospace">}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">AFAICT this is not sending anything to the HLR</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@708">Patch Set #29, Line 708:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">so an SGsAP Service Request is a reply to an earlier message from the 2G CN?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, its the response to the paging</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@772">Patch Set #29, Line 772:</a> <code style="font-family:monospace,monospace">  /* Allocate subscriber connection */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">is it possible that we obtain a conn that was established on GSM and not via SGs? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You mean when a subscriber has just created a conn on 2G/3G, then looses reception and the UE does an LU on LTE, resulting in an LU on SGs and then an MO SMS is sent we would get the old conn instead of creating a new one.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe you are right and we are better off if we check if there are lingering conns when either a location update on the SGs happens or when there is an CSFB indication (subsciber just came back to LTE aufer a fallback call). In both cases we shouldn't have connections associated with the VSUB. I have integrated such a check to make won't run into this.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@851">Patch Set #29, Line 851:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">LOGL_ERROR? Also below?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@865">Patch Set #29, Line 865:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">you are often repeating the pattern "create_status, tx, free". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@871">Patch Set #29, Line 871:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">GSM23003_IMSI_MIN_DIGITS</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@872">Patch Set #29, Line 872:</a> <code style="font-family:monospace,monospace">#define TX_STATUS_AND_LOG(sgc, msg_type, cause, fmt) \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(strlen() is enough, it implies a "[0] == '\0' check")</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@984">Patch Set #29, Line 984:</a> <code style="font-family:monospace,monospace">         break;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">others free() after sgs_tx(). One or the other is wrong.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The checks above abort the function early. The msg that comes in needs to be freed before this function exists. The checks here do not free because they have no return statement and the free is done at the bottom. I have restructured it a bit to reduce the amount of msgb_free() here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@987">Patch Set #29, Line 987:</a> <code style="font-family:monospace,monospace">                break;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1083">Patch Set #29, Line 1083:</a> <code style="font-family:monospace,monospace">           reset_params.vlr_name_present = true;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(would be nice to also log the Ns11 value)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1140">Patch Set #29, Line 1140:</a> <code style="font-family:monospace,monospace">     },</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">inline comment rather without doxygen marker</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1212">Patch Set #29, Line 1212:</a> <code style="font-family:monospace,monospace">       *  the VLR subscriber usage to be balanced. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">register sgs_ue_fsm?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is the only FSM we need to register here. sgs_ue_fsm is inside the VLR and everything related to that is done there.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/5/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/5/src/libmsc/sgs_server.c@50">Patch Set #5, Line 50:</a> <code style="font-family:monospace,monospace">           goto out;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Set rc to -EBADF to allow an outer event loop to detect a closed socket?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I assign -EBADF to rc after each osmo_stream_srv_destroy() now. I hope this is correct now.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/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/8/src/libmsc/sgs_server.c@160">Patch Set #8, Line 160:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Shouldn't this be configurable from the VTY?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Those are default values which are changed by sgs_server_open() afterwards.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It should be configurable of course. I have fixed it now and there are default values.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/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/29/src/libmsc/sgs_server.c@1">Patch Set #29, Line 1:</a> <code style="font-family:monospace,monospace">/* (C) 2018-2019 by sysmocom s.f.m.c. GmbH</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">) 19</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_vty.c">File src/libmsc/sgs_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/11642/5/src/libmsc/sgs_vty.c@141">Patch Set #5, Line 141:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Use a #define for the port number?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/sgs_vty.c">File src/libmsc/sgs_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/11642/8/src/libmsc/sgs_vty.c@46">Patch Set #8, Line 46:</a> <code style="font-family:monospace,monospace">}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Let's don't mix everything into a single line. Hard to read.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I see, I have now split it up. I hope it is now better to read.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c">File src/libmsc/sgs_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/11642/29/src/libmsc/sgs_vty.c@1">Patch Set #29, Line 1:</a> <code style="font-family:monospace,monospace">/* (C) 2018-2019 by sysmocom s.f.m.c. GmbH</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">) 19</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@99">Patch Set #29, Line 99:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_sgs_timer, cfg_sgs_timer_cmd,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">have you tested that the keywords are allowed to contain capital letters? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I did not know that before. I have tested it now and it indeed causes problems. I have now made it all into small letters.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@123">Patch Set #29, Line 123:</a> <code style="font-family:monospace,monospace">DEFUN(cfg_sgs_counter, cfg_sgs_counter_cmd,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">same capital chars: tested?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/subscr_conn.c">File src/libmsc/subscr_conn.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/5/src/libmsc/subscr_conn.c@725">Patch Set #5, Line 725:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What does 'flag' mean? Try to choose a more expressive name?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c">File src/libvlr/vlr_sgs.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/3/src/libvlr/vlr_sgs.c@91">Patch Set #3, Line 91:</a> <code style="font-family:monospace,monospace">    vsub->sgs.paging_cb = paging_cb;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">that kind of printf of course needs to be removed before an eventual merge</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I removed this one, but there are others. I am using them as orientation while testing things. I will remove all of them of course before merge.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c">File src/libvlr/vlr_sgs.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/5/src/libvlr/vlr_sgs.c@143">Patch Set #5, Line 143:</a> <code style="font-family:monospace,monospace"> case SGSAP_ID_NONEPS_T_COMBINED_UE_EPS_NONEPS:</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Missing 'break;' in default case.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@174">Patch Set #5, Line 174:</a> <code style="font-family:monospace,monospace">           break;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Missing 'break;' in default case.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@203">Patch Set #5, Line 203:</a> <code style="font-family:monospace,monospace">}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">spelling: reception</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@237">Patch Set #5, Line 237:</a> <code style="font-family:monospace,monospace"> /* Stop Ts5 and and consider the paging as successful */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">spelling: reception</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@291">Patch Set #5, Line 291:</a> <code style="font-family:monospace,monospace">        * an explicit failure is indicated (MME actively rejects paging).</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Could be simplified to: return osmo_timer_pending(&vsub->sgs. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c">File src/libvlr/vlr_sgs.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/29/src/libvlr/vlr_sgs.c@89">Patch Set #29, Line 89:</a> <code style="font-family:monospace,monospace">     vsub->sgs.cfg = *cfg;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">rather […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c@101">Patch Set #29, Line 101:</a> <code style="font-family:monospace,monospace">    vsub->cgi.lai = *new_lai;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">please don't use memcpy, but assign members directly. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c@205">Patch Set #29, Line 205:</a> <code style="font-family:monospace,monospace">/*! Notify that an SGs paging has been rejected by the MME.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">wrong comment</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c@226">Patch Set #29, Line 226:</a> <code style="font-family:monospace,monospace">/*! Notify that an SGs paging has been accepted by the MME.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">wrong comment</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/5/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/5/src/libvlr/vlr_sgs_fsm.c@197">Patch Set #5, Line 197:</a> <code style="font-family:monospace,monospace">           /* In cases where the LU has interrupted the paging, respawn the paging now,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Missing 'break;' here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">yes, its missing. Thanks.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/3/tests/msc_vlr/msc_vlr_test_call.c">File tests/msc_vlr/msc_vlr_test_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/11642/3/tests/msc_vlr/msc_vlr_test_call.c@592">Patch Set #3, Line 592:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">please don't copy+paste this to each test but rather introduce a shared file with those stubs which  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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: 30 </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: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-CC: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 15 Jan 2019 10:51:54 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>