<p style="white-space: pre-wrap; word-wrap: break-word;">I did have done the API changes (unions, full parse) now. However, I did not update the patch yet since I am not entirely done yet. There are basically two problems left:</p><p style="white-space: pre-wrap; word-wrap: break-word;">The encoder functions currently write to an uint8_t buffer but depending on how the API will be used an msg buffer might be better. I started working on osmo-pcu, there I will use the API, I first want to see how things turn out there. One big adavantage of msg buffers would also be that they have array bounds checking.</p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862">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/libosmocore/+/21862/5/include/osmocom/gprs/gprs_bssgp_rim.h">File include/osmocom/gprs/gprs_bssgp_rim.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/libosmocore/+/21862/5/include/osmocom/gprs/gprs_bssgp_rim.h@13">Patch Set #5, Line 13:</a> <code style="font-family:monospace,monospace">    const uint8_t *app_cont;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">is this a pointer to struct bssgp_ran_inf_rim_cont ?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">yes, but to its unparsed binary version. The memory location is inside *buf, which is given as parameter to the decoder function. So, when buf is freed there will be a problem, but I do not see any alternative. The strings can be up to 65535 (or 32767?, I am not sure), so reserving this statically memory is not a good option. Dynamic allocation is also not very attractive 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/+/21862/5/include/osmocom/gprs/gprs_bssgp_rim.h@73">Patch Set #5, Line 73:</a> <code style="font-family:monospace,monospace"> uint8_t app_id;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I see lots of structs sharing half of the fields. What about adding a HDR struct or alike? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I thought about having a header shared over all message types. However, when looking closely its always a bit different. We would have to put a header that satisfies every message / container type (bool xyz_present members) but then the structs do not match up with the spec anymore. I would like to keep it like this.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I have now put unions into the application container structs, so things become easier at least at that level. We could also add a "master" struct that unifies all possible rim container types, then we could have a single function that decodes/encodes everything. However I think this would be overkill since in the actual application we would still need If or switch depending on a discriminator. Then it is probably better to go by the TLV IEI on that level.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c">File src/gb/gprs_bssgp_rim.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/+/21862/5/src/gb/gprs_bssgp_rim.c@32">Patch Set #5, Line 32:</a> <code style="font-family:monospace,monospace">#define DEC_RIM_CONT_COMMON \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this looks more like a function tbh.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It was a function...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@51">Patch Set #5, Line 51:</a> <code style="font-family:monospace,monospace">#define ENC_RIM_CONT_COMMON \</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">same, would be a lot clearer (to read and ebug) having a static function. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If I do it that way I can only call the function with one struct type, so it does not work for me.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Its probably not that effective anyway. I think its only used in 3 locations anyway.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@68">Patch Set #5, Line 68:</a> <code style="font-family:monospace,monospace">  memset(cont, 0, sizeof(*cont));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">this memset not needed afaict.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Its not needed because I set the struct members to NULL / 0 in the else branches below. I think its better to remove the else branches and keep the memset.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@106">Patch Set #5, Line 106:</a> <code style="font-family:monospace,monospace">       ENC_RIM_CONT_COMMON</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">if (enc_rim_cont_common(cont) < 0) […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is all in the macro.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@127">Patch Set #5, Line 127:</a> <code style="font-family:monospace,monospace">        memset(cont, 0, sizeof(*cont));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">memset not needed afaict</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">see above</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@288">Patch Set #5, Line 288:</a> <code style="font-family:monospace,monospace">       } else</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">coding sytle: else clause shoudl also have brackets if "if" clause have them.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/21862/5/src/gb/gprs_bssgp_rim.c@514">Patch Set #5, Line 514:</a> <code style="font-family:monospace,monospace">      return (int)(buf_ptr - buf);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You may need to cast this through intptr_t to avoid warnings.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">like this: return (int)((intptr_t)buf_ptr - (intptr_t)buf) ?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/21862">change 21862</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/+/21862"/><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: Ibbc7fd67658e3040c12abb5706fe9d1f31894352 </div>
<div style="display:none"> Gerrit-Change-Number: 21862 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 06 Jan 2021 09:53:43 +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"> Gerrit-MessageType: comment </div>