Review Request: Import/Export Routes for VPN connections

Discussion:

Rajeesh K Nambiar

2012-03-03 07:45:57 UTC

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

Review request for Network Management.

Description
-------

Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.

Diffs
-----

libs/ui/vpnpreferences.cpp 9afc6f2
settings/config/manageconnectionwidget.cpp caa2e06
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp 79f62ec

Diff: http://git.reviewboard.kde.org/r/104141/diff/

Testing
-------

Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/

Thanks,

Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120303/2e46bde2/attachment.html>

Lamarque Vieira Souza

2012-03-03 12:25:59 UTC

Permalink

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104141/#review11097
-----------------------------------------------------------

libs/ui/vpnpreferences.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8961>

setNeverDefault receives a bool parameter, not int. Please, fix this.

libs/ui/vpnpreferences.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8959>

you should use a plain space character ' ' instead of QRegExp("\\s"). Constructing a QRegExp object is not cheap and for simple things like this one it's better use the simpler alternative.

libs/ui/vpnpreferences.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8960>

Use the character '/' instead of the string "/". Using a character is faster than using a string.

libs/ui/vpnpreferences.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8953>

remove trailing whitespace.

settings/config/manageconnectionwidget.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8954>

join this line with the next to keep coding style.

settings/config/manageconnectionwidget.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8962>

You can use VpnUiPlugin::lastErrorMessage() here. Saying that the operation did not work but not saying why is very annoying.

vpnplugins/vpnc/vpnc.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8955>

remove trailing whitespace.

vpnplugins/vpnc/vpnc.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8957>

remove trailing whitespace.

vpnplugins/vpnc/vpnc.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8958>

where is the parameter for %1?

vpnplugins/vpnc/vpnc.cpp
<http://git.reviewboard.kde.org/r/104141/#comment8956>

remove trailing whitespace.

- Lamarque Vieira Souza

Post by Rajeesh K Nambiar
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104141/
-----------------------------------------------------------
(Updated March 3, 2012, 7:45 a.m.)
Review request for Network Management.
Description
-------
Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.
Diffs
-----
libs/ui/vpnpreferences.cpp 9afc6f2
settings/config/manageconnectionwidget.cpp caa2e06
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp 79f62ec
Diff: http://git.reviewboard.kde.org/r/104141/diff/
Testing
-------
Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/
Thanks,
Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120303/83227a43/attachment.html>

Rajeesh K Nambiar

2012-03-03 13:43:08 UTC

Permalink

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

(Updated March 3, 2012, 1:43 p.m.)

Review request for Network Management.

Changes
-------

Please review the bool conversion for setNeverDefault method. All other review comments are addressed.

Description
-------

Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.

Diffs (updated)
-----

libs/ui/vpnpreferences.cpp 9afc6f2
settings/config/manageconnectionwidget.cpp caa2e06
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp 79f62ec

Diff: http://git.reviewboard.kde.org/r/104141/diff/

Testing
-------

Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/

Thanks,

Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120303/174ef504/attachment-0001.html>

Lamarque Vieira Souza

2012-03-03 13:57:21 UTC

Permalink

Post by Rajeesh K Nambiar
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104141/
-----------------------------------------------------------
(Updated March 3, 2012, 1:43 p.m.)
Review request for Network Management.
Description
-------
Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.
Diffs
-----
libs/ui/vpnpreferences.cpp 9afc6f2
settings/config/manageconnectionwidget.cpp caa2e06
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp 79f62ec
Diff: http://git.reviewboard.kde.org/r/104141/diff/
Testing
-------
Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/
Thanks,
Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120303/cbe0148f/attachment.html>

Lamarque Vieira Souza

2012-03-03 15:10:25 UTC

Permalink

Post by Rajeesh K Nambiar

libs/ui/vpnpreferences.cpp, line 73
<http://git.reviewboard.kde.org/r/104141/diff/2/?file=51665#file51665line73>
It's ok now.
I tried to test the patch but it seem NetworkManager is refusing to accept the route configuration from Plasma NM. That is not the fault of your patch but I cannot add new routes to test your patch.
The message "NetworkManager[9513]: nm_ip4_route_set_prefix: assertion `prefix <= 32' failed" appears in the log when I try to add new routes (for examploe 192.168.2.0/255.255.255.0 through gateway 192.168.1.1). 255.255.255.0 == prefix 0 and 0 is < 32, I do not know why this is happening. It would be better if the error messages printed the wrong prefix, but whatever.
I will have to fix that problem first before I can test your patch.

Nevermind, downgrading to NetworkManager 0.9.2.0 fixed the issue. Well, NM is not doing so well, I used using a git version from 0.9.3 something, I tried to upgrade to fix that problem but then NM 0.9.3.995 simply does not start here. Downgrading was the solution.

I tested your patch, it seems to work. Can you update the OpenVPN export/import code to support this feature? can you provide a patch against master too?

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104141/#review11104
-----------------------------------------------------------

Post by Rajeesh K Nambiar
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104141/
-----------------------------------------------------------
(Updated March 3, 2012, 1:43 p.m.)
Review request for Network Management.
Description
-------
Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.
Diffs
-----
libs/ui/vpnpreferences.cpp 9afc6f2
settings/config/manageconnectionwidget.cpp caa2e06
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp 79f62ec
Diff: http://git.reviewboard.kde.org/r/104141/diff/
Testing
-------
Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/
Thanks,
Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120303/e9e107b9/attachment.html>

Rajeesh K Nambiar

2012-03-03 15:44:38 UTC

Permalink

Post by Lamarque Vieira Souza

