GNOME Bugzilla – Bug 745997
[PATCH] vpn: handle D-Bus errors from libnm-glib-based plugins
Last modified: 2015-04-07 15:31:40 UTC
libnm-glib based plugins use a different D-Bus error interface than libnm-core based ones. The libnm-core way is the "more correct" way but due to the haphazard GError mappings in libnm-glib, we have to check for the old way too. So that we can use GDBus automatic error mapping between D-Bus and GErrors, create a new GError domain and enum for the old VPN error namespace which is based off the new one, since the values should stay in sync. Register that with GDBus, and then use a helper to check remote errors that catches both old and new errors.
Created attachment 299058 [details] [review] 0001-vpn-handle-D-Bus-errors-from-libnm-glib-based-plugin.patch
Comment on attachment 299058 [details] [review] 0001-vpn-handle-D-Bus-errors-from-libnm-glib-based-plugin.patch >libnm-glib based plugins use a different D-Bus error interface than >libnm-core based ones. The libnm-core way is the "more correct" way Doh. I don't think I meant to change it. We still define NM_DBUS_VPN_ERROR_PREFIX with the old interface in libnm-core/nm-vpn-dbus-interface.h... So do we want this patch, or should we fix libnm-core to use the old error interface, and then add the reverse backward-compat to the daemon? >+ /* Create a shadow error GType using the values in NM_VPN_PLUGIN_ERROR */ Note that two of the errors got renamed (GENERAL -> FAILED, CONNECTION_INVALID -> INVALID_CONNECTION). We never check for those error codes, so it doesn't actually matter, but registering them with the new names is still technically wrong... >+ g_once_init_leave (&g_define_type_id__volatile, g_define_type_id); >+ } >+ return g_define_type_id__volatile; (assuming this stays, fix the indentation) >- if (g_error_matches (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED)) { >+ if (match_remote_vpn_error (error, NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED)) { Given that this is the only code we ever check for specifically, maybe we should just use g_dbus_error_get_remote_error() here rather than going to all the trouble to register both domains...
(In reply to Dan Winship from comment #2) > Comment on attachment 299058 [details] [review] [review] > 0001-vpn-handle-D-Bus-errors-from-libnm-glib-based-plugin.patch > > >libnm-glib based plugins use a different D-Bus error interface than > >libnm-core based ones. The libnm-core way is the "more correct" way > > Doh. I don't think I meant to change it. We still define > NM_DBUS_VPN_ERROR_PREFIX with the old interface in > libnm-core/nm-vpn-dbus-interface.h... > > So do we want this patch, or should we fix libnm-core to use the old error > interface, and then add the reverse backward-compat to the daemon? I don't think we should fix libnm-core. The old way is about 100 years old and done when I/we didn't really understand D-Bus interface stuff and how errors should be namespaced. For example, like half the errors in the daemon were never actually registered with dbus-glib, and when they were, they didn't have an interface at all :) > Given that this is the only code we ever check for specifically, maybe we > should just use g_dbus_error_get_remote_error() here rather than going to > all the trouble to register both domains... Yeah, agreed, especially considering the naming changes. I'll make things more explicit.
ok, in that case we should add a comment to the NM_DBUS_VPN_ERROR_PREFIX define in nm-vpn-dbus-interface.h too noting that it's wrong and shouldn't be used
or, I guess, just fix it... :)
Created attachment 299129 [details] [review] 0001-vpn-handle-D-Bus-errors-from-libnm-glib-based-plugin.patch
(In reply to Dan Winship from comment #5) > or, I guess, just fix it... :) What did you mean by fix it?
(In reply to Dan Williams from comment #7) > (In reply to Dan Winship from comment #5) > > or, I guess, just fix it... :) > > What did you mean by fix it? change NM_DBUS_VPN_ERROR_PREFIX to "org.freedesktop.NetworkManager.VPN.Plugin" so that we don't have an incorrect #define in nm-vpn-dbus-interface.h except... I just noticed that introspection/nm-vpn-plugin.xml and libnm-core/nm-errors.h both claim that the error prefix is o.fd.NM.VPN.Error. So, it's now 4-to-1 in favor of that rather than o.fd.NM.VPN.Plugin. also: (In reply to Dan Williams from comment #3) > (In reply to Dan Winship from comment #2) > > Given that this is the only code we ever check for specifically, maybe we > > should just use g_dbus_error_get_remote_error() here rather than going to > > all the trouble to register both domains... > > Yeah, agreed, especially considering the naming changes. I'll make things > more explicit. What I meant there was "we don't even need to bother thinking about any of the errors except InteractiveNotSupported". There doesn't need to be a loop or an array or anything. Just: >- if (g_error_matches (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED)) { >+ if ( g_error_matches (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_INTERACTIVE_NOT_SUPPORTED) >+ || strstr (error->message, "org.freedesktop.NetworkManager.VPN.Error.InteractiveNotSupported")) { (or s/Error/Plugin/, depending)
Created attachment 300441 [details] [review] libnm-core: fix VPN error domain In theory, NM_VPN_PLUGIN_ERROR should have names under org.freedesktop.NetworkManager.VPN.Plugin, but for historical reasons, it's actually org.freedesktop.NetworkManager.VPN.Error. ===== My suggestion. I don't think there are any backward-compatibility issues with this fix; failing to recognize the old prefix would only cause a problem if you were running a git/1.2 daemon, but had a VPN plugin linked against libnm.so from NM 1.0. But we don't support mismatched library and daemon versions.
(In reply to Dan Winship from comment #9) > I don't think there are any backward-compatibility issues with this > fix; failing to recognize the old prefix would only cause a problem if > you were running a git/1.2 daemon, but had a VPN plugin linked against > libnm.so from NM 1.0. But we don't support mismatched library and > daemon versions. that presumes that all VPN plugins are based on libnm, right?
(In reply to Thomas Haller from comment #10) > that presumes that all VPN plugins are based on libnm, right? No. In fact, we know that there aren't any VPN plugins based on libnm.so, because a plugin linked to libnm.so 1.0 would not work against /sbin/NetworkManager 1.0, because libnm 1.0 used o.fd.NM.VPN.Plugin, and NM 1.0 used o.fd.NM.VPN.Error. All released versions of the NM daemon have expected the prefix org.freedesktop.NetworkManager.VPN.Error, so any VPN tested against any released NM version would be sending that prefix. The only way someone could have a VPN plugin using o.fd.NM.VPN.Plugin would be if they had only ever tested it against NM from the last few weeks of git master.
Might be obsoleted by https://bugzilla.gnome.org/show_bug.cgi?id=746901 "libnm-core: fix VPN error domain".
the patch on that branch is the same as the one in comment 9; i didn't mean to include it in that branch when I pushed it, I had just done it locally because I couldn't test otherwise
patch works for me. It was a bit complicated to get my system into the setup that got me to this point, but I'll see if I can get it there again. Don't wait for that though.
Just trying to activate a vpnc connection while running git master NM should trigger the bug; nm-vpnc-service will send back o.fd.NM.VPN.Error.InteractiveNotSupported, and git master NM will not recognize that because it was expecting o.fd.NM.VPN.Plugin.InteractiveNotSupported.
pushed to master (773f047)