Review Request 129111: Add Iodine VPN support

Discussion:

Jan Grulich

2016-10-06 11:09:26 UTC

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

Review request for Network Management and Lamarque Souza.

Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655

Repository: plasma-nm

Description
-------

This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.

Diffs
-----

vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION

Diff: https://git.reviewboard.kde.org/r/129111/diff/

Testing
-------

Thanks,

Jan Grulich

Lamarque Souza

2016-10-06 13:23:55 UTC

Permalink

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

vpn/iodine/iodine.h (line 34)
<https://git.reviewboard.kde.org/r/129111/#comment67082>

You should use Q_DECL_OVERRIDE here instead of virtual. The same applies to all methods below.

vpn/iodine/iodine.ui (line 31)
<https://git.reviewboard.kde.org/r/129111/#comment67088>

This should be a spinbox to prevent users from typing non-digit characters and negative numbers.

vpn/iodine/iodineauth.h (line 35)
<https://git.reviewboard.kde.org/r/129111/#comment67084>

nullptr instead of 0? Or is Q_NULLPTR still recommended?

vpn/iodine/iodineauth.h (line 37)
<https://git.reviewboard.kde.org/r/129111/#comment67083>

Q_DECL_OVERRIDE

vpn/iodine/iodinewidget.h (line 40)
<https://git.reviewboard.kde.org/r/129111/#comment67085>

Q_DECL_OVERRIDE here and all lines below.

vpn/iodine/iodinewidget.cpp (line 57)
<https://git.reviewboard.kde.org/r/129111/#comment67086>

setting is passed to loadSecrets() at the end of this method, so this line is not needed.

vpn/iodine/iodinewidget.cpp (line 140)
<https://git.reviewboard.kde.org/r/129111/#comment67087>

Only toplevelDomain is required? Shouldn't nameserver and password also be required? It does not make sense to create a vpn without password.

- Lamarque Souza

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129111/
-----------------------------------------------------------
(Updated Oct. 6, 2016, 11:09 a.m.)
Review request for Network Management and Lamarque Souza.
Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655
Repository: plasma-nm
Description
-------
This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.
Diffs
-----
vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/129111/diff/
Testing
-------
Thanks,
Jan Grulich

Jan Grulich

2016-10-06 13:28:03 UTC

Permalink

Post by Jan Grulich

vpn/iodine/iodinewidget.cpp, line 140
<https://git.reviewboard.kde.org/r/129111/diff/1/?file=481768#file481768line140>
Only toplevelDomain is required? Shouldn't nameserver and password also be required? It does not make sense to create a vpn without password.

Yes, only toplevelDomain is required, the rest of properties are optional.

- Jan

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

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129111/
-----------------------------------------------------------
(Updated ÅÃj. 6, 2016, 11:09 dop.)
Review request for Network Management and Lamarque Souza.
Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655
Repository: plasma-nm
Description
-------
This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.
Diffs
-----
vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/129111/diff/
Testing
-------
Thanks,
Jan Grulich

Jan Grulich

2016-10-06 13:39:59 UTC

Permalink

Post by Jan Grulich

vpn/iodine/iodine.ui, line 31
<https://git.reviewboard.kde.org/r/129111/diff/1/?file=481763#file481763line31>
This should be a spinbox to prevent users from typing non-digit characters and negative numbers.

I implemented this exactly the same way as it is implemented in nm-connection-editor. Do you have idea what would be the minimum and maximum value for this? I couldn't find it in the source code for NetworkManager-iodine plugin.

- Jan

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

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129111/
-----------------------------------------------------------
(Updated ÅÃj. 6, 2016, 11:09 dop.)
Review request for Network Management and Lamarque Souza.
Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655
Repository: plasma-nm
Description
-------
This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.
Diffs
-----
vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/129111/diff/
Testing
-------
Thanks,
Jan Grulich

Lamarque Souza

2016-10-06 13:58:21 UTC

Permalink

Post by Jan Grulich

