<p>Patch set 9:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/12556">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/Makefile.am">File tests/trans/Makefile.am:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/Makefile.am@4">Patch Set #9, Line 4:</a> <code style="font-family:monospace,monospace">-I$(top_srcdir)/src/libmsc</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks useless to me. The only header in there is smpp_smsc.h (which should be also moved to 'include' in some separate patch).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c">File tests/trans/trans_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/12556/9/tests/trans/trans_test.c@34">Patch Set #9, Line 34:</a> <code style="font-family:monospace,monospace">ref_id</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This name looks a bit confusing. What is the meaning of 'ref_id'?<br>Reference ID? What is the reference then?</p><p style="white-space: pre-wrap; word-wrap: break-word;">As far as I can see, you're always passing transaction ID, so let's name it 'trans_id' then?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@51">Patch Set #9, Line 51:</a> <code style="font-family:monospace,monospace">trans_assign_trans_id</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You're always calling it with ti_flag = 0. I would really like this test case to cover both '0'B and '1'B cases.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@68">Patch Set #9, Line 68:</a> <code style="font-family:monospace,monospace">base_callref</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What is the point of passing callref?<br>Why not to use a static counter?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@79">Patch Set #9, Line 79:</a> <code style="font-family:monospace,monospace">return</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would rather use OSMO_ASSERT here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@83">Patch Set #9, Line 83:</a> <code style="font-family:monospace,monospace">return</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">... and here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@88">Patch Set #9, Line 88:</a> <code style="font-family:monospace,monospace">return</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's just imagine that trans_has_conn() returns X != 0. This test would just fail, and one would have to investigate *why*, most likely by adding debug printf()s or asserts.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is why I also recommend to use OSMO_ASSERT here too.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@92">Patch Set #9, Line 92:</a> <code style="font-family:monospace,monospace">return</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">... and here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@94">Patch Set #9, Line 94:</a> <code style="font-family:monospace,monospace">vlr_subscr_name</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You say 'connection', but print subscriber name? o_O</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@134">Patch Set #9, Line 134:</a> <code style="font-family:monospace,monospace">base_callref</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hmm, allocating multiple transactions with same callref?</p><p style="white-space: pre-wrap; word-wrap: break-word;">It's a known issue (at least to me), that trans_alloc() would happily allocate a transaction with duplicate callref. And I expected that this test case would disclose this problem...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.c@151">Patch Set #9, Line 151:</a> <code style="font-family:monospace,monospace">\n\tCleared   %u transactions.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's split up this message into 2 printf() calls.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.err">File tests/trans/trans_test.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/12556/9/tests/trans/trans_test.err@1">Patch Set #9, Line 1:</a> <code style="font-family:monospace,monospace">Unknown RAN type</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Cosmetic: you could use any 'known' RAN type to avoid this.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12556/9/tests/trans/trans_test.err@2">Patch Set #9, Line 2:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, I expected to see debug messages here, like:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  (ti %02x sub %s callref %x) New transaction</pre><p style="white-space: pre-wrap; word-wrap: break-word;">transaction.c (for some reason) is using DCC, so<br>you could set its level to DEBUG.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12556">change 12556</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/12556"/><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: I78dfb7cd35073a305cf668beda7d9d58d5a5a713 </div>
<div style="display:none"> Gerrit-Change-Number: 12556 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: Max <msuraev@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: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 14 Jan 2019 23:42:41 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>