Review Request 124140: Ported over to Netty and added SSL support

Sat Jun 27 00:56:46 UTC 2015

> On June 26, 2015, 12:57 p.m., Albert Vaca Cintora wrote:
> > src/org/kde/kdeconnect/Device.java, lines 244-251
> > <https://git.reviewboard.kde.org/r/124140/diff/1-2/?file=380829#file380829line244>
> >
> >     We don't need to call sendPackage in Device, the link can do it because they have a similar (but lower-level) function. Also, this allows us to have Links that don't even need to send a packet to pair, because they use some other mechanism. If we make this change, Device should only allow sending packets if the device is paired.
> 
> Vineet Garg wrote:
>     But still we want the device to fully paired with all the links, that is why pair package traverses through all link providers. If we dont send a pair package when the link doesn't need it, it will create problems to other links. Current protocol is like, create a meta pair package to pair or unpair fully.

The way you do it, only ONE link will receive the packet (sendPackage stops after the first successfull link). It's better if we let the Links manage the pairing themselves, and not the Device. The Device only needs to know if it's paired or not.

> On June 26, 2015, 12:57 p.m., Albert Vaca Cintora wrote:
> > src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java, line 396
> > <https://git.reviewboard.kde.org/r/124140/diff/1-2/?file=380825#file380825line396>
> >
> >     No keepalive? Why?
> 
> Vineet Garg wrote:
>     It is set in TcpInitialiser.

Ok

> On June 26, 2015, 12:57 p.m., Albert Vaca Cintora wrote:
> > src/org/kde/kdeconnect/Backends/BasePairingHandler.java, line 26
> > <https://git.reviewboard.kde.org/r/124140/diff/2/?file=381381#file381381line26>
> >
> >     Could be an interface instead of an abstract class.
> 
> Vineet Garg wrote:
>     Then implenting classes need to implement all methods, they might not want to.

They should implement them, and they can always implement them empty if they really need to.

> On June 26, 2015, 12:57 p.m., Albert Vaca Cintora wrote:
> > src/org/kde/kdeconnect/Device.java, lines 498-503
> > <https://git.reviewboard.kde.org/r/124140/diff/1-2/?file=380829#file380829line498>
> >
> >     ?
> 
> Vineet Garg wrote:
>     This is when one device unpairs when network is off, and later tries to pair with other device where other device thinks it is paired. So if paired and still getting pairing request, just silently unpair and continue.

Add a comment explaining so. And anyway, to my previous point, all this logic should be handled by the link and not by Device.

> On June 26, 2015, 12:57 p.m., Albert Vaca Cintora wrote:
> > src/org/kde/kdeconnect/Backends/LanBackend/LanPairingHandler.java, line 54
> > <https://git.reviewboard.kde.org/r/124140/diff/2/?file=381384#file381384line54>
> >
> >     This function is called requestPairing but does not request anything, it just puts info in a NP. See my comments about this below.
> 
> Vineet Garg wrote:
>     All these methods are named same as their calling methods for ease of remembrance. If their names are not matching what they are doing, what about prefixing them with an "on" like onRequestPairing?

When I said "below" I meant "above". What I wanted to say is that this function should actually send the NetworkPackage if it needs too, not just "configure" a package that somebody else will send (and might even send it to a different link than this one!).

- Albert

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124140/#review81789
-----------------------------------------------------------

On June 25, 2015, 7:35 a.m., Vineet Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124140/
> -----------------------------------------------------------
> 
> (Updated June 25, 2015, 7:35 a.m.)
> 
> 
> Review request for kdeconnect and Albert Vaca Cintora.
> 
> 
> Repository: kdeconnect-android
> 
> 
> Description
> -------
> 
> * Since MINA was tying hands in adding SSL, ported over Netty which has a good, clean and easy interface and a larger user and developer base.
> * Added support to setup links on SSL
> * Links automatically removed if wrong certificate is sent
> * Shows keys based on hash of certificate to check right certificates are received
> * Added a preference to use SSL, as it is experienced to cause high CPU usage on devices.
> * Corrected unit tests, LanLinkProvider is removed as it is nearly impossible to write it with current design. Will find a way
> 
> 
> Diffs
> -----
> 
>   build.gradle 6520c1c 
>   res/values/strings.xml c128342 
>   src/org/kde/kdeconnect/Backends/BaseLink.java 579a7af 
>   src/org/kde/kdeconnect/Backends/BaseLinkProvider.java cfaf621 
>   src/org/kde/kdeconnect/Backends/BasePairingHandler.java PRE-CREATION 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLink.java 5994142 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanLinkProvider.java ae26958 
>   src/org/kde/kdeconnect/Backends/LanBackend/LanPairingHandler.java PRE-CREATION 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLink.java add92f8 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackLinkProvider.java bd9c41b 
>   src/org/kde/kdeconnect/Backends/LoopbackBackend/LoopbackPairingHandler.java PRE-CREATION 
>   src/org/kde/kdeconnect/BackgroundService.java 5e3d8c2 
>   src/org/kde/kdeconnect/Device.java 3262aef 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/RsaHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/Helpers/SecurityHelpers/SslHelper.java PRE-CREATION 
>   src/org/kde/kdeconnect/NetworkPackage.java f62b5d0 
>   src/org/kde/kdeconnect/UserInterface/PairActivity.java 7a45751 
>   tests/org/kde/kdeconnect/DeviceTest.java 5d3383d 
>   tests/org/kde/kdeconnect/LanLinkProviderTest.java fe7863a 
>   tests/org/kde/kdeconnect/LanLinkTest.java d3d94c9 
>   tests/org/kde/kdeconnect/NetworkPackageTest.java a21114e 
> 
> Diff: https://git.reviewboard.kde.org/r/124140/diff/
> 
> 
> Testing
> -------
> 
> Tesed on some device with where both supports SSL, also with PC where ssl is not supported, working fine. Need a little bit more testing with more devices.
> 
> 
> Thanks,
> 
> Vineet Garg
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20150627/ddd9d81d/attachment-0001.html>