GNOME Bugzilla – Bug 651657
[openvpn, UI] - unable to specify values for "ping" or "ping-exit" or related options
Last modified: 2015-08-26 08:49:38 UTC
There needs to be some way to specify at least the "ping" option; it appears that some openvpn servers will disconnect after an inactivity period otherwise. The symptom is that a VPN connection only stays up for a couple of minutes. Ideally this would appear in the OpenVPN "Advanced" window, something like this: [x] Use custom ping interval: [ 10 ] [x] Enable ping timeout [ 120 ] Ping timeout action: [disconnect] [ restart ] ...where [x] are checkbox, [ ## ] are numerical spinny things and the other [] is a dropdown. Be in awe of my ASCII GUI proficiency. Name : NetworkManager Arch : x86_64 Epoch : 1 Version : 0.8.999 Release : 3.git20110526.fc15 Name : NetworkManager-openvpn Arch : x86_64 Epoch : 1 Version : 0.8.999 Release : 1.fc15
I think this should be option: Keepalive [15]s [60]s where: 15 - Ping interval 60 - Enable ping timeout And this should be add to config file like: Keepalive 15 60
Created attachment 304205 [details] [review] network-manager-openvpn-keepalive.patch FWIW, here's a patch I'm using so that network-manager-openvpn won't drop me from the Suse VPN. I'm not sure if this is a quirk with our VPN, or if one genuinely needs to be able to set a timeout by hand. If it's the latter, I can make a patch to: 1. put this in the GUI (maybe just the keepalive values, not the explicit ping time values?) 2. Import the keepalive option from .ovpn files when creating a new VPN.
See bug #656214 for another request to have extra options.
(In reply to Federico Mena Quintero from comment #2) > Created attachment 304205 [details] [review] [review] > network-manager-openvpn-keepalive.patch > > 1. put this in the GUI (maybe just the keepalive values, not the explicit > ping time values?) > > 2. Import the keepalive option from .ovpn files when creating a new VPN. yes, it would good to support these options too. The most complicate part seems how to present them in the UI. If that is a problem, a first patch could add support for the options without adding them to the UI. Patch attachment 304205 [details] [review] enables ping timeout unconditionally. That might not be what all users want, so IMO this patch shouldn't be used upstream as-is. But it might be useful for somebody to patch his own build. Thank you for that. Another way to have this as a workaround (without recompiling the plugin) could be to replace the openvpn binary by a wrapper script like: #!/bin/bash exec /path/to/real/openvpn "$@" --keep-alive 10 30
Created attachment 305631 [details] [review] network-manager-openvpn-bgo651657.patch This patchset adds support for import/export of these options: ping ping-exit ping-restart keepalive I haven't added the GUI bits. Does this look like a step in the right direction?
(In reply to Federico Mena Quintero from comment #5) > Created attachment 305631 [details] [review] [review] > network-manager-openvpn-bgo651657.patch These patches look good to me. We can also merge them early, and have the GUI bits later. ... except, that their order is wrong. Adding the test first, and the fix afterwards, makes that commit broken. I reordered the commits and pushed them to a branch https://git.gnome.org/browse/network-manager-openvpn/log/?h=th/ping-bgo651657 (no other changes there).
Thanks for creating a branch :) I'll ask Jakub here at GUADEC if he has any ideas for how to present these. *I* only need keepalive myself, but since they are esoteric options anyway, it's probably okay to show values for each option...
> properties: Import the ping/ping-exit/ping-restart options from openvpn some whitespace fixup needed for handle_num_seconds_item()'s arguments. > properties/tests: Add tests for import/export of the ping/ping-exit/ping-restart options whitespace on test_ping_import() too. Also we can just use g_assert_cmp*() these days instead of the homegrown ASSERT(), which was from before we had any g_assert_*() stuff in glib. Yay history. Beyond that the patches look good. Thanks!
> properties: Import the keepalive option + nm_setting_vpn_add_data_item (s_vpn, NM_OPENVPN_KEY_PING, tmp); + nm_setting_vpn_add_data_item (s_vpn, NM_OPENVPN_KEY_PING_RESTART, tmp2); + + g_free (tmp); tmp2 should be released too. Apart from this, LGTM.
Fixed branch with remarks from comment 8 and comment 9. Also merged two commits into one resulting in "properties: import/export the ping/ping-exit/ping-restart options from..." otherwise, the commit broke `make check` too. Also, added one more commit "service: use ping,ping-exit,ping-restart arguments" Repushed for review
After discussion on IRC, I merged the branch to upstream master: https://git.gnome.org/browse/network-manager-openvpn/commit/?id=f5927b16ea9f08e2c5a70117f2b29a84c90ab36b (Thank you Federico!!) Leave bug open to implement UI support.
Thanks for tweaking/merging this :)
(In reply to Thomas Haller from comment #11) > > Leave bug open to implement UI support. UI added in branch jk/ping-ui-bgo651657
(In reply to Jiri Klimes from comment #13) > (In reply to Thomas Haller from comment #11) > > > > Leave bug open to implement UI support. > > UI added in branch jk/ping-ui-bgo651657 LGTM
Committed to master: 6d589f2 properties/ui: add ping, ping-exit and ping-restart to the UI (bgo #651657)