<p><a href="https://gerrit.osmocom.org/12402">View Change</a></p><p>10 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12402/9/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/12402/9/src/libmsc/gsm_04_08.c@304">Patch Set #9, Line 304:</a> <code style="font-family:monospace,monospace">gsm48_mi_type_name(mi_type), mi_string</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This could be also changed to osmo_mi_name(...).</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ok. I didn't want to refactor all the logging though :P<br>Putting it 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/12402/9/src/libmsc/gsm_04_08.c@752">Patch Set #9, Line 752:</a> <code style="font-family:monospace,monospace">req->cm_service_type</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Looks like a mistake to me. The message says "mi_type ... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">that's right. I already fixed it later on my branch, didn't realize I actually introduced the mistake here. Thanks!</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12402/9/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/12402/9/src/libmsc/ran_conn.c@696">Patch Set #9, Line 696:</a> <code style="font-family:monospace,monospace">_ran_conn_update_id</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">TBH, this looks unreadable. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">because of the string fmt args. I'd have to introduce yet another va_args thing like osmo_fsm_inst_update_id_f().<br>I mean, it's possible. But then I'd also need another char buffer of limited size, or another talloc string as well, and all that jazz. So I went for ## ARGS instead.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Oh wait, looking at it now, I guess the FMT passed from all callers ends up just being '%s' now, so there is indeed a simpler way to implement it. (there were more complex intermediate patch states)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks!</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12402/9/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/12402/9/src/libvlr/vlr.c@82">Patch Set #9, Line 82:</a> <code style="font-family:monospace,monospace">vlr_subscr_name</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I couldn't resist and wrote an alternative version using the Pointer arithmetic. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I've done exactly that before, but I wanted to have simpler code this time.<br>See used_ref_counts_str(), the APPEND_STR macro.<br>Or see gsm0808_utils.c, the APPEND_THING, APPEND_STR and APPEND_CELL_ID_U macros.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe we can glorify such a macro to the OSMO_* namespace? But it's not so trivial to make it universally useful.<br>If there is a limited fixed amount of elements, I prefer the fixed string buffers approach,<br>because it is trivially obvious that it doesn't contain mistakes. As you can see below... oh wait...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@89">Patch Set #9, Line 89:</a> <code style="font-family:monospace,monospace">present</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You could just init buf[0] as '\0', and then check like this: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">no, buf only gets composed right at 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/12402/9/src/libvlr/vlr.c@93">Patch Set #9, Line 93:</a> <code style="font-family:monospace,monospace">snprintf</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What if this call would fail?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">then imsi would be empty, right?<br>But how can it possibly fail. There is enough space. The string fmt is trivial. The input is a plain ASCII string, even only ['0'-'9'] chars.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@101">Patch Set #9, Line 101:</a> <code style="font-family:monospace,monospace">tmsi, sizeof(buf)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Printing to tmsi, but sizeof(buf)?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh shit!</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr.c@105">Patch Set #9, Line 105:</a> <code style="font-family:monospace,monospace">snprintf(buf, sizeof(buf),</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Wouldn't this call overwrite the content of buffer?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">actually it writes to buf[], and later buf[] gets overwritten from scratch, so TMSInew is always lost.</p><p style="white-space: pre-wrap; word-wrap: break-word;">oh so *that's* why I'm not seeing TMSInew in any logging! :D<br>thanks man</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12402/9/src/libvlr/vlr_core.h">File src/libvlr/vlr_core.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/12402/9/src/libvlr/vlr_core.h@7">Patch Set #9, Line 7:</a> <code style="font-family:monospace,monospace">const</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This could be done in a separate change...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">someone else actually already did.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12402/9/tests/msc_vlr/msc_vlr_test_authen_reuse.err">File tests/msc_vlr/msc_vlr_test_authen_reuse.err:</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/12402/9/tests/msc_vlr/msc_vlr_test_authen_reuse.err@11">Patch Set #9, Line 11:</a> <code style="font-family:monospace,monospace">:</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think it makes sense to separate the RAT type from the subscriber name in some different way, e.g. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Limitation here is that the FSM instance ID must be a valid identifier as osmo_identifier_valid().<br>One reason is that the FSM instances are browsable from the CTRL interface.</p><p style="white-space: pre-wrap; word-wrap: break-word;">An earlier patch version had this:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  GERAN-A-0_IMSI-123456_LU</pre><p style="white-space: pre-wrap; word-wrap: break-word;">First off, we get the RAN conn (GERAN-A-0), then obtain the mobile identity and note down the L3 complete type.<br>My first change was to put the L3 type next to the conn.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  GERAN-A-0_LU_IMSI-123456</pre><p style="white-space: pre-wrap; word-wrap: break-word;">But semantically, a subscriber may contact us several times with (of course) the same IMSI.<br>So I prefer to put the subscriber identification at the start.<br>What connection ID is in use and its L3 complete type follows:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  IMSI-123456_MSISDN-1234_GERAN-A-0_LU<br>  IMSI-123456_MSISDN-1234_GERAN-A-1_CM_SERVICE_REQ<br>  IMSI-123456_MSISDN-1234_UTRAN-Iu-5_LU</pre><p style="white-space: pre-wrap; word-wrap: break-word;">That seems to be more logical to me to follow subscriber context in a log.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Next up, I didn't like the '-' and '_' mix, it is hard to figure out visually which of them is the separator.<br>Neither space nor ',|/' are permitted characters for osmo_identifier_valid().<br>We already use ':' as separator elsewhere.</p><p style="white-space: pre-wrap; word-wrap: break-word;">A drawback of ':' is that it conflicts with stats exporting where ':' is not permitted, but IIUC we have solved this problem generically, already.</p><p style="white-space: pre-wrap; word-wrap: break-word;">That's how I reached this ordering of identifiers. I think it makes a lot of sense.<br>If you have good reasons or strong opinions I'm fine with changing it, it's a bikeshed after all.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12402">change 12402</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/12402"/><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: I66a68ce2eb8957a35855a3743d91a86299900834 </div>
<div style="display:none"> Gerrit-Change-Number: 12402 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </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-Comment-Date: Thu, 10 Jan 2019 23:32:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>