Adjust code for AsteroidOSSync/pull/216 #1
Conversation
- Use Connman for managing the TAP interface - Rely on kernel packet fragmentation instead of a custom header - Respect the negotiated MTU when choosing the TAP MTU
| @@ -179,6 +179,6 @@ void BLE::onReceivedFromCompanion(const QByteArray &content) { | |||
| void BLE::onMtuChanged(int mtu) { | |||
| if (mCurrentMtu != mtu) { | |||
| mCurrentMtu = mtu; | |||
| emit mtuChanged(mtu); | |||
| emit mtuChanged(mtu - 3); | |||
There was a problem hiding this comment.
Could you explain the origin of this constant ? And extract it into a named macro maybe ?
There was a problem hiding this comment.
And squash thsi into the code that introduced it :)
asteroid-tap2ble.service
Outdated
| @@ -10,8 +10,6 @@ ExecStart=/usr/bin/asteroid-tap2ble | |||
| Restart=always | |||
| User=ceres | |||
| Group=ceres | |||
| AmbientCapabilities=CAP_NET_ADMIN | |||
| NoNewPrivileges=true | |||
There was a problem hiding this comment.
It's awkward to introduce those lines and delete them as part of the same PR. Could you squash those changes into one commit ?
| @@ -1,4 +1,16 @@ | |||
| # asteroid-tap2ble | |||
| asteroid-tap2ble | |||
There was a problem hiding this comment.
The convention across our projects seems to be #
I don't care all that much but let's just stay consistent with what we already have https://github.com/AsteroidOS/asteroid-calculator/blob/master/README.md?plain=1#L1
|
|
||
| This daemon creates a TAP interface and exposes a BLE service with a RX and a | ||
| TX characteristic. These map to read/write operations on that TAP interface. | ||
|
|
||
| D-Bus forwarding |
There was a problem hiding this comment.
Maybe a more helpful commit title would be "Document D-Bus forwarding"
| #define TX_UUID "00001002-0000-0000-0000-00A57E401D05" | ||
| #define SERVICE_UUID "0000A071-0000-0000-0000-00A57E401D05" | ||
| #define RX_UUID "0000A001-0000-0000-0000-00A57E401D05" | ||
| #define TX_UUID "0000A002-0000-0000-0000-00A57E401D05" |
There was a problem hiding this comment.
Why change those ? If you are worried about backward compatibility since the protocol changes, don't worry, there are no users at the moment and it's undocumented, we're free to evolve it however we want.
| } | ||
| close(sock); | ||
|
|
||
| qDebug() << "MTU reset to" << mtu; |
There was a problem hiding this comment.
Is a space after to necessary ? I never remember if this qDebug syntax does it automagically or not
| const auto &value = it->second; | ||
|
|
||
| if (value.contains("Ethernet")) { | ||
| //QString name = value["Ethernet"].toMap()["Name"].toString(); |
| qCritical() << "Failed to get IFFLAGS"; | ||
| exit(1); | ||
| } | ||
| if (ifr.ifr_flags & IFF_UP) { |
There was a problem hiding this comment.
Let's extract a few helpers to bring the interface up and to configure connman
|
|
||
| void TAP::iffReset(int mtu) { | ||
| struct ifreq ifr = {}; | ||
| auto sock = socket(PF_INET, SOCK_DGRAM, 0); |
There was a problem hiding this comment.
Since this socket is just required to do ioctls, let's create it once as part of the constructor and keep the fd around.
| // Poll the interface file descriptor | ||
| mNotifier = new QSocketNotifier(mFd, QSocketNotifier::Read, this); | ||
| QObject::connect(mNotifier, &QSocketNotifier::activated, this, &TAP::fdActivated); | ||
| ifr.ifr_mtu = mtu - MY_ETH_TAP_HARD_FRAME_SIZE; |
There was a problem hiding this comment.
Let's reuse the mtuChanged() function here to avoid code duplication and chances that implementations drift over time :)
This PR is merely a dependency of the following PR: AsteroidOS/AsteroidOSSync#216