<p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c">File src/libmsc/cc_sdp.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/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c@10">Patch Set #6, Line 10:</a> <code style="font-family:monospace,monospace">    /* In order of preference. TODO: make configurable */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Are we adding regression here? Wasn't it already configurable before?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c@11">Patch Set #6, Line 11:</a> <code style="font-family:monospace,monospace">  static const enum gsm48_bcap_speech_ver mobile_codecs[] = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure if it really makes sense having this static here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/cc_sdp.c@107">Patch Set #6, Line 107:</a> <code style="font-family:monospace,monospace">                         for (a->payload_type = 96; a->payload_type <= 127; a->payload_type++) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">would be great having defines for this numbers (96,127).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/codec_sdp_cc_t9n.c">File src/libmsc/codec_sdp_cc_t9n.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/osmo-msc/+/15953/6/src/libmsc/codec_sdp_cc_t9n.c@170">Patch Set #6, Line 170:</a> <code style="font-family:monospace,monospace">const struct codec_mapping *codec_mapping_by_gsm0808_speech_codec_type(enum gsm0808_speech_codec_type sct, uint16_t cfg)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">either fix the TODO or mark cfg as unused for the compiler.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/gsm_04_08_cc.c">File src/libmsc/gsm_04_08_cc.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/osmo-msc/+/15953/6/src/libmsc/gsm_04_08_cc.c@1261">Patch Set #6, Line 1261:</a> <code style="font-family:monospace,monospace">                                           GSM48_PDISC_CC | (trans->transaction_id << 4),</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">wrong indentation.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/src/libmsc/sdp_msg.c">File src/libmsc/sdp_msg.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/osmo-msc/+/15953/6/src/libmsc/sdp_msg.c@558">Patch Set #6, Line 558:</a> <code style="font-family:monospace,monospace">      if (codec->rate != 8000)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Next time, please split this kind of stuff into separate commits for easier review.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953/6/tests/msc_vlr/msc_vlr_test_call.err">File tests/msc_vlr/msc_vlr_test_call.err:</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/osmo-msc/+/15953/6/tests/msc_vlr/msc_vlr_test_call.err@429">Patch Set #6, Line 429:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This empty line in stderr (libosmocore log) looks suspìcious.As if you are adding an extra \n somewhere.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-msc/+/15953">change 15953</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/c/osmo-msc/+/15953"/><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-Change-Id: I8c3b2de53ffae4ec3a66b9dabf308c290a2c999f </div>
<div style="display:none"> Gerrit-Change-Number: 15953 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 02 Jun 2020 10:06:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>