<p>Patch set 2:<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/c/osmo-trx/+/17805">View Change</a></p><p>19 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2//COMMIT_MSG">Commit Message:</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-trx/+/17805/2//COMMIT_MSG@7">Patch Set #2, Line 7:</a> <code style="font-family:monospace,monospace">transceiver: get rid of the ctrl thread</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ctrl thread(s)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h">File Transceiver52M/Transceiver.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/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h@153">Patch Set #2, Line 153:</a> <code style="font-family:monospace,monospace">      strncpy(data,src,99);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">probably OSMO_STRLCPY_ARRAY() ? Also leave spaes after each comma.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, I'm wondering why do we want a struct holding only a char array.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h@160">Patch Set #2, Line 160:</a> <code style="font-family:monospace,monospace">  std::deque<ctrl_msg> txmsgqueue;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we have osmo_wqueue for that, check osmocom/core/write_queue.h, there's plenty of similar code for osmo_fd.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h@272">Patch Set #2, Line 272:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This empty line can be dropped.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp">File Transceiver52M/Transceiver.cpp:</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-trx/+/17805/2/Transceiver52M/Transceiver.cpp@a161">Patch Set #2, Line 161:</a> <code style="font-family:monospace,monospace">    if (mCtrlSockets[i] >= 0)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So we don't clean control sockets anymore?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@167">Patch Set #2, Line 167:</a> <code style="font-family:monospace,monospace">    if (flags & BSC_FD_READ)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">required version for osmo-trx most probably allready contains OSMO_FD_READ and OSMO_FD_WRITE, so better use those. (BSC_FD_READ/WRITE are deprecated and only kept for some old code).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@170">Patch Set #2, Line 170:</a> <code style="font-family:monospace,monospace">        return rc;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">aren't you misisng something like osmo_signal_dispatch(...) here to request stop? And then I'd osmo_fd_unregister() since we are gonna exit anyway.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@206">Patch Set #2, Line 206:</a> <code style="font-family:monospace,monospace">  mCtrlSockets.resize(mChans);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">open question: how do we now during destructor which ofds are valid if you don't set all ofds to -1 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-trx/+/17805/2/Transceiver52M/Transceiver.cpp@221">Patch Set #2, Line 221:</a> <code style="font-family:monospace,monospace">                    mLocalAddr.c_str(), mBasePort,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Non related and not the type of padding we use in osmocom in general, drop it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@233">Patch Set #2, Line 233:</a> <code style="font-family:monospace,monospace">    int rv = osmo_sock_init2_ofd(&mCtrlSockets[i].conn_bfd, AF_UNSPEC, SOCK_DGRAM, IPPROTO_UDP,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if possible define variables at the start of the function (or block)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@240">Patch Set #2, Line 240:</a> <code style="font-family:monospace,monospace">    mCtrlSockets[i].conn_bfd.cb = ctrl_sock_cb;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Preferred way to set these: osmo_fd_setup()</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@246">Patch Set #2, Line 246:</a> <code style="font-family:monospace,monospace">                      OSMO_SOCK_F_BIND | OSMO_SOCK_F_CONNECT);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">wrond indentation, and this chang eis anyway not needed, drop it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@247">Patch Set #2, Line 247:</a> <code style="font-family:monospace,monospace">    if (mDataSockets[i] < 0)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Good catch, but this is actually an unrelated change. Please submit it as a separate fix prior to this patch.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@250">Patch Set #2, Line 250:</a> <code style="font-family:monospace,monospace">    if (i && filler == FILLER_DUMMY)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This was added here but it's still a few lines below (263), so probably unintended.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@762">Patch Set #2, Line 762:</a> <code style="font-family:monospace,monospace">    conn_bfd = &transceiver->mCtrlSockets[chan].conn_bfd;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why not make the function non-static so we have direct access to the object?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@764">Patch Set #2, Line 764:</a> <code style="font-family:monospace,monospace">    if (conn_bfd->fd <= 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">How could this happen? do you have a scenario? Otherwise at most use OSMO_ASSERT.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@779">Patch Set #2, Line 779:</a> <code style="font-family:monospace,monospace">    while (transceiver->mCtrlSockets[chan].txmsgqueue.size()) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">check osmo_wqueue.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@817">Patch Set #2, Line 817:</a> <code style="font-family:monospace,monospace">  msgLen = read(bfd->fd, buffer, sizeof(cmd_received.data));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd keep a simple char* buffer or switch to msgb rather tan adding a struct 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-trx/+/17805/2/Transceiver52M/Transceiver.cpp@840">Patch Set #2, Line 840:</a> <code style="font-family:monospace,monospace">    transceiver->stop();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Let's make it non-static to avoid all these changes.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805">change 17805</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-trx/+/17805"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-trx </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I642a34451e1825eafecf71a902df916ccee7944c </div>
<div style="display:none"> Gerrit-Change-Number: 17805 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Hoernchen <ewild@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Hoernchen <ewild@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Sun, 12 Apr 2020 02:05:27 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>