UDP Identity packet should set tcpPort to the actual tcpServer port used
Details
- Reviewers
albertvaka - Group Reviewers
KDE Connect - Commits
- R225:2c32bd334a95: Broadcast actual tcp server port used
Diff Detail
- Repository
- R225 KDE Connect - Android application
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
This looks like a good idea. Do you have a test case where the server ends up running on a different port?
src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java | ||
---|---|---|
396–397 | I am not an expert in this code. Do you know in what cases tcpServer would be null or not bound? Does it make sense to even try to prepare a packet in those cases? (In other words, should these checks be outside of this method?) |
src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java | ||
---|---|---|
396–397 | tcpServer will be null when running on Android version < 14 (ICE_CREAM_SANDWICH), see onStart. tcpServer will also be null if openServerSocketOnFreePort cannot find any available port to bind to. It still makes sense to broadcast the udp packet because the receiving party will try to establish a tcp connection and if that fails it will send a udp packet to us so we can try to establish a tcp connection to them. |
Any other app could be using the default port so we will end up using another port. If you don't advertise that port in the udp packet a reverse udp packet will have to be send to in order to get a working tcp connection (assuming nothing prevents our outgoing tcp connection from succeeding eg. firewall)
Is the code in the desktop side correct? This might be a really old bug if it is broken!
Good catch! Thanks a lot :3
Do you have commit rights? If not, we will need your email to commit this in your behalf.