<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/20334">View Change</a></p><p>1 comment:</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;">the point is that the user of a function is the calling function. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would like to log the errors, but we cannot log from here. We don't even know whether the caller initialized logging.</p><p style="white-space: pre-wrap; word-wrap: break-word;">it's not so bad to adjust the error returning if needed. but I thought it would be a drastic improvement compared to how we parse BSSMAP (gsm0808.h). For BSSMAP, every caller does their own tlv parsing and decoding, and logs strings also duplicated by every caller. The argument that we usually return an rc doesn't really count for BSSMAP because we simply don't provide a common decoding API at all, besides the TLV structure definition.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This patch puts the parsing into one common implementation (next to encoding where it belongs) and returns the strings that the caller can then log. No functional difference to how e.g. osmo-bsc parses BSSMAP, except we avoid code dup spread across osmo-bsc and osmo-smlc.</p><p style="white-space: pre-wrap; word-wrap: break-word;">But I get that you see it differently. I could go on arguing that this API does not provide less but more useful information than usually, but let's just move forward.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I can change this to just returing -EINVAL for each parsing error, how we usually do for parsing -- but that would be essentially identical to testing whether the returned char* is non-NULL, just a single-bit success flag.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Or, I think nicest would be to return the IEI where the error occured, which would allow easy logging of at least the rough location of a parsing error. We could also define that the message type is returned in the out-parameter pdu struct to allow relatively easy logging like</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  LOGP("error in msg %s IE %s", osmo_bssmap_le_msg_type(pdu->msg_type), osmo_bssmap_le_iei_name(rc)</pre><p style="white-space: pre-wrap; word-wrap: break-word;">We could also return integer codes, but make the error message an optional out-arg in osmo_bssap_le_dec(), which would make the error message just an extra feature for bssmap_le.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Another idea would be to add a parsing error struct to the PDU union, providing an rc and a string buffer, which would also allow composing strings with actual values read from the data (in this patch only string constants are possible).</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: 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: 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 21:46:29 +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>