<p>Patch set 3:<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/12349">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12349/2/include/osmocom/msc/gsm_04_08.h">File include/osmocom/msc/gsm_04_08.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/12349/2/include/osmocom/msc/gsm_04_08.h@80">Patch Set #2, Line 80:</a> <code style="font-family:monospace,monospace">int gsm48_conn_sendmsg(struct msgb *msg, struct ran_conn *conn, struct gsm_trans *trans);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Yes that works, too. Done in next patch set.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually in .h files, in general, please rather define opaque structs.<br>Headers grow less dependency spaghetti and side effects if we just define opaque structs.<br>#include ran_conn.h would rather belong in the .c file.</p><p style="white-space: pre-wrap; word-wrap: break-word;">It's not really a problem, no need to change it back again.<br>But for next time: @Max, an opaque struct is good, not bad; and @stsp, next time refuse to #include lots of stuff if all you need is a single struct pointer.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12349/2/src/libmsc/a_iface_bssap.c">File src/libmsc/a_iface_bssap.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/12349/2/src/libmsc/a_iface_bssap.c@410">Patch Set #2, Line 410:</a> <code style="font-family:monospace,monospace"> int supported;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This was copied from existing code in gsm_04_08.c. I'll fix it there, too.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">it's because there is A5/0 .. A5/7. That's eight. This is a fact throughout the encryption code. BTW, A5/4...7 don't even exist, only as placeholders / defined bits in the specs.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c">File src/libmsc/a_iface_bssap.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/12349/3/src/libmsc/a_iface_bssap.c@409">Patch Set #3, Line 409:</a> <code style="font-family:monospace,monospace"> for (i = 0; i < ENCRY_INFO_PERM_ALGO_MAXLEN; i++) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This loop wants to iterate the a5_encryption_mask, so it should actually be exactly i < 8.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This constant instead defines how many perm_algo[] items are allowed. Those are theoretically two different pairs of shoes, but incidentally they are the same. Consider, even though 8 encryption types exist, the amount of permitted algos passed in that bssmap ie could have been chosen as three, or five, or two...</p><p style="white-space: pre-wrap; word-wrap: break-word;">Right. Both are eight. So it comes down to the same thing. At least now you know what things mean.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@419">Patch Set #3, Line 419:</a> <code style="font-family:monospace,monospace"> ei.perm_algo[j++] = vlr_ciph_to_gsm0808_alg_id(i);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this here could instead make sure that j < ENCRY_INFO_PERM_ALGO_MAXLEN</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@426">Patch Set #3, Line 426:</a> <code style="font-family:monospace,monospace"> vlr_ciph = ei.perm_algo[ei.perm_algo_len - 1] - 1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">do we have a reverse function for vlr_ciph_to_gsm0808_alg_id()? The -1 might be correct, but it's a magic number here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@429">Patch Set #3, Line 429:</a> <code style="font-family:monospace,monospace"> LOGPCONN(conn, LOGL_NOTICE, "BSC didn't specify algorithm in CIHPER MODE COMPLETE; falling back to %s\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You are parsing the "Ciphering is complete" message. Ciphering is already in place between MS and BTS, it's not like you could fall back or anything. Either you know what cipher is being used, or you don't. But definitely ciphering is being used now.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@430">Patch Set #3, Line 430:</a> <code style="font-family:monospace,monospace"> get_value_string(vlr_ciph_names, vlr_ciph));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">just use vlr_ciph_name(vlr_ciph)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@440">Patch Set #3, Line 440:</a> <code style="font-family:monospace,monospace"> LOGPCONN(conn, LOGL_ERROR, "Unsupported encryption algorithm in CIHPER MODE COMPLETE: %s\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"Non-permitted"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@456">Patch Set #3, Line 456:</a> <code style="font-family:monospace,monospace"> ran_conn_cipher_mode_compl(conn, msg, vlr_ciph);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe this function needs to be taught to not pass a cipher algo to the MSC when the BTS didn't send an IE with the chosen cipher. This entire "which cipher is it" is merely informational, also towards the MSC. All this is in the end after all ciphering negotiation, and nothing can be changed about it at this point.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c">File src/libmsc/gsm_04_08.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/12349/3/src/libmsc/gsm_04_08.c@125">Patch Set #3, Line 125:</a> <code style="font-family:monospace,monospace">int gsm48_classmark_supports_a5(const struct gsm_classmark *cm, uint8_t a5)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">gsm48 is a prefix commonly used by old libosmocore API. Would prefer to not use that here as well.<br>Since there is only one kind of classmark, 'classmark_..' is fine even for a non-static function, it is still used only in the msc.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c@1613">Patch Set #3, Line 1613:</a> <code style="font-family:monospace,monospace"> for (i = 0; i < ENCRY_INFO_PERM_ALGO_MAXLEN; i++) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">again, 8 is accurate here, because of (1 << i), and this is not a gsm0808 array of perm algo bytes.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12349">change 12349</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/12349"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I3260bee43cfe135ebfc33c13aee3c4ba43466c81 </div>
<div style="display:none"> Gerrit-Change-Number: 12349 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <stsp@stsp.name> </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: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 21 Dec 2018 02:52:10 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>