<p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/include/osmocom/msc/mncc.h">File include/osmocom/msc/mncc.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/osmo-msc/+/13422/12/include/osmocom/msc/mncc.h@166">Patch Set #12, Line 166:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Need to bump the MNCC version.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">@laforge, this is the reason why this patch must remain WIP, it changes the MNCC socket protocol without bumping the revision (which should also be coordinated with osmo-sip-connector)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/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/osmo-msc/+/13422/12/include/osmocom/msc/transaction.h@135">Patch Set #12, Line 135:</a> <code style="font-family:monospace,monospace"> struct osmo_lcls *lcls;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this should go in the "cc" struct above, because it only concerns voice transactions</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/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/osmo-msc/+/13422/12/include/osmocom/msc/vlr.h@274">Patch Set #12, Line 274:</a> <code style="font-family:monospace,monospace"> bool lcls_enable;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">i don't see this flag being used anywhere. It gets initialized to 1 at some point but never has any effect, right? (also, the VLR is not the right place to put an LCLS flag)<br>I think we can drop the flag until a need arises...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/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/osmo-msc/+/13422/12/src/libmsc/gsm_04_08_cc.c@503">Patch Set #12, Line 503:</a> <code style="font-family:monospace,monospace"> trans->lcls = trans_lcls_compose(trans, true);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(if (!trans->lcls), log a comment that composing LCLS info failed?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/src/libmsc/gsm_04_08_cc.c@1984">Patch Set #12, Line 1984:</a> <code style="font-family:monospace,monospace"> trans->lcls->gcr = trans->cc.msg.gcr;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Might be worth placing in a comment here that this is happing as a result of Paging success, so we n […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">imho above comment is sufficient</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/src/libmsc/mncc_builtin.c">File src/libmsc/mncc_builtin.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/osmo-msc/+/13422/12/src/libmsc/mncc_builtin.c@72">Patch Set #12, Line 72:</a> <code style="font-family:monospace,monospace"> struct osmo_gcr_parsed gcr)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">setup->gcr == gcr, there should be no need to add another argument.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(BTW, passing a struct as argument must always be done as a pointer)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/src/libmsc/mncc_builtin.c@116">Patch Set #12, Line 116:</a> <code style="font-family:monospace,monospace"> setup->gcr = gcr;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">afaict the same gcr is already in there, just leave it up to the caller and not bother here</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/src/libmsc/mncc_builtin.c@310">Patch Set #12, Line 310:</a> <code style="font-family:monospace,monospace"> rc = mncc_setup_ind(call, arg, data->gcr);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">arg and data are pointers to the same MNCC struct, it shouldn't be necessary to add another arg</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422/12/src/libmsc/transaction.c">File src/libmsc/transaction.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/osmo-msc/+/13422/12/src/libmsc/transaction.c@117">Patch Set #12, Line 117:</a> <code style="font-family:monospace,monospace"> Can we get primary_pc elsewhere? */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Not sure this commment is still valid. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">i think this comment can be dropped, doesn't seem to convey any useful information</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-msc/+/13422">change 13422</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/+/13422"/><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: I35ae6b6ca04925c8d300bc1a0269af00eac727f3 </div>
<div style="display:none"> Gerrit-Change-Number: 13422 </div>
<div style="display:none"> Gerrit-PatchSet: 12 </div>
<div style="display:none"> Gerrit-Owner: Max <suraev@alumni.ntnu.no> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-CC: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 23 Dec 2020 20:31:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: keith <keith@rhizomatica.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>