Change in osmo-trx[master]: transceiver: get rid of the ctrl thread

[Hoernchen]

Hoernchen has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/17805 )

Change subject: transceiver: get rid of the ctrl thread
......................................................................


Patch Set 2:

(11 comments)

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h 
File Transceiver52M/Transceiver.h:

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h@153 
PS2, Line 153: 	  strncpy(data,src,99);
> probably OSMO_STRLCPY_ARRAY() ? Also leave spaes after each comma. […]
wrapping due to queue container


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h@160 
PS2, Line 160:   std::deque<ctrl_msg> txmsgqueue;
> we have osmo_wqueue for that, check osmocom/core/write_queue. […]
can't we just use convenient stl containers instead of arcane stuff if we do c++?


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp 
File Transceiver52M/Transceiver.cpp:

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@a161 
PS2, Line 161:     if (mCtrlSockets[i] >= 0)
> So we don't clean control sockets anymore?
no, because this is c++, and resources can clean up themselves upon destruction..


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@170 
PS2, Line 170:         return rc;
> aren't you misisng something like osmo_signal_dispatch(... […]
only a signal is missing, unregister happens upon destruction. as it turns out the return value of our select cb is never used....


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@206 
PS2, Line 206:   mCtrlSockets.resize(mChans);
> open question: how do we now during destructor which ofds are valid if you don't set all ofds to -1  […]
that got lost in the constructor.


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@221 
PS2, Line 221:                     mLocalAddr.c_str(), mBasePort,
> Non related and not the type of padding we use in osmocom in general, drop it.
Can't we agree on one way, instead of leaving files in mixed states?
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....


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@250 
PS2, Line 250:     if (i && filler == FILLER_DUMMY)
> This was added here but it's still a few lines below (263), so probably unintended.
indeed, the one at the bottom has to go


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@762 
PS2, Line 762:     conn_bfd = &transceiver->mCtrlSockets[chan].conn_bfd;
> Why not make the function non-static so we have direct access to the object?
right, forgot about that one after rearranging the code.


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@764 
PS2, Line 764:     if (conn_bfd->fd <= 0) {
> How could this happen? do you have a scenario? Otherwise at most use OSMO_ASSERT.
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.


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@817 
PS2, Line 817:   msgLen = read(bfd->fd, buffer, sizeof(cmd_received.data));
> I'd keep a simple char* buffer or switch to msgb rather tan adding a struct here.
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.


https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@840 
PS2, Line 840:     transceiver->stop();
> Let's make it non-static to avoid all these changes.
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.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/17805
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: I642a34451e1825eafecf71a902df916ccee7944c
Gerrit-Change-Number: 17805
Gerrit-PatchSet: 2
Gerrit-Owner: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Sun, 12 Apr 2020 06:51:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200412/a5cbd6f3/attachment.htm>