<p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/17805">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/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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">probably OSMO_STRLCPY_ARRAY() ? Also leave spaes after each comma. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">wrapping due to queue container</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we have osmo_wqueue for that, check osmocom/core/write_queue. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">can't we just use convenient stl containers instead of arcane stuff if we do c++?</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">So we don't clean control sockets anymore?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">no, because this is c++, and resources can clean up themselves upon destruction..</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">aren't you misisng something like osmo_signal_dispatch(... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">only a signal is missing, unregister happens upon destruction. as it turns out the return value of our select cb is never used....</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">open question: how do we now during destructor which ofds are valid if you don't set all ofds to -1  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">that got lost in the constructor.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Non related and not the type of padding we use in osmocom in general, drop it.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can't we agree on one way, instead of leaving files in mixed states?<br>This got changed while replacing my own tabs with spaces, but someone else previously committed a bunch of random tab spacing changes that got changed as well....</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This was added here but it's still a few lines below (263), so probably unintended.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">indeed, the one at the bottom has to go</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Why not make the function non-static so we have direct access to the object?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">right, forgot about that one after rearranging the 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@764">Patch Set #2, Line 764:</a> <code style="font-family:monospace,monospace">    if (conn_bfd->fd <= 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How could this happen? do you have a scenario? Otherwise at most use OSMO_ASSERT.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">having a transceiver and a valid channel id does not imply a valid fd, it might be closed, or not open yet - although the way this is currently used makes this impossible. I've had a previous version with a single ctrl thread and more complicated issues.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'd keep a simple char* buffer or switch to msgb rather tan adding a struct here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">see header for reason, a simple struct wrap of a char array is still far less complicated than the whole msgb cruft that adds nothing of value.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Let's make it non-static to avoid all these changes.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is c++ and we can't have callbacks from somewhere for member functions, where would they get their this from? this is called from the socket cb.</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 06:51:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>