<p style="white-space: pre-wrap; word-wrap: break-word;">Good work so far; I did still find a lot of remaining problems, but we're well on our way to get this merged.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(Some of the remarks are optional to fix, skip those where you are sure and prefer to leave as is)</p><p>Patch set 30:<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/11642">View Change</a></p><p>42 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/include/osmocom/msc/sgs_iface.h@42">Patch Set #30, Line 42:</a> <code style="font-family:monospace,monospace"> /* global list of MME contexts */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is the *entry* in that list, right?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/include/osmocom/msc/sgs_server.h@4">Patch Set #30, Line 4:</a> <code style="font-family:monospace,monospace">#define DEFAULT_VLR_NAME "*_SGS_SERVER_*"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I meant to name the variables DEFAULT_SGS_SERVER_IP and _NAME, not the enclosed URL.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@18">Patch Set #30, Line 18:</a> <code style="font-family:monospace,monospace"> char *local_addr;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(char array?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/include/osmocom/msc/sgs_server.h@21">Patch Set #30, Line 21:</a> <code style="font-family:monospace,monospace"> char *vlr_name;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">... here I'd prefer char arrays.<br>(If you have reasons against it then keep it this way though.)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/include/osmocom/msc/vlr.h@349">Patch Set #30, Line 349:</a> <code style="font-family:monospace,monospace"> const char *imsi);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If an IMSI is present, it is already part of the vlr_subscr_name().<br>Also, since recently, vlr_susbcr_name() includes all available info on a subscriber, including IMSI, MSISDN and TMSI at the same time. (I think we shouldn't continue on the weird path I started with vlr_subscr_msisdn_or_name().)</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;">I think it makes sense to pull this apart, we already have that for BSSMAP and for SGs as well but I […]</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/11642/30/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/30/src/libmsc/gsm_04_08.c@473">Patch Set #30, Line 473:</a> <code style="font-family:monospace,monospace"> * See also GSM 04.08, chapter 9.2.15a */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I guess preferably these days include the more specific 3GPP TS 24.008 or 44.008 ... nr; maybe next time.)</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;">I am not sure, I have now made it look like other function calls in the file. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Vadim, aligning function arguments with the opening brace is a common code formatting style, also default of vim's cindent. I've used this style ever since I started in Osmocom (and also in other code bases before that).</p><p style="white-space: pre-wrap; word-wrap: break-word;">I would gladly adopt the single-tab indent if I could only figure out cindent config to do so.<br>Do you use vim? Do you happen to know a cindent rule set to achieve single tab?</p><p style="white-space: pre-wrap; word-wrap: break-word;">I am definitely not going to manually edit all function arguments indenting and fight against cindent, then I'd rather just keep cindent's align-with-opening-brace; I am using automatic indenting extensively, doing it manually would multiply the time i need to write code.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Consistency wise, I've seen both single-tab and align-with-brace indenting in the Osmocom code base, though align-with-brace seems to be more common.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(Although, for function arguments, two tabs would actually make more sense, especially for function definitions where the following body also has one tab indent.)</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;">This is a static function, so its not part of the API and there shouldn't be a doc-string.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">just because it is a static function doesn't mean you should drop common commenting practices :P<br>We're not as strict with static functions, but if you are documenting the function's return code, then put it in a comment above the function definition.</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;">When the MME performs an LU on the SGs the VLR knows that the subscriber exists and in case of pagin […]</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/11642/30/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/30/src/libmsc/gsm_subscriber.c@120">Patch Set #30, Line 120:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(unrelated ws)</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@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;">Thats correct. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">then all of this is fine, including the LOGL_ERROR.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(I am sometimes a bit unsure whether services being rejected should qualify for LOGL_ERROR or LOGL_NOTICE, but so far I've always decided for LOGL_ERROR in the end)</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;">Yes, at least SGs calls it a "Request", see 3GPP TS 29.118 8.12. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ack; same on the E-interface, where DTAP is forwarded both ways as "Request"s, regardless whether they are a response to earlier DTAP or not.</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;">You mean when a subscriber has just created a conn on 2G/3G, then looses reception and the UE does a […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">for starters, this code path could check that via_ran == OSMO_RAT_EUTRAN_SGS and error out otherwise.<br>In the long run it might make more sense to discard the other conn, to not have to wait a periodic lu expiry before SGs attach becomes possible?</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;">The checks above abort the function early. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ah, of course, thx</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;">This is the only FSM we need to register here. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">last time i grepped it wasn't there (?) but it's definitely there now. ack.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/src/libmsc/sgs_iface.c@54">Patch Set #30, Line 54:</a> <code style="font-family:monospace,monospace">/* See also comment in a_iface.c */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">which one, "The pointer is set once when calling a_init()"? :)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@99">Patch Set #30, Line 99:</a> <code style="font-family:monospace,monospace"> conn->vsub = vsub; </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be vlr_subscr_get(vsub), right??</p><p style="white-space: pre-wrap; word-wrap: break-word;">(whitespace error)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@100">Patch Set #30, Line 100:</a> <code style="font-family:monospace,monospace"> conn->vsub->cs.attached_via_ran = conn->via_ran;</code></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">here (after assigning vsub and via_ran) please call<br> ran_conn_update_id(conn)<br>for logging context.</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@103">Patch Set #30, Line 103:</a> <code style="font-family:monospace,monospace"> * authenticated by the MME no authentication is requred */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"required"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@117">Patch Set #30, Line 117:</a> <code style="font-family:monospace,monospace"> vlr_subscr_msisdn_or_name(vsub));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">would still prefer if the LOGxxx macros include the log ctx implicitly. Actually, I have recently added such to ran_conn, so:</p><p style="white-space: pre-wrap; word-wrap: break-word;">IMO what you should do is, in ran_conn.c, in ran_conn_alloc add</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> case OSMO_RAT_EUTRAN_SGS:<br> conn->log_subsys = DSGS;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and then use LOG_RAN_CONN(conn, LOGL_DEBUG, "Allocated\n");</p><p style="white-space: pre-wrap; word-wrap: break-word;">(the conn's FI should anyway already log that it is allocated, but use LOG_RAN_CONN() for all SGs logging; then you will always have the full subscriber and conn context added to the logging implicitly)</p><p style="white-space: pre-wrap; word-wrap: break-word;">For places where there is no ran_conn available I'm not sure how to best solve this, but if possible also go for logging macros that include all applicable context info implicitly. (I think I've mentioned this in an earlier review; maybe you decided it is not possible?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@132">Patch Set #30, Line 132:</a> <code style="font-family:monospace,monospace"> vlr_subscr_msisdn_or_name(vsub));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">LOG_RAN_CONN()</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libmsc/sgs_iface.c@245">Patch Set #30, Line 245:</a> <code style="font-family:monospace,monospace"> vlr_subscr_msisdn_or_name(vsub), sgsap_msg_type_name(msg_type));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(rather just vlr_subscr_name(), same below)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/src/libmsc/sgs_server.c@187">Patch Set #30, Line 187:</a> <code style="font-family:monospace,monospace"> talloc_free(name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">suggest using osmo_sock_get_name2(), needs no talloc_free()</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/src/libmsc/sgs_vty.c">Patch Set #30:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">I would appreciate if you could add some .vty transcript test for the SGs VTY nodes, verifying that the new commands work as expected, show the expected docs and print back the expected 'show running-config'.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr.c">File src/libvlr/vlr.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/30/src/libvlr/vlr.c@125">Patch Set #30, Line 125:</a> <code style="font-family:monospace,monospace"> const char *imsi)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you want to keep this, the name more accurately would be 'vlr_subscr_imsi_or_msisdn_or_name()' ... but again rather just use vlr_subscr_name(), as we now also do in most FSM instance IDs and log contexts</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@1">Patch Set #3, Line 1:</a> <code style="font-family:monospace,monospace">/* (C) 2018-2019 by sysmocom s.f.m.c. GmbH</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">-2019?</p><p style="white-space: pre-wrap; word-wrap: break-word;">feel free to grep the other files yourself...</p><p style="white-space: pre-wrap; word-wrap: break-word;">(BTW, I am often thinking we should automate the copyright year indications somehow, by which years contain changes to the 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/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;">I removed this one, but there are others. I am using them as orientation while testing things. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">confirmed: I grepped the latest patch set for '\<printf' and found no hits.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/src/libvlr/vlr_sgs.c@215">Patch Set #30, Line 215:</a> <code style="font-family:monospace,monospace"> vlr_subscr_put(vsub);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">throughout this file: do the vlr_subscr_put() *below* all uses! That's the whole idea of ref counting.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs.c@223">Patch Set #30, Line 223:</a> <code style="font-family:monospace,monospace"> osmo_fsm_inst_dispatch(vsub->sgs_fsm, SGS_UE_E_RX_PAGING_FAILURE, &cause);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">you are still using it here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Same in other functions in this file, not marking each one 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/30/src/libvlr/vlr_sgs.c@308">Patch Set #30, Line 308:</a> <code style="font-family:monospace,monospace"> vsub->sgs.paging_serv_ind = serv_ind;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm getting the impression that you should do another vlr_subscr_get() to increase the vlr_subscr's use count while paging, and decrease when paging is done. Have you given this consideration yet?</p><p style="white-space: pre-wrap; word-wrap: break-word;">For example, when above Ts5_timeout_cb() gets invoked, you want to be sure that the vsub has not been deallocated yet, even if the subscriber has somehow detached from the SGs (like an IMSI detach or an SGs connection reset?)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/src/libvlr/vlr_sgs_fsm.h">File src/libvlr/vlr_sgs_fsm.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/30/src/libvlr/vlr_sgs_fsm.h@35">Patch Set #30, Line 35:</a> <code style="font-family:monospace,monospace"> SGS_UE_E_RX_SGSAP_UE_UNRECHABLE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">UNREACHABLE with A</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@227">Patch Set #5, Line 227:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(here and below would prefer N-tabs indent instead of align-with-opening-brace; curly braces are definitely no candidates for align-with-brace)</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_fsm.c@231">Patch Set #5, Line 231:</a> <code style="font-family:monospace,monospace"> struct vlr_subscr *vsub = fi->priv;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">one more indent for multi-line member assignment</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_fsm.c@238">Patch Set #5, Line 238:</a> <code style="font-family:monospace,monospace"> /* See 5.4.3 and 5.5.3 */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">more indent</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_fsm.c@250">Patch Set #5, Line 250:</a> <code style="font-family:monospace,monospace"> default:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">more indent</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_fsm.c@252">Patch Set #5, Line 252:</a> <code style="font-family:monospace,monospace"> break;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">more indent</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_fsm.c@262">Patch Set #5, Line 262:</a> <code style="font-family:monospace,monospace"> /* Failed TMSI reallocation procedure, deallocate all TMSI</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rather indent with two tabs (and how did you end up with the weird line breaks)</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_fsm.c@293">Patch Set #5, Line 293:</a> <code style="font-family:monospace,monospace"> S(SGS_UE_E_RX_SGSAP_UE_UNRECHABLE) | S(SGS_UE_E_RX_PAGING_FAILURE) | S(SGS_UE_E_RX_ALERT_FAILURE) |</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">drop?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/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/30/src/libvlr/vlr_sgs_fsm.c@61">Patch Set #30, Line 61:</a> <code style="font-family:monospace,monospace"> LOGPFSML(fi, LOGL_ERROR, "(sub %s) VLR LU tmsi allocation failed\n", vlr_subscr_msisdn_or_name(vsub));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use vlr_subscr_name(), or rather have that context in the fi->id already</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/tests/msc_vlr/stubs.h">File tests/msc_vlr/stubs.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/30/tests/msc_vlr/stubs.h@5">Patch Set #30, Line 5:</a> <code style="font-family:monospace,monospace"> * Author: Neels Hofmeyr <nhofmeyr@sysmocom.de></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">that's me, though.<br>are you copying another stubs.h that we could just re-use instead of copying?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11642/30/tests/sms_queue/sms_queue_test.c">File tests/sms_queue/sms_queue_test.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/30/tests/sms_queue/sms_queue_test.c@240">Patch Set #30, Line 240:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">are these the same as stubs.h?</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: Thu, 17 Jan 2019 18:27:20 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>