I have not found that either. What I have found is that that number is like MTU for ethernet and the smaller that number the more overhead the connection will have. The maximum value should not be big since DNS' packets are not supposed to have a big payload, maybe 2 kbytes at the very most. It is also calculated automatically when it is not set, so maybe we sould add a hint saying that the user should not set that option and allow it to be calculcated automatically for better performance.

- Lamarque

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

Jan Grulich

2016-10-10 06:21:57 UTC

Permalink

Post by Jan Grulich

I implemented this exactly the same way as it is implemented in nm-connection-editor. Do you have idea what would be the minimum and maximum value for this? I couldn't find it in the source code for NetworkManager-iodine plugin.
I have not found that either. What I have found is that that number is like MTU for ethernet and the smaller that number the more overhead the connection will have. The maximum value should not be big since DNS' packets are not supposed to have a big payload, maybe 2 kbytes at the very most. It is also calculated automatically when it is not set, so maybe we sould add a hint saying that the user should not set that option and allow it to be calculcated automatically for better performance.

I changed the line edit to spinbox with minimum value to be 0 (Automatic) and maximum value 10000 (according to MTU for ethernet).

- Jan

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

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129111/
-----------------------------------------------------------
(Updated ÅÃj. 6, 2016, 11:09 dop.)
Review request for Network Management and Lamarque Souza.
Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655
Repository: plasma-nm
Description
-------
This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.
Diffs
-----
vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/129111/diff/
Testing
-------
Thanks,
Jan Grulich

Jan Grulich

2016-10-10 06:22:00 UTC

Permalink

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

(Updated ÅÃj. 10, 2016, 6:22 dop.)

Review request for Network Management and Lamarque Souza.

Changes
-------

Fixes mentioned issues.

Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655

Repository: plasma-nm

Description
-------

This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.

Diffs (updated)
-----

vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION

Diff: https://git.reviewboard.kde.org/r/129111/diff/

Testing
-------

Thanks,

Jan Grulich

Lamarque Souza

2016-10-10 13:23:54 UTC

Permalink

Post by Jan Grulich
-----------------------------------------------------------
https://git.reviewboard.kde.org/r/129111/
-----------------------------------------------------------
(Updated Oct. 10, 2016, 6:22 a.m.)
Review request for Network Management and Lamarque Souza.
Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655
Repository: plasma-nm
Description
-------
This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.
Diffs
-----
vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION
Diff: https://git.reviewboard.kde.org/r/129111/diff/
Testing
-------
Thanks,
Jan Grulich

Jan Grulich

2016-10-10 13:28:22 UTC

Permalink

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

(Updated Oct. 10, 2016, 1:28 p.m.)

Status
------

This change has been marked as submitted.

Review request for Network Management and Lamarque Souza.

Changes
-------

Submitted with commit ca89f40549efded1eb63c3f526465b9162c7cc50 by Jan Grulich to branch master.

Bugs: 367655
http://bugs.kde.org/show_bug.cgi?id=367655

Repository: plasma-nm

Description
-------

This patch adds support for Iodine VPN connections allowing tunnel connections via DNS.

Diffs
-----

vpn/CMakeLists.txt 3d3f962
vpn/iodine/CMakeLists.txt PRE-CREATION
vpn/iodine/Messages.sh PRE-CREATION
vpn/iodine/iodine.h PRE-CREATION
vpn/iodine/iodine.cpp PRE-CREATION
vpn/iodine/iodine.ui PRE-CREATION
vpn/iodine/iodineauth.h PRE-CREATION
vpn/iodine/iodineauth.cpp PRE-CREATION
vpn/iodine/iodineauth.ui PRE-CREATION
vpn/iodine/iodinewidget.h PRE-CREATION
vpn/iodine/iodinewidget.cpp PRE-CREATION
vpn/iodine/nm-iodine-service.h PRE-CREATION
vpn/iodine/plasmanetworkmanagement_iodineui.desktop PRE-CREATION

Diff: https://git.reviewboard.kde.org/r/129111/diff/

Testing
-------

Thanks,

Jan Grulich