I see what's happening - if you change configuration file entry to 192.168.2.0/24, it will work. You can see that in the IPV4 settings tab, under Routes the netmask is set as 0.0.0.0 if 255.255.255.0 is used, which is why NetworkManager refuses to add the connection.

We should ideally do a sanity check on the address and prefix. I could use SimpleIpV4AddressValidator to validate address. Any clue how to handle 255.255.255.0 and 24 as same? Nothing helpful in QHostAdress class...

- Rajeesh

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104141/#review11104
-----------------------------------------------------------

Post by Lamarque Vieira Souza
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104141/
-----------------------------------------------------------
(Updated March 3, 2012, 1:43 p.m.)
Review request for Network Management.
Description
-------
Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.
Diffs
-----
libs/ui/vpnpreferences.cpp 9afc6f2
settings/config/manageconnectionwidget.cpp caa2e06
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp 79f62ec
Diff: http://git.reviewboard.kde.org/r/104141/diff/
Testing
-------
Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/
Thanks,
Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120303/123f2e87/attachment-0001.html>

Lamarque Vieira Souza

2012-03-03 16:05:15 UTC

Permalink

Post by Lamarque Vieira Souza

Nevermind, downgrading to NetworkManager 0.9.2.0 fixed the issue. Well, NM is not doing so well, I used using a git version from 0.9.3 something, I tried to upgrade to fix that problem but then NM 0.9.3.995 simply does not start here. Downgrading was the solution.
I tested your patch, it seems to work. Can you update the OpenVPN export/import code to support this feature? can you provide a patch against master too?
I see what's happening - if you change configuration file entry to 192.168.2.0/24, it will work. You can see that in the IPV4 settings tab, under Routes the netmask is set as 0.0.0.0 if 255.255.255.0 is used, which is why NetworkManager refuses to add the connection.
We should ideally do a sanity check on the address and prefix. I could use SimpleIpV4AddressValidator to validate address. Any clue how to handle 255.255.255.0 and 24 as same? Nothing helpful in QHostAdress class...

I do not see that. The gateway is set to 0.0.0.0 by default in Routes, not the netmask. Even after I changed it to 192.168.1.1 NM still refuses to add the route. Anyway, NM 0.9.2.0 works. There is already inputmasks for all entries in the dialog.

- Lamarque Vieira

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104141/#review11104
-----------------------------------------------------------

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120303/bcbfa20e/attachment.html>

Rajeesh K Nambiar

2012-03-04 14:02:42 UTC

Permalink

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

(Updated March 4, 2012, 2:02 p.m.)

Review request for Network Management.

Changes
-------

Added openvpn support also; but not tested. This patch is against nm09 branch.

Description
-------

Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.

Diffs (updated)
-----

libs/ui/vpnpreferences.cpp 9afc6f2
settings/config/manageconnectionwidget.cpp caa2e06
vpnplugins/openvpn/CMakeLists.txt ce9e1e6
vpnplugins/openvpn/openvpn.cpp 2d4402f
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp 79f62ec

Diff: http://git.reviewboard.kde.org/r/104141/diff/

Testing
-------

Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/

Thanks,

Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120304/2a130304/attachment.html>

Rajeesh K Nambiar

2012-03-04 14:03:40 UTC

Permalink

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

(Updated March 4, 2012, 2:03 p.m.)

Review request for Network Management.

Changes
-------

Patch against master branch. I could not compile-test yet.

Description
-------

Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.

Diffs (updated)
-----

libs/ui/vpnpreferences.cpp a8442ec
settings/config/manageconnectionwidget.cpp dd8b406
vpnplugins/openvpn/CMakeLists.txt ce9e1e6
vpnplugins/openvpn/openvpn.cpp 05bb9ac
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp dc6bc58

Diff: http://git.reviewboard.kde.org/r/104141/diff/

Testing
-------

Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/

Thanks,

Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120304/5761b132/attachment.html>

Commit Hook

2012-03-06 12:10:14 UTC

Permalink

Post by Rajeesh K Nambiar
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104141/
-----------------------------------------------------------
(Updated March 4, 2012, 2:03 p.m.)
Review request for Network Management.
Description
-------
Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.
Diffs
-----
libs/ui/vpnpreferences.cpp a8442ec
settings/config/manageconnectionwidget.cpp dd8b406
vpnplugins/openvpn/CMakeLists.txt ce9e1e6
vpnplugins/openvpn/openvpn.cpp 05bb9ac
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp dc6bc58
Diff: http://git.reviewboard.kde.org/r/104141/diff/
Testing
-------
Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/
Thanks,
Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120306/9ad011d2/attachment.html>

Commit Hook

2012-03-06 12:21:12 UTC

Permalink

Post by Rajeesh K Nambiar
-----------------------------------------------------------
http://git.reviewboard.kde.org/r/104141/
-----------------------------------------------------------
(Updated March 4, 2012, 2:03 p.m.)
Review request for Network Management.
Description
-------
Import/Export Routes setting for VPN connections from/to configuration files. Please review the Gateway/Metrics part in particular.
Diffs
-----
libs/ui/vpnpreferences.cpp a8442ec
settings/config/manageconnectionwidget.cpp dd8b406
vpnplugins/openvpn/CMakeLists.txt ce9e1e6
vpnplugins/openvpn/openvpn.cpp 05bb9ac
vpnplugins/vpnc/CMakeLists.txt 5af39b9
vpnplugins/vpnc/vpnc.cpp dc6bc58
Diff: http://git.reviewboard.kde.org/r/104141/diff/
Testing
-------
Tested the import/export part, Routes appear properly in the new connection. I don't have a real configuration file to test :-/
Thanks,
Rajeesh K Nambiar

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-networkmanager/attachments/20120306/ec22e540/attachment.html>