<p>Patch set 10:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ddd;">-Code-Review</span></p><p><a href="https://gerrit.osmocom.org/11069">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h">File include/osmocom/gsm/gsup_sms.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/11069/10/include/osmocom/gsm/gsup_sms.h@17">Patch Set #10, Line 17:</a> <code style="font-family:monospace,monospace"> OSMO_GSUP_SMS_SM_RP_ODA_NONE = 0x00,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">isn't "SMS_SM" redundant? could it be just OSMO_GSUP_SMS_ or just OSMO_GSUP_SM_?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">OSMO_GSUP_SMS is the gsup-sub-type SMS<br>SM_RP_ODA is the 3GPP part for the SM- sub-layer. Everything fine here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11069/10/include/osmocom/gsm/gsup_sms.h@29">Patch Set #10, Line 29:</a> <code style="font-family:monospace,monospace">/* SM-RP-DA IE coding functions */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">a) I see that in the function name; unless you also explain "SM", "RP" or "DA", you might as well dr […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The assumption here is probably that the potential reader is familiar with the SMS system as specified by 3GPP (and hence knows SM-RP-DA), but he may not be familiar with the osmocom specific GSUP protocol.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c">File src/gsm/gsup.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/11069/10/src/gsm/gsup.c@569">Patch Set #10, Line 569:</a> <code style="font-family:monospace,monospace"> int idx, rc;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(rather place each var on its own line)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">we never had any such rule</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup.c@668">Patch Set #10, Line 668:</a> <code style="font-family:monospace,monospace"> sizeof(u8), gsup_msg->sm_rp_mr);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(either line up with '(' or use a single tab as indent)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">we also never enforced a rule like this. I'm sure there are plenty of examples that look like this.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c">File src/gsm/gsup_sms.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/11069/10/src/gsm/gsup_sms.c@55">Patch Set #10, Line 55:</a> <code style="font-family:monospace,monospace">msgb_tv_put</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think we should introduce anything but TLV in GSUP. Having the IEI determine if it's a TV/TLV/TL16V etc. is quite ugly and we shouldn't replicate this in our own protocols. Let's have each IE be a TLV.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11069/10/src/gsm/gsup_sms.c@82">Patch Set #10, Line 82:</a> <code style="font-family:monospace,monospace"> *ie_len = msg->tail - (ie_len + 1);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">so ... this looks like this now? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm sure we have examples of nested IEs in other cases / protocol layers (like TS 12.21 Abis OML), in the xUA routing key management of osmo-stp. Not in GSUP so far, but I don't understand your strong opposition to it. </p><p style="white-space: pre-wrap; word-wrap: break-word;">What I do understand is that the current proposed code by vadim isn't nice. Whatever the solutoin (nested or not), there must be proper helper functions that avoid manual pointer calculations like the lines above.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11069">change 11069</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/11069"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ibe325c64ae2d6c626b232533bb4cbc65fc2b5d71 </div>
<div style="display:none"> Gerrit-Change-Number: 11069 </div>
<div style="display:none"> Gerrit-PatchSet: 10 </div>
<div style="display:none"> Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 13 Dec 2018 10:21:03 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>