GNOME Bugzilla – Bug 749686
Port PPTP VPN plugin to new libnm library and GDBus
Last modified: 2015-08-31 12:22:17 UTC
PPTP VPN plugin (and all the plugins) has to be ported to new libnm so that it can be used together with network-manager-applet from master (that has been already ported). I pushed code migrating the plugin to libnm (instead of libnm-glib) and also to GDBus (instead of dbus-glib) - jk/libnm-port branch. See also a fix in the plugin interface in libnm - jk/libnm-nm-vpn-plugin-old-fixes branch in the main NetworkManager repository.
The frist two patches LGTM. >> all: port pptp plugin to libnm I proposed a patch in bug 749877 to load plugins via libnm. This mainly affects NMVpnEditorPlugin and nm_vpn_editor_plugin_factory(). Actually, there is not overlap there, and it should just work together fine as is. But lets merge bug 749877 first to make sure that they work together. Also, we must also stabalize the libnm API "NMVpnPluginOld" (bug 749951). This bug depends on that too. It is valuable to have a first plugin ported to a WIP API in libnm to have a proof-of-concept implementation. But this bug is blocked on the libnm API. As discussed in https://bugzilla.gnome.org/show_bug.cgi?id=749365#c3, we IMO need to build a legacy version of the properties plugin. So, we need both new legacy nm_vpn_plugin_ui_factory() and new nm_vpn_editor_plugin_factory() as separate dlls [1].
> libnm: NMVpnPluginOld fixes I don't think "&v" is a legal format string in nm_vpn_plugin_old_set_config(). & only works for strings and object paths. I think you want just g_variant_lookup_value() here, eg: if (priv->banner) g_variant_unref (priv->banner); priv->banner = g_variant_lookup_value (config, NM_VPN_PLUGIN_CONFIG_BANNER, "s"); I think nm_vpn_plugin_old_set_ip4_config() now leaks the GVariant from the "while (g_variant_iter_next (&iter, "{&sv}", &key, &value))" loop. The old code didn't have to free the string because it iterated it with "&" and thus got a constant copy, but that's not possible with GVariant here. If the variant was floating (eg, created with g_variant_new() directly in the call) that would be OK, but in this case the variant isn't floating becuase it's already in another variant. So I think the loop needs to be: + while (g_variant_iter_next (&iter, "{&sv}", &key, &value)) { + g_variant_builder_add (&builder, "{sv}", key, value); + g_variant_unref (value); + } Also, now that these values are GVariants, finalize() needs to g_variant_unref() them all if they are !NULL, instead of g_free(). > all: port to GDBus (pptp plugin) +/* We have a separate objec to handle ppp plugin requests since "object" Same thing as above here in service_ip4_config_cb(); the while loop iterating over the IP config hash will leak 'value'.
(In reply to Dan Williams from comment #2) > > libnm: NMVpnPluginOld fixes > > I don't think "&v" is a legal format string in > nm_vpn_plugin_old_set_config(). & only works for strings and object paths. > I think you want just g_variant_lookup_value() here, eg: > > if (priv->banner) > g_variant_unref (priv->banner); > priv->banner = g_variant_lookup_value (config, NM_VPN_PLUGIN_CONFIG_BANNER, > "s"); > Fixed, thanks. > I think nm_vpn_plugin_old_set_ip4_config() now leaks the GVariant from the > "while (g_variant_iter_next (&iter, "{&sv}", &key, &value))" loop. The old > code didn't have to free the string because it iterated it with "&" and thus > got a constant copy, but that's not possible with GVariant here. If the > variant was floating (eg, created with g_variant_new() directly in the call) > that would be OK, but in this case the variant isn't floating becuase it's > already in another variant. > > So I think the loop needs to be: > > + while (g_variant_iter_next (&iter, "{&sv}", &key, &value)) { > + g_variant_builder_add (&builder, "{sv}", key, value); > + g_variant_unref (value); > + } > > Also, now that these values are GVariants, finalize() needs to > g_variant_unref() them all if they are !NULL, instead of g_free(). > Fixed, thanks. > > all: port to GDBus (pptp plugin) > > +/* We have a separate objec to handle ppp plugin requests since > > "object" > > Same thing as above here in service_ip4_config_cb(); the while loop > iterating over the IP config hash will leak 'value'. Fixed too. I need to test the stuff. Of course, testing from anybody is appretiated.
(In reply to Thomas Haller from comment #1) > The frist two patches LGTM. > > >> all: port pptp plugin to libnm > > I proposed a patch in bug 749877 to load plugins via libnm. > This mainly affects NMVpnEditorPlugin and nm_vpn_editor_plugin_factory(). > Actually, there is not overlap there, and it should just work together fine > as is. But lets merge bug 749877 first to make sure that they work together. > > Also, we must also stabalize the libnm API "NMVpnPluginOld" (bug 749951). > This bug depends on that too. > I would first port the plugins to libnm (with the existing NMVpnPluginOld). We can merge it to master or keep it at separate branch. And then we can create the new NMVpnPlugin and start using it by plugins. By the way, is there any design for the new plugin API?
(In reply to Jiri Klimes from comment #4) > (In reply to Thomas Haller from comment #1) > > The frist two patches LGTM. > > > > >> all: port pptp plugin to libnm > > > > I proposed a patch in bug 749877 to load plugins via libnm. > > This mainly affects NMVpnEditorPlugin and nm_vpn_editor_plugin_factory(). > > Actually, there is not overlap there, and it should just work together fine > > as is. But lets merge bug 749877 first to make sure that they work together. > > > > Also, we must also stabalize the libnm API "NMVpnPluginOld" (bug 749951). > > This bug depends on that too. > > > > I would first port the plugins to libnm (with the existing NMVpnPluginOld). > We can merge it to master or keep it at separate branch. And then we can > create the new NMVpnPlugin and start using it by plugins. By the way, is > there any design for the new plugin API? Too bad we didn't exclude NMVpnPluginOld from public API when releasing 1.0. I'd rather not have any users of an API that was deprecated and unused from the start. I even think we can still drop it before 1.2 (and even from 1.0.4). What do you think? In the very first step, creating "NMVpnPlugin" based on NMVpnPluginOld does not mean anything beyond copying the files and rename the functions [1]. That is already a major part of bug 749951. Porting ~one~ plugin to NMVpnPluginOld, get it to work, and then step-by-step implement the new plugin API (while being able to test it), makes sense. But why not do [1] and get the class-name and the header-file right from the start, at least we know that the new API will not be called "NMVpnPluginOld". Or does anybody want to release 1.2 with plugins based on NMVpnPluginOld?
I took the only patch from branch jk/libnm-nm-vpn-plugin-old-fixes and moved it to th/libnm-vpn-plugin-bgo749877. The patch [Jiří Klimeš] libnm: NMVpnPluginOld fixes moved there.
Btw, jk/libnm-nm-vpn-plugin-old-fixes now also contains the new VPN API for nm-1-2. Actually, I just renamed nm-vpn-plugin-old to nm-vpn-service-plugin. But based on that we can stabilize on the new API.
Regarding network-manager-pptp (jk/libnm-port), the first question is IMO: do we want to support a legacy "properties" library for libnm-glib and do we need to build them both (from the same source)? IMO "yes".
(In reply to Thomas Haller from comment #8) > Regarding network-manager-pptp (jk/libnm-port), > > the first question is IMO: do we want to support a legacy "properties" > library for libnm-glib and do we need to build them both (from the same > source)? > > IMO "yes". I think we will do the same as for nm-applet, i.e. * leave nm-1-0 branch as it is - using old libnm * switch master branch to new libnm
(In reply to Jiri Klimes from comment #9) > (In reply to Thomas Haller from comment #8) > > Regarding network-manager-pptp (jk/libnm-port), > > > > the first question is IMO: do we want to support a legacy "properties" > > library for libnm-glib and do we need to build them both (from the same > > source)? > > > > IMO "yes". > > I think we will do the same as for nm-applet, i.e. > * leave nm-1-0 branch as it is - using old libnm > * switch master branch to new libnm the significant problem is that gnome-control-center, plasma-nm, and nm-applet make use of the same properties plugin. It is not feasible to upgrade all to libnm-1-2 together. So, a distribution either break lots of users when the first packages make the switch, or it packages all VPN plugins twice... The latter is a large amount of work, that would have to be done multiple times by the various downstreams. We should instead do the work upstream, once-per-plugin.
(In reply to Thomas Haller from comment #10) > (In reply to Jiri Klimes from comment #9) > > (In reply to Thomas Haller from comment #8) > > > Regarding network-manager-pptp (jk/libnm-port), > > > > > > the first question is IMO: do we want to support a legacy "properties" > > > library for libnm-glib and do we need to build them both (from the same > > > source)? > > > > > > IMO "yes". > > > > I think we will do the same as for nm-applet, i.e. > > * leave nm-1-0 branch as it is - using old libnm > > * switch master branch to new libnm > > the significant problem is that gnome-control-center, plasma-nm, and > nm-applet make use of the same properties plugin. It is not feasible to > upgrade all to libnm-1-2 together. > > So, a distribution either break lots of users when the first packages make > the switch, or it packages all VPN plugins twice... The latter is a large > amount of work, that would have to be done multiple times by the various > downstreams. We should instead do the work upstream, once-per-plugin. Yeah, I see your point. So I guess we do need parallel 'properties dialog' pieces for now. Which kinda sucks.
Added the libnm-glib version of the properties dialog to lr/libnm
I've made some changes lr/libnm, to make it work with libnm-glib and align with what has been done for other plugins. Please take a look: 562af74 service: fix a nonsense condition 15f1a33 build: use AM_CPPFLAGS, avoid deprecated INCLUDES 92e4c25 fixup! build: use AM_CPPFLAGS, avoid deprecated INCLUDES 4b87f65 build: fix the libdbus substitution a49864c all: port pptp plugin to libnm 15a0fc9 fixup! all: port pptp plugin to libnm 195991d fixup! all: port pptp plugin to libnm 26ab355 fixup! all: port pptp plugin to libnm 19f79ea properties: build also the libnm-glib version of the properties plugin 4e54512 service: port pptp plugin to libnm 21df8e7 fixup! service: port pptp plugin to libnm ad5a871 all: port to GDBus d00c7ec build: allow build without libnm-glib Builds & works with: NetworkManager:th/libnm-vpn-plugin-bgo749877 network-manager-applet:th/vpn-plugin-info-bgo749877
https://git.gnome.org/browse/network-manager-pptp/log/?h=lr/libnm
(In reply to Lubomir Rintel from comment #14) > https://git.gnome.org/browse/network-manager-pptp/log/?h=lr/libnm LGTM