<p style="white-space: pre-wrap; word-wrap: break-word;">I found a few more things, some less important than others...</p><p>Patch set 4:<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/11734">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/11734/4/src/osmo-bsc/handover_fsm.c">File src/osmo-bsc/handover_fsm.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/11734/4/src/osmo-bsc/handover_fsm.c@678">Patch Set #4, Line 678:</a> <code style="font-family:monospace,monospace">       struct gsm0808_handover_performed ho_perf_params;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">could just write '= {};' instead of the memset below</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@680">Patch Set #4, Line 680:</a> <code style="font-family:monospace,monospace">  struct gsm0808_speech_codec sc;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">maybe also = {} here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@689">Patch Set #4, Line 689:</a> <code style="font-family:monospace,monospace">   if (!cell)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no error message or anything? Please do some</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  LOG_HO(conn, LOGL_ERROR, "...")</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@692">Patch Set #4, Line 692:</a> <code style="font-family:monospace,monospace">    ho_perf_params.cell.id_discr = CELL_IDENT_WHOLE_GLOBAL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please use the proper union member, here id.global.<br>I prefer this notation:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  ho_perf_params.cell = (struct gsm0808_cell_id){<br>    .id_discr = CELL_IDENT_WHOLE_GLOBAL,<br>    .id.global = *cell,<br>  }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">because then you're always sure all unset items become zero.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(Also, I think a new version of cgi_for_msc() version should return a struct gsm0808_cell_id directly, again a note to self to fix it.)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@696">Patch Set #4, Line 696:</a> <code style="font-family:monospace,monospace">    if (!ho_perf_params.chosen_channel)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">LOG_HO(..,ERROR</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@719">Patch Set #4, Line 719:</a> <code style="font-family:monospace,monospace">     msg = gsm0808_create_handover_performed(&ho_perf_params);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">check msg for NULL and log error?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11734/4/src/osmo-bsc/handover_fsm.c@720">Patch Set #4, Line 720:</a> <code style="font-family:monospace,monospace"> gscon_sigtran_send(conn, msg);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">check rc and log error?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11734">change 11734</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/11734"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: If26e5807280e0f75a423b3b04f8e3c698c82a351 </div>
<div style="display:none"> Gerrit-Change-Number: 11734 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@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: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 27 Nov 2018 01:03:18 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>