After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 752499 - [review] lr/libnm: Port vpnc plugin to libnm
[review] lr/libnm: Port vpnc plugin to libnm
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: VPN: vpnc
git master
Other Linux
: Normal normal
: ---
Assigned To: Lubomir Rintel
NetworkManager maintainer(s)
Depends on: 752500
Blocks: nm-review 749365
 
 
Reported: 2015-07-16 15:51 UTC by Lubomir Rintel
Modified: 2015-08-31 12:22 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Lubomir Rintel 2015-07-16 15:51:22 UTC
WIP in danw/wip/libnm
Comment 1 Lubomir Rintel 2015-07-16 16:08:12 UTC
Some more fixups lr/libnm
Comment 2 Lubomir Rintel 2015-07-25 09:03:21 UTC
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
Comment 3 Dan Williams 2015-08-01 03:39:17 UTC
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.
Comment 4 Dan Williams 2015-08-01 03:57:56 UTC
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.
Comment 5 Dan Williams 2015-08-01 04:14:42 UTC
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.
Comment 6 Thomas Haller 2015-08-01 15:12:10 UTC
-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.?
Comment 7 Lubomir Rintel 2015-08-15 18:22:43 UTC
(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.
Comment 8 Lubomir Rintel 2015-08-15 18:38:15 UTC
(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.
Comment 9 Lubomir Rintel 2015-08-16 14:48:06 UTC
Ported to GDBus.

https://git.gnome.org/browse/network-manager-vpnc/log/?h=lr/libnm
Comment 10 Thomas Haller 2015-08-31 09:22:05 UTC
(In reply to Lubomir Rintel from comment #9)
> Ported to GDBus.
> 
> https://git.gnome.org/browse/network-manager-vpnc/log/?h=lr/libnm

LGTM