<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">TBH I don't see a problem with also checking for osmux support on RANAP, even if no HNGBW or whatever doesn't support it yet, it may in the future.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">That's exactly why there is a supports_osmux param in ranap_is_reset_msg().</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">That being said, if you think it's more convenient or there's any benefit in doing it this way for whatever reason then fine with me.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">It sounds like you don't acknowledge the big problem of feeding a RANAP message to the gsm0808 TLV decoding functions. That is just plain insane, and that comes from doing message decoding in ran_peer.c where it doesn't belong at all. The "whatever reason" is absolute basic sanity of keeping completely different RAN message coding in the proper places :P</p><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/19024">View Change</a></p><p>5 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/+/19024/2/src/libmsc/ran_msg_a.c">File src/libmsc/ran_msg_a.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/+/19024/2/src/libmsc/ran_msg_a.c@1283">Patch Set #2, Line 1283:</a> <code style="font-family:monospace,monospace">    if (supports_osmux == NULL)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">simply check supports_osmux in bssmap_is_reset_msg()? What abnout simply returning a bool?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">then i'd have to do that check twice.</p><p style="white-space: pre-wrap; word-wrap: break-word;">bool: I want to also reflect a "don't know" case where neither support nor non-support are certain, for non-RESET related messages. Hence -1, 0, 1 rc. The caller can then simply handle the supports_osmux rc without having to convolute with the returned enum reset_msg_type.</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/+/19024/2/src/libmsc/ran_msg_a.c@1286">Patch Set #2, Line 1286:</a> <code style="font-family:monospace,monospace"> rc = tlv_parse(&tp, gsm0808_att_tlvdef(), msgb_l3(msg) + 1, msgb_l3len(msg) - 1, 0, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(answering below comment here)</p><p style="white-space: pre-wrap; word-wrap: break-word;">I was going back and forth on modifying msg->l3h.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I had already written</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  const char *l3h = msgb_l2(msg) + sizeof(struct bssmap_header);<br>  int l3_len = msgb_l2len(msg) - sizeof(struct bssmap_header);<br>  rc = tlv_parse(..., l3h + 1, l3_len - 1, ...);</pre><p style="white-space: pre-wrap; word-wrap: break-word;">But dropped that again because IMHO we should rather use the proper msgb_l3len() definitions than copying the calculation code.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also when receiving a BSSMAP message, setting l3h to the right location is the right thing to do. Since this code is called in code paths where we anyway set l3h everywhere else, it is a bit ridiculous to require a const msgb instead of using the proper msgb macros.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I also considered putting the l3h modification outside of *_is_reset_msg() but that doesn't fit well, because we're doing msg size plausibility checks below in bssmap_is_reset_msg(), and setting l3h only makes sense after those checks passed.</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/+/19024/2/src/libmsc/ran_msg_a.c@1314">Patch Set #2, Line 1314:</a> <code style="font-family:monospace,monospace"> l2->l3h = msgb_l2(l2) + sizeof(struct bssmap_header);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ack</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">(answer for this is above)</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/+/19024/2/src/libmsc/ran_msg_a.c@1316">Patch Set #2, Line 1316:</a> <code style="font-family:monospace,monospace">    switch (l2->l2h[sizeof(*bs)]) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">...and I should actually also do msgb_l3(l2)[0] here</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/19024/2/src/libmsc/ran_msg_iu.c">File src/libmsc/ran_msg_iu.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/+/19024/2/src/libmsc/ran_msg_iu.c@447">Patch Set #2, Line 447:</a> <code style="font-family:monospace,monospace">     if (supports_osmux != NULL)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You are using "if (supports_osmux)" on the other, you could use it too here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh you mean writing "x != NULL" vs just "x" ... well, does it matter?<br>(usually I'm all for 'if (x)' but here 'if (supports_osmux)' reads like we're analysing the flag, and '!= NULL' sort of hints that it is a pointer to the flag instead; so I'm changing the other one)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-msc/+/19024">change 19024</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/+/19024"/><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: I1ad4a3f9356216dd4bf8c48fba29fd23438810a7 </div>
<div style="display:none"> Gerrit-Change-Number: 19024 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 01 Jul 2020 14:51:34 +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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>