<p><a href="https://gerrit.osmocom.org/11069">View Change</a></p><p>23 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@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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">in doxygen this becomes […]</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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">oh wow, so we don't inline uint8_t[] then? hmm. ok. ok then... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Sorry, what is this comment / question about? Alignment?<br>How is this comment related to this change?</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">OSMO_GSUP_SMS is the gsup-sub-type SMS […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Harald is correct here, so ignored.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">yes, that's what they are, and I think all C programmers should be aware of that concept?</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/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;">Removed</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;">Ignored. Why not?</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;">I actually followed the style of msgb_tlv_put() statements<br>above. We can unify the alignment in a separate path.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">please please use punctuation to end the summary line.</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/11069/10/src/gsm/gsup_sms.c@35">Patch Set #10, Line 35:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(I think we usually write in one line […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Agree, fixed.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">please please use punctuation to end the summary line.</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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(I guess this fits on a 120 wide line?)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not critical, ignored.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">i doubt code inline doxygen makes sense. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Copy-pasted comment. Fixed, thanks!</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@55">Patch Set #10, Line 55:</a> <code style="font-family:monospace,monospace">msgb_tv_put</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think we should introduce anything but TLV in GSUP. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I was actually abusing this function in order to put TL of the final TLTLV.<br>Anyway, this doesn't make sense anymore, please see the new patch.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(also looks like comfortable fit for 120 width, at least for the char constant)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not critical, ignored.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">max recently added API for this, see msgb_tl_put()</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Good to know, thanks!</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;">I'm sure we have examples of nested IEs in other cases / protocol layers (like TS 12. […]</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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">.</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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Copy pasted comment. Thanks, fixed!</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">you are failing to check that the inner len does not surpass the outer TLV. […]</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/11069/10/src/gsm/gsup_sms.c@147">Patch Set #10, Line 147:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/*!<br> * Encode SM-RP-OA IE (see 7.6.8.2), Originating Address<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Fixed too.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Copy pasted comment. Thanks, fixed!</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@197">Patch Set #10, Line 197:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/*!<br> * Decode SM-RP-OA IE (see 7.6.8.2), Originating Address<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Fixed too.</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@234">Patch Set #10, Line 234:</a> <code style="font-family:monospace,monospace">/*! </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Fixed.</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: Fri, 14 Dec 2018 00:26:39 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>