<p style="white-space: pre-wrap; word-wrap: break-word;">YOu need to resolve the merging conflicts. Some of my comments are not really hardlines, just some ideas to improve the code.</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 style="white-space: pre-wrap; word-wrap: break-word;">is this a pointer to struct bssgp_ran_inf_rim_cont ?</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 style="white-space: pre-wrap; word-wrap: break-word;">I see lots of structs sharing half of the fields. What about adding a HDR struct or alike?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Even better, one struct with a union handling different msg types. It may also make sense to write it in primitive<->msgb alike code like we do in other protocol stacks.</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 style="white-space: pre-wrap; word-wrap: break-word;">this looks more like a function tbh.</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 style="white-space: pre-wrap; word-wrap: break-word;">same, would be a lot clearer (to read and ebug) having a static function. Let the compiler decide whether to inline or not.</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 style="white-space: pre-wrap; word-wrap: break-word;">this memset not needed afaict.</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><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (enc_rim_cont_common(cont) < 0)<br>   return -EINVAL;</pre></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 style="white-space: pre-wrap; word-wrap: break-word;">memset not needed afaict</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 style="white-space: pre-wrap; word-wrap: break-word;">coding sytle: else clause shoudl also have brackets if "if" clause have them.</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 style="white-space: pre-wrap; word-wrap: break-word;">You may need to cast this through intptr_t to avoid warnings.</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: Mon, 04 Jan 2021 09:33:03 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>