<p style="white-space: pre-wrap; word-wrap: break-word;">build failure: you need to adjust the vty transcript test that failed. You added the 'callwaiting' vty option, and the transcript test didn't expect that to appear.</p><p>Patch set 1:<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/c/osmo-msc/+/15120">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15120/1//COMMIT_MSG">Commit Message:</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/15120/1//COMMIT_MSG@22">Patch Set #1, Line 22:</a> <code style="font-family:monospace,monospace">of call waiting.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">would be nice to briefly mention or reference issues of the unpredictable behavior</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15120/1/include/osmocom/msc/gsm_data.h">File include/osmocom/msc/gsm_data.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/15120/1/include/osmocom/msc/gsm_data.h@258">Patch Set #1, Line 258:</a> <code style="font-family:monospace,monospace"> uint8_t callwaiting;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rather use bool</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15120/1/include/osmocom/msc/transaction.h">File include/osmocom/msc/transaction.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/15120/1/include/osmocom/msc/transaction.h@142">Patch Set #1, Line 142:</a> <code style="font-family:monospace,monospace"> struct vlr_subscr *vsub);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Instead of adding this I would suggest the caller use</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> trans_find_by_type(msc_a_for_vsub(vsub, true), TRANS_CC);</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(to avoid API clutter)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c">File src/libmsc/gsm_04_08_cc.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/15120/1/src/libmsc/gsm_04_08_cc.c@1806">Patch Set #1, Line 1806:</a> <code style="font-family:monospace,monospace"> struct gsm_trans *_trans = NULL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Not sure about this, but I got a segfault (in other code path) while testing, wasn't sure if it was […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">as I read the code there shouldn't be a segfault, but it is better style to use a separate variable indeed.<br>Rather declare the separate variable at the start of the "if (!net->callwaiting)" scope because it isn't used elsewhere, and rather name it like existing_cc_trans or something to clarify.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1879">Patch Set #1, Line 1879:</a> <code style="font-family:monospace,monospace"> if (!net->callwaiting) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(put the trans var decl here)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1880">Patch Set #1, Line 1880:</a> <code style="font-family:monospace,monospace"> _trans = trans_find_cc_by_vsub(net, vsub);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(use trans_find_by_type() here, see below)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1886">Patch Set #1, Line 1886:</a> <code style="font-family:monospace,monospace"> _trans->cc.state,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">cosmetic: no need to log both the state number and the gsm48_cc_state_name(), just the name is enough</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1894">Patch Set #1, Line 1894:</a> <code style="font-family:monospace,monospace"> if (!vsub->lu_complete) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">would make sense to keep this check before the callwaiting check. It's higher up the priority of reject reasons. so... (read below)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1902">Patch Set #1, Line 1902:</a> <code style="font-family:monospace,monospace"> }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;"><-- MARKER (see below)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15120/1/src/libmsc/gsm_04_08_cc.c@1917">Patch Set #1, Line 1917:</a> <code style="font-family:monospace,monospace"> msc_a = msc_a_for_vsub(vsub, true);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">and here is an msc_a already.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I would suggest:</p><ul><li>move this msc_a = msc_a_for_vsub() up to "MARKER" above.</li><li>put the net->callwaiting check right below that</li><li>use trans_find_by_type(msc_a, TRANS_CC) instead of adding API</li></ul></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15120/1/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/15120/1/src/libmsc/msc_vty.c@360">Patch Set #1, Line 360:</a> <code style="font-family:monospace,monospace"> vty_out(vty, " callwaiting%s", VTY_NEWLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(we often don't write back the default, so could only write 'no callwaiting' in case it is false)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-msc/+/15120">change 15120</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/c/osmo-msc/+/15120"/><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-Change-Id: I3eb6f23f7103e3002874fb5d3a30c9de952202ae </div>
<div style="display:none"> Gerrit-Change-Number: 15120 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 08 Aug 2019 14:19:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>