<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Build Started https://jenkins.osmocom.org/jenkins/job/gerrit-libosmocore/1287/</p></blockquote><p><a href="https://gerrit.osmocom.org/12199">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/12199/1/src/gsm/gsm29118.c">File src/gsm/gsm29118.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/12199/1/src/gsm/gsm29118.c@186">Patch Set #1, Line 186:</a> <code style="font-family:monospace,monospace">/* Allocate an empty message buffer, suitable to hold a complete SGsAP msg. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">typo: complete?</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/12199/1/src/gsm/gsm29118.c@191">Patch Set #1, Line 191:</a> <code style="font-family:monospace,monospace">     return msgb_alloc_headroom(1024, 512, "SGsAP");</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So 512 seems far more reasonable, right?</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/12199/1/src/gsm/gsm29118.c@207">Patch Set #1, Line 207:</a> <code style="font-family:monospace,monospace">           return -1;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Probably makes sense to at least print an error if strlen(name) > 55 in this case.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we should try to avoid error printing here since we do not have any real reference here. I think its better to have a return code so that the caller can know that an error occurred.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@219">Patch Set #1, Line 219:</a> <code style="font-family:monospace,monospace">    /* skip first two bytes (tag+length) so we can use msgb_tlv_put */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why not using msgb_put() if we already have the whole TLV in buf? because the TAG is different?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes its, different, we need to chop it off and use the SGSAP tag.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12199/1/src/gsm/gsm29118.c@227">Patch Set #1, Line 227:</a> <code style="font-family:monospace,monospace">      gsm48_generate_lai2(&lai_enc, lai);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">sizeof(lai_enc) instead of 5 probably?</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/12199/1/src/gsm/gsm29118.c@251">Patch Set #1, Line 251:</a> <code style="font-family:monospace,monospace"> *  \returns callee-allocated msgb with the encoded message. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">does it make sense to require a msgb here API-wise? probably a pointer + len makes more sense. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">In our case it makes sense because in the MSC we already have the data ready as a message buffer. I think this is fine.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12199/2/src/gsm/gsm29118.c">File src/gsm/gsm29118.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/12199/2/src/gsm/gsm29118.c@207">Patch Set #2, Line 207:</a> <code style="font-family:monospace,monospace">         return -1;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">setting a a local value then returning -1? That makes no sense.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for catching that. I have hopefully fixed it now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12199/2/src/gsm/gsm29118.c@222">Patch Set #2, Line 222:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">why not using msgb_put() if we already have the whole TLV in buf? because the TAG is different?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, its different. We need to chop it off and use the SGSAP tag. (I responded to this earler, gerrit is swallowing messages again?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12199/2/src/gsm/gsm29118.c@254">Patch Set #2, Line 254:</a> <code style="font-family:monospace,monospace">    struct msgb *msg = gsm29118_msgb_alloc();</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">does it make sense to require a msgb here API-wise? probably a pointer + len makes more sense. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">In osmo-msc we already have the nas_msg as a message buffer, so it makes sense to use a message buffer here as well.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12199">change 12199</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/12199"/><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: Ic87f8a771b87b52215d0a7451b67794557b80b8a </div>
<div style="display:none"> Gerrit-Change-Number: 12199 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 07 Dec 2018 17:35:49 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>