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

historical

pespin 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: Code-Review-1

(19 comments)

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2//COMMIT_MSG 
Commit Message:

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2//COMMIT_MSG@7 
PS2, Line 7: transceiver: get rid of the ctrl thread
ctrl thread(s)

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.

Also, I'm wondering why do we want a struct holding only a char array.

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.h, there's plenty of similar code for osmo_fd.

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.h@272 
PS2, Line 272: 
This empty line can be dropped.

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?

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@167 
PS2, Line 167:     if (flags & BSC_FD_READ)
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).

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(...) here to request stop? And then I'd osmo_fd_unregister() since we are gonna exit anyway.

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 here?

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.

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@233 
PS2, Line 233:     int rv = osmo_sock_init2_ofd(&mCtrlSockets[i].conn_bfd, AF_UNSPEC, SOCK_DGRAM, IPPROTO_UDP,
if possible define variables at the start of the function (or block)

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@240 
PS2, Line 240:     mCtrlSockets[i].conn_bfd.cb = ctrl_sock_cb;
Preferred way to set these: osmo_fd_setup()

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@246 
PS2, Line 246:                       OSMO_SOCK_F_BIND | OSMO_SOCK_F_CONNECT);
wrond indentation, and this chang eis anyway not needed, drop it.

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@247 
PS2, Line 247:     if (mDataSockets[i] < 0)
Good catch, but this is actually an unrelated change. Please submit it as a separate fix prior to this patch.

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.

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?

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.

https://gerrit.osmocom.org/c/osmo-trx/+/17805/2/Transceiver52M/Transceiver.cpp@779 
PS2, Line 779:     while (transceiver->mCtrlSockets[chan].txmsgqueue.size()) {
check osmo_wqueue.

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.

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.

-- 
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 02:05:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200412/3601b245/attachment.htm>