Broadcast actual tcp server port used
ClosedPublic

Authored by eduisters on Oct 21 2018, 12:16 PM.

Details

Summary

UDP Identity packet should set tcpPort to the actual tcpServer 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.
eduisters created this revision.Oct 21 2018, 12:16 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptOct 21 2018, 12:16 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
eduisters requested review of this revision.Oct 21 2018, 12:16 PM

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

Does this solve an actual issue for you?

eduisters added inline comments.Oct 24 2018, 8:59 AM
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.

Does this solve an actual issue for you?

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!

Is the code in the desktop side correct? This might be a really old bug if it is broken!

Yes, the desktop code broadcasts the tcp port it's actually bound to

Good catch! Thanks a lot :3

Do you have commit rights? If not, we will need your email to commit this in your behalf.

albertvaka accepted this revision.Oct 25 2018, 3:12 PM
This revision is now accepted and ready to land.Oct 25 2018, 3:12 PM

I don't have commit rights (yet), my email address: e.duisters1@gmail.com

This revision was automatically updated to reflect the committed changes.