<p>Patch set 7:<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/11827">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/11827/7/include/osmocom/gsm/gsm0808_utils.h">File include/osmocom/gsm/gsm0808_utils.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/11827/7/include/osmocom/gsm/gsm0808_utils.h@71">Patch Set #7, Line 71:</a> <code style="font-family:monospace,monospace"> uint8_t net[5];  /* Network ID, ITU-T Q.1902.3 */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">doxygen wise, these comments should be of the form</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  /*< doc */</pre><p style="white-space: pre-wrap; word-wrap: break-word;">because otherwise they end up being associated to the following item instead of the previous one.</p><p style="white-space: pre-wrap; word-wrap: break-word;">actually ... I guess they get dropped by doxygen...</p><p style="white-space: pre-wrap; word-wrap: break-word;">I also see the rest of this file not doing that, so I guess this is optional.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe we should just drop doxygen altogether...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808.c">File src/gsm/gsm0808.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/11827/7/src/gsm/gsm0808.c@408">Patch Set #7, Line 408:</a> <code style="font-family:monospace,monospace">/*! Create BSSMAP Global Call Reference</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">IIUC the GCR is some IE or IE content, so this should be a gsm0808_enc_gcr() function with similar semantics like the other gsm0808_enc_* functions. i.e. pass a target msgb to this function and make it append the GCR to that, instead of returning an entire msgb just containing the GCR.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808_utils.c">File src/gsm/gsm0808_utils.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/11827/7/src/gsm/gsm0808_utils.c@675">Patch Set #7, Line 675:</a> <code style="font-family:monospace,monospace">int gsm0808_dec_gcr(struct gsm0808_gcr *gcr, const struct msgb *msg)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm expecting that the GCR is some IE or IE content, so instead it would make more sense to pass {pointer,max_length} to this function and parse that instead of an entire msgb. The current code implies that we decode the same TLV structure all over for every single IE.</p><p style="white-space: pre-wrap; word-wrap: break-word;">See the other *_dec_* functions in this file.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is the decoding function in a different .c file than the encoding function? Let's put all of them at the bottom of all the existing gsm0808_{enc,dec} function definitions.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11827/7/src/gsm/gsm0808_utils.c@700">Patch Set #7, Line 700:</a> <code style="font-family:monospace,monospace">       if (buf[point] != 5) /* see Table B 2.1.9.2 */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(prefer comment above the 'if')</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c">File tests/gsm0808/gsm0808_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/11827/7/tests/gsm0808/gsm0808_test.c@137">Patch Set #7, Line 137:</a> <code style="font-family:monospace,monospace"> struct gsm0808_gcr g = { .net_len = 3, .node = 0xDEAD }, p = { 0 };</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(BTW, just writing '{}' is identical to '{ 0 }' and it also works with all other structs and arrays even if the first member is not an int)</p><p style="white-space: pre-wrap; word-wrap: break-word;">(normally we prefer one var def per line)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@141">Patch Set #7, Line 141:</a> <code style="font-family:monospace,monospace">    memset(g.net, 'U', g.net_len);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">... F. U? are you telling me something?? :)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@145">Patch Set #7, Line 145:</a> <code style="font-family:monospace,monospace">     msg->l3h = msgb_wrap_with_TL(msg, GSM0808_IE_GLOBAL_CALL_REF);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">seeing this, I believe the IE discriminator should also be part of gsm0808_enc_gcr().</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@149">Patch Set #7, Line 149:</a> <code style="font-family:monospace,monospace">        if (rc < 0)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(technically this should exit now, but nevermind)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11827/7/tests/gsm0808/gsm0808_test.c@1811">Patch Set #7, Line 1811:</a> <code style="font-family:monospace,monospace">     test_create_gcr();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">so the function definition is above test_create_reset(), but it gets called after it? why not just put the new test at the bottom, both def and invocation?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11827">change 11827</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/11827"/><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: Iee95aa4e5c056645b6cb5667e4a067097d52dfbf </div>
<div style="display:none"> Gerrit-Change-Number: 11827 </div>
<div style="display:none"> Gerrit-PatchSet: 7 </div>
<div style="display:none"> Gerrit-Owner: Max <msuraev@sysmocom.de> </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: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 21 Nov 2018 17:13:38 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>