<p style="white-space: pre-wrap; word-wrap: break-word;">many comments aren't critical, except:</p><ul><li>I think the OA/DA IE structure needs to be flattened.</li><li>Use punctuation and other doxygen misc.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">couldn't resist to remark on some peculiarities that you are just taking over from other code around there, please ignore those...</p><p>Patch set 10:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/11069">View Change</a></p><p>25 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.h">File include/osmocom/gsm/gsup.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.h@94">Patch Set #10, Line 94:</a> <code style="font-family:monospace,monospace">      OSMO_GSUP_SM_RP_MR_IE                   = 0x40,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh ok, we really have "_IE" in the end then? well... ok then.</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.h@134">Patch Set #10, Line 134:</a> <code style="font-family:monospace,monospace">        OSMO_GSUP_MSGT_MO_FORWARD_SM_REQUEST    = 0b00100100,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">also interesting that we define the message type in binary... ok then</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.h@235">Patch Set #10, Line 235:</a> <code style="font-family:monospace,monospace">     * Please note that there is no SM-RP-MR in TCAP/MAP! SM-RP-MR</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">in doxygen this becomes</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  SM-RP-MR (see 3GPP TS 29.002, 7.6.1.1), Message Reference Please note that...</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Please use punctuation!!</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.h@237">Patch Set #10, Line 237:</a> <code style="font-family:monospace,monospace">     const uint8_t                   *sm_rp_mr;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh wow, so we don't inline uint8_t[] then? hmm. ok. ok then...</p><p style="white-space: pre-wrap; word-wrap: break-word;">and we use tabs... well then</p></li></ul></li><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 style="white-space: pre-wrap; word-wrap: break-word;">isn't "SMS_SM" redundant? could it be just OSMO_GSUP_SMS_ or just OSMO_GSUP_SM_?</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@25">Patch Set #10, Line 25:</a> <code style="font-family:monospace,monospace">/* Forward declarations (to avoid mutual include) */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">yes, that's what they are, and I think all C programmers should be aware of that concept?</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 style="white-space: pre-wrap; word-wrap: break-word;">a) I see that in the function name; unless you also explain "SM", "RP" or "DA", you might as well drop the entire comment. (I would opt for dropping cruft)</p><p style="white-space: pre-wrap; word-wrap: break-word;">b) place function comments in the .c file plz</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 style="white-space: pre-wrap; word-wrap: break-word;">(rather place each var on its own line)</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 style="white-space: pre-wrap; word-wrap: break-word;">(either line up with '(' or use a single tab as indent)</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@33">Patch Set #10, Line 33:</a> <code style="font-family:monospace,monospace"> *  SMS (Short Message Service) extensions for Osmocom GSUP</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">please please use punctuation to end the summary line.</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@35">Patch Set #10, Line 35:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I think we usually write in one line</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  /*! Foo yada</pre><p style="white-space: pre-wrap; word-wrap: break-word;">but then again I think Linus Torvalds referred to that as brain damaged once, so whatever.)</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@37">Patch Set #10, Line 37:</a> <code style="font-family:monospace,monospace"> * Encode SM-RP-DA IE (see 7.6.8.1), Destination Address</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">please please use punctuation to end the summary line.</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@43">Patch Set #10, Line 43:</a> <code style="font-family:monospace,monospace">       const struct osmo_gsup_message *gsup_msg)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I guess this fits on a 120 wide line?)</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@53">Patch Set #10, Line 53:</a> <code style="font-family:monospace,monospace">        /*! Special case for noSM-RP-DA and noSM-RP-OA */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">i doubt code inline doxygen makes sense. it might even end up in above function doc block?</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@62">Patch Set #10, Line 62:</a> <code style="font-family:monospace,monospace">                     "(type=0x%02x)!\n", gsup_msg->sm_rp_da_type);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(also looks like comfortable fit for 120 width, at least for the char constant)</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@74">Patch Set #10, Line 74:</a> <code style="font-family:monospace,monospace">       ie_len = msg->tail + 1; /* To be calculated later */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">max recently added API for this, see msgb_tl_put()</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 style="white-space: pre-wrap; word-wrap: break-word;">so ... this looks like this now?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  [DA_IE, len, [DA_TYPE, len, DA data...] ]</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Can this instead remain a flat layer of TLV structures? There are two nested layers, I think we should avoid that. Otherwise we are opening up corner cases, and both len fields need to be checked: the first one implicitly by our TLV decoder (nice), and the second one manually after having retrieved the first V (sigh)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Define the IE as</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">   [ DA_IE, len, TYPE, data ]</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and you can derive data_len = overall_len - sizeof(TYPE), and you're already sure that this len doesn't break the TLV structure.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Furthermore, if this will ever be likely to need extension, there should be a reserved type value that indicates a differing structure. Like, type = 0xff indicates a format to be defined in the future. Inner TLVs? That would be quite ugly, but if we can officially reserve one type value for that now (which we can still get for free), we might be glad in the future. Then maybe NULL could be 0xfe...</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@88">Patch Set #10, Line 88:</a> <code style="font-family:monospace,monospace"> * Decode SM-RP-DA IE (see 7.6.8.1), Destination Address</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">.</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@125">Patch Set #10, Line 125:</a> <code style="font-family:monospace,monospace"> /*! Special case for noSM-RP-DA and noSM-RP-OA */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">.</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@142">Patch Set #10, Line 142:</a> <code style="font-family:monospace,monospace">    gsup_msg->sm_rp_da_len = id_len;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">you are failing to check that the inner len does not surpass the outer TLV. Hence flatten please and this becomes unnecessary: define the IE as</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">   [ DA_IE, len, TYPE, data ]</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and you can derive data_len = overall_len - sizeof(TYPE), and you're already sure that this len doesn't break the TLV structure.</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@163">Patch Set #10, Line 163:</a> <code style="font-family:monospace,monospace">  /*! Special case for noSM-RP-DA and noSM-RP-OA */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">.</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@189">Patch Set #10, Line 189:</a> <code style="font-family:monospace,monospace">            gsup_msg->sm_rp_oa_len, gsup_msg->sm_rp_oa);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(same)</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@251">Patch Set #10, Line 251:</a> <code style="font-family:monospace,monospace">      gsup_msg->sm_rp_oa_len = id_len;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(same)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11069/10/tests/gsup/gsup_test.c">File tests/gsup/gsup_test.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/tests/gsup/gsup_test.c@231">Patch Set #10, Line 231:</a> <code style="font-family:monospace,monospace">                    0xff, 0x00, /* Special case: noSM-RP-OA */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(again the inner len issue; would it still be valid with a nonzero len? would have to add tests...)</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/tests/gsup/gsup_test.c@247">Patch Set #10, Line 247:</a> <code style="font-family:monospace,monospace">                     0x03, 0x07, /* SMSC address */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(again the inner len issue; if you kept it this way you would have to add tests that assure a mismatching inner len results in parse error)</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: Mon, 10 Dec 2018 21:38:29 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>