GNOME Bugzilla – Bug 752499
[review] lr/libnm: Port vpnc plugin to libnm
Last modified: 2015-08-31 12:22:10 UTC
WIP in danw/wip/libnm
Some more fixups lr/libnm
lr/libnm ready for review: 71f54d2 properties: build separate libnm-based and libnm-glib-based plugins e1f5f52 service: port to libnm e936529 auth-dialog: port to libnm 0965f00 test: port to libnm f170158 build: allow build without libnm-glib Builds & works with: NetworkManager:th/libnm-vpn-plugin-bgo749877 network-manager-applet:th/vpn-plugin-info-bgo749877
LGTM, and seems to work. I pushed one commit: properties: disconnect password storage type icon signal on dispose to git master, which will then conflict with your branch, but it's a simple rename from VpncPluginUiInterface -> VpncPlugin etc. Fixes stupid warnings I saw print out on the console.
Although, if we move up the #define NM_VPN_LIBNM_COMPAT to before #include <nm-setting-vpn.h> then we get the compat defines for NMSettingVpn and those two lines can be removed from here. We still need the IPConfig ones though for import.
On the service side, I spoke too soon... First off, and this is probably true for all the plugins, we need to g_initable_init() the plugin objects after creating them. Should probably do that in the nm_XXX_plugin_new() functions. Otherwise the libnm/nm-vpnc-service-plugin.c code that actually registers the plugins on D-Bus doesn't run and NM doesn't know about them. Next, and specific to the vpnc plugin perhaps, but the helper doesn't appear to convey the information to the service, or the service doesn't send it to NM or something. But the VPN times out, and I do see in the debug logs that the VPN concentrator returned IP information and that the helper got run, but somehow the IP config doesn't get back to service/NM.
-plugin_LTLIBRARIES = libnm-vpnc-properties.la libnm-vpnc-properties-old.la +plugin_LTLIBRARIES = libnm-vpnc-properties.la +if WITH_LIBNM_GLIB +plugin_LTLIBRARIES += libnm-vpnc-properties-old.la +endif The old plugins were: [GNOME] properties= the new ones are: [libnm] plugin= I think we should keep the old name to be "libnm-vpn-cproperties". And for the new name, I don't feel that "properties" is a good name anymore. Let's have them "libnm-vpn-plugin-vpnc", "libnm-vpn-plugin-openvpn", "libnm-vpn-plugin-pptp", etc.?
(In reply to Dan Williams from comment #4) > Although, if we move up the #define NM_VPN_LIBNM_COMPAT to before #include > <nm-setting-vpn.h> then we get the compat defines for NMSettingVpn and those > two lines can be removed from here. We still need the IPConfig ones though > for import. Fixed up. (In reply to Dan Williams from comment #5) > On the service side, I spoke too soon... > > First off, and this is probably true for all the plugins, we need to > g_initable_init() the plugin objects after creating them. Should probably > do that in the nm_XXX_plugin_new() functions. Otherwise the > libnm/nm-vpnc-service-plugin.c code that actually registers the plugins on > D-Bus doesn't run and NM doesn't know about them. Fixed up. > Next, and specific to the vpnc plugin perhaps, but the helper doesn't appear > to convey the information to the service, or the service doesn't send it to > NM or something. But the VPN times out, and I do see in the debug logs that > the VPN concentrator returned IP information and that the helper got run, > but somehow the IP config doesn't get back to service/NM. That's bug #753663 in the daemon. (In reply to Thomas Haller from comment #6) > -plugin_LTLIBRARIES = libnm-vpnc-properties.la libnm-vpnc-properties-old.la > +plugin_LTLIBRARIES = libnm-vpnc-properties.la > +if WITH_LIBNM_GLIB > +plugin_LTLIBRARIES += libnm-vpnc-properties-old.la > +endif Fixed up. > The old plugins were: > > [GNOME] > properties= > > > the new ones are: > > [libnm] > plugin= > > > > I think we should keep the old name to be "libnm-vpn-cproperties". And for > the new name, I don't feel that "properties" is a good name anymore. Let's > have them "libnm-vpn-plugin-vpnc", "libnm-vpn-plugin-openvpn", > "libnm-vpn-plugin-pptp", etc.? Sounds good to me. Didn't update it in the version I just pushed, but I'll do that.
(In reply to Lubomir Rintel from comment #7) > > I think we should keep the old name to be "libnm-vpn-cproperties". And for > > the new name, I don't feel that "properties" is a good name anymore. Let's > > have them "libnm-vpn-plugin-vpnc", "libnm-vpn-plugin-openvpn", > > "libnm-vpn-plugin-pptp", etc.? > > Sounds good to me. Didn't update it in the version I just pushed, but I'll > do that. Done too.
Ported to GDBus. https://git.gnome.org/browse/network-manager-vpnc/log/?h=lr/libnm
(In reply to Lubomir Rintel from comment #9) > Ported to GDBus. > > https://git.gnome.org/browse/network-manager-vpnc/log/?h=lr/libnm LGTM