<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/20334">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c">File src/gsm/bssmap_le.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/libosmocore/+/20334/1/src/gsm/bssmap_le.c@317">Patch Set #1, Line 317:</a> <code style="font-family:monospace,monospace">#define PARSE_ERR(ERRMSG) \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">meh, that is not realistic. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">the point is that the user of a function is the calling function. And a calling function (maybe not today, but tomorrow) may want to take decisions baesd on the type of error. For example, in order to encode the type of error into another protocol (a "cause" value), etc. For sure one doesn't want to !strcmp() the error string in order to find that out.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We do have code that returns "-cause" values in the respective protocol, if it has sufficient cause values. If it doesn't, you either need an enum, or simply log the error directly while returning -1 or -EINVAL or whatever.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is yet another example where some completely new concept/pattern is introduced, inconsistent with any of the existing code - despite it doing the same as much of our existing code: decoding some protocol. And that concept is implemented all over the Lb code fist, in many occasions, without first giving the team to provide some fedback. Rather, at the end of some weeks a large patch bomb is released and we discover it now. Putting all of us in the sad situation of either accepting this inconsistent approach, or investing additional time to convert it. I really feel sorry for all sides here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c@335">Patch Set #1, Line 335:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">msgb_put_u8(msg, OSMO_BSSMAP_LE_IEI_APDU);<br> l = msgb_put(msg, 2);<br> old_tail = msg->tail;<br> msgb_put_u8(msg, OSMO_BSSMAP_LE_APDU_PROT_BSSLAP);<br> int rc = osmo_bsslap_enc(msg, bsslap);<br> if (rc <= 0)<br> return -EINVAL;<br> osmo_store16be(msg->tail - old_tail, l);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">no, because the IE is appended to existing data</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/20334/1/src/gsm/bssmap_le.c@349">Patch Set #1, Line 349:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br> if (len < 1)<br> return "APDU too short";<br><br> proto = data[0];<br><br> switch (proto) {<br> case OSMO_BSSMAP_LE_APDU_PROT_BSSLAP:<br> return osmo_bsslap_dec(bsslap, data + 1, len - 1);<br> default:<br> return "Unimplemented APDU type<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">i prefer strings!</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">see my explanation in other locations. We are not writing a protocol stringifire or something like wireshark where the consumer is a human being. We write protocol decodesr in a library so that other library or application code can react on the results, and hence programmatic, machine-readable results are required. If there is human-readable output, it is optionally in addition, either logged directly or generatd fromt the machine-readable code.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/20334">change 20334</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/libosmocore/+/20334"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I271e59b794bafc0a7ae0eabbf58918f6d7df431d </div>
<div style="display:none"> Gerrit-Change-Number: 20334 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </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: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 01 Oct 2020 18:11:56 +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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>