From 1f091945da51e0c2b600d7af0575b4543bab53f8 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 11 Sep 2024 15:35:42 +0200 Subject: [PATCH 1/4] contrib/nm-vpn-plugin-utils: split editor plugin lookup and load nm-connection-editor (and presumably the Control Center) expects the nm_vpn_editor_plugin_factory() to fail if the editor plugin (the thing that goes into the *-gnome subpackage in Fedora) is not installed. However, factory() never fails, because the plugin is checked for existence only when get_editor() is called. --- src/contrib/nm-vpn-plugin-utils.c | 97 ++++++++++++++++++------------- src/contrib/nm-vpn-plugin-utils.h | 6 +- 2 files changed, 62 insertions(+), 41 deletions(-) diff --git a/src/contrib/nm-vpn-plugin-utils.c b/src/contrib/nm-vpn-plugin-utils.c index 32af40cf12..a9407608db 100644 --- a/src/contrib/nm-vpn-plugin-utils.c +++ b/src/contrib/nm-vpn-plugin-utils.c @@ -11,8 +11,47 @@ /*****************************************************************************/ +char * +nm_vpn_plugin_utils_get_editor_module_path(const char *module_name, GError **error) +{ + gs_free char *module_path = NULL; + gs_free char *dirname = NULL; + Dl_info plugin_info; + + g_return_val_if_fail(module_name, NULL); + g_return_val_if_fail(!error || !*error, NULL); + + /* + * Look for the editor from the same directory this plugin is in. + * Ideally, we'd get our .so name from the NMVpnEditorPlugin if it + * would just have a property with it... + */ + if (!dladdr(nm_vpn_plugin_utils_load_editor, &plugin_info)) { + /* Really a "can not happen" scenario. */ + g_set_error(error, + NM_VPN_PLUGIN_ERROR, + NM_VPN_PLUGIN_ERROR_FAILED, + _("unable to get editor plugin name: %s"), + dlerror()); + } + + dirname = g_path_get_dirname(plugin_info.dli_fname); + module_path = g_build_filename(dirname, module_name, NULL); + + if (!g_file_test(module_path, G_FILE_TEST_EXISTS)) { + g_set_error(error, + G_FILE_ERROR, + G_FILE_ERROR_NOENT, + _("missing plugin file \"%s\""), + module_path); + return NULL; + } + + return g_steal_pointer(&module_path); +} + NMVpnEditor * -nm_vpn_plugin_utils_load_editor(const char *module_name, +nm_vpn_plugin_utils_load_editor(const char *module_path, const char *factory_name, NMVpnPluginUtilsEditorFactory editor_factory, NMVpnEditorPlugin *editor_plugin, @@ -21,48 +60,36 @@ nm_vpn_plugin_utils_load_editor(const char *module_name, GError **error) { + gs_free char *compat_module_path = NULL; static struct { gpointer factory; void *dl_module; - char *module_name; + char *module_path; char *factory_name; } cached = {0}; - NMVpnEditor *editor; - gs_free char *module_path = NULL; - gs_free char *dirname = NULL; - Dl_info plugin_info; + NMVpnEditor *editor; - g_return_val_if_fail(module_name, NULL); + g_return_val_if_fail(module_path, NULL); g_return_val_if_fail(factory_name && factory_name[0], NULL); g_return_val_if_fail(editor_factory, NULL); g_return_val_if_fail(NM_IS_VPN_EDITOR_PLUGIN(editor_plugin), NULL); g_return_val_if_fail(NM_IS_CONNECTION(connection), NULL); g_return_val_if_fail(!error || !*error, NULL); - if (!g_path_is_absolute(module_name)) { - /* - * Load an editor from the same directory this plugin is in. - * Ideally, we'd get our .so name from the NMVpnEditorPlugin if it - * would just have a property with it... - */ - if (!dladdr(nm_vpn_plugin_utils_load_editor, &plugin_info)) { - /* Really a "can not happen" scenario. */ - g_set_error(error, - NM_VPN_PLUGIN_ERROR, - NM_VPN_PLUGIN_ERROR_FAILED, - _("unable to get editor plugin name: %s"), - dlerror()); - } - - dirname = g_path_get_dirname(plugin_info.dli_fname); - module_path = g_build_filename(dirname, module_name, NULL); - } else { - module_path = g_strdup(module_name); + if (!g_path_is_absolute(module_path)) { + /* This presumably means the VPN plugin factory() didn't verify that the plugin is there. + * Now it might be too late to do so. */ + g_warning("VPN plugin bug: load_editor() argument not an absolute path. Continuing..."); + compat_module_path = nm_vpn_plugin_utils_get_editor_module_path(module_path, error); + if (compat_module_path == NULL) + return NULL; + else + module_path = compat_module_path; } - /* we really expect this function to be called with unchanging @module_name + /* we really expect this function to be called with unchanging @module_path * and @factory_name. And we only want to load the module once, hence it would - * be more complicated to accept changing @module_name/@factory_name arguments. + * be more complicated to accept changing @module_path/@factory_name arguments. * * The reason for only loading once is that due to glib types, we cannot create a * certain type-name more then once, so loading the same module or another version @@ -70,12 +97,12 @@ nm_vpn_plugin_utils_load_editor(const char *module_name, * name. * * Only support loading once, any future calls will reuse the handle. To simplify - * that, we enforce that the @factory_name and @module_name is the same. */ + * that, we enforce that the @factory_name and @module_path is the same. */ if (cached.factory) { g_return_val_if_fail(cached.dl_module, NULL); g_return_val_if_fail(cached.factory_name && nm_streq0(cached.factory_name, factory_name), NULL); - g_return_val_if_fail(cached.module_name && nm_streq0(cached.module_name, module_name), + g_return_val_if_fail(cached.module_path && nm_streq0(cached.module_path, module_path), NULL); } else { gpointer factory; @@ -83,14 +110,6 @@ nm_vpn_plugin_utils_load_editor(const char *module_name, dl_module = dlopen(module_path, RTLD_LAZY | RTLD_LOCAL); if (!dl_module) { - if (!g_file_test(module_path, G_FILE_TEST_EXISTS)) { - g_set_error(error, - G_FILE_ERROR, - G_FILE_ERROR_NOENT, - _("missing plugin file \"%s\""), - module_path); - return NULL; - } g_set_error(error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_FAILED, @@ -117,7 +136,7 @@ nm_vpn_plugin_utils_load_editor(const char *module_name, * Thus we just leak the dl_module handle indefinitely. */ cached.factory = factory; cached.dl_module = dl_module; - cached.module_name = g_strdup(module_name); + cached.module_path = g_strdup(module_path); cached.factory_name = g_strdup(factory_name); } diff --git a/src/contrib/nm-vpn-plugin-utils.h b/src/contrib/nm-vpn-plugin-utils.h index b27b75134c..6a6ea0b99c 100644 --- a/src/contrib/nm-vpn-plugin-utils.h +++ b/src/contrib/nm-vpn-plugin-utils.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* - * Copyright (C) 2016 Red Hat, Inc. + * Copyright (C) 2016,2024 Red Hat, Inc. */ #ifndef __NM_VPN_PLUGIN_UTILS_H__ @@ -14,7 +14,9 @@ typedef NMVpnEditor *(NMVpnPluginUtilsEditorFactory) (gpointer factory gpointer user_data, GError **error); -NMVpnEditor *nm_vpn_plugin_utils_load_editor(const char *module_name, +char *nm_vpn_plugin_utils_get_editor_module_path(const char *module_name, GError **error); + +NMVpnEditor *nm_vpn_plugin_utils_load_editor(const char *module_path, const char *factory_name, NMVpnPluginUtilsEditorFactory editor_factory, NMVpnEditorPlugin *editor_plugin, From c09edb8293e48aef12bafdd96f85709957bf47ad Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 11 Sep 2024 15:43:29 +0200 Subject: [PATCH 2/4] libnm/vpn-editor-plugin: formatting fixes Fix up a slightly unpleasant comment and docstring formatting. --- src/libnm-core-public/nm-vpn-editor-plugin.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libnm-core-public/nm-vpn-editor-plugin.h b/src/libnm-core-public/nm-vpn-editor-plugin.h index 4e632244fa..8c8803c6fc 100644 --- a/src/libnm-core-public/nm-vpn-editor-plugin.h +++ b/src/libnm-core-public/nm-vpn-editor-plugin.h @@ -32,9 +32,9 @@ typedef NMVpnEditorPlugin *(*NMVpnEditorPluginFactory)(GError **error); NMVpnEditorPlugin *nm_vpn_editor_plugin_factory(GError **error); #endif -/*****************************************************************************/ -/* Editor plugin interface */ -/*****************************************************************************/ +/* + * Editor plugin interface + */ #define NM_TYPE_VPN_EDITOR_PLUGIN (nm_vpn_editor_plugin_get_type()) #define NM_VPN_EDITOR_PLUGIN(obj) \ @@ -45,10 +45,10 @@ NMVpnEditorPlugin *nm_vpn_editor_plugin_factory(GError **error); /** * NMVpnEditorPluginCapability: - * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_NONE: unknown or no capability - * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_IMPORT: the plugin can import new connections - * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_EXPORT: the plugin can export connections - * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_IPV6: the plugin supports IPv6 addressing + * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_NONE: Unknown or no capability. + * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_IMPORT: The plugin can import new connections. + * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_EXPORT: The plugin can export connections. + * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_IPV6: The plugin supports IPv6 addressing. * * Flags that indicate certain capabilities of the plugin to editor programs. **/ From a1a9a6509e1c5d8bd3f2daeff0896cd7cb76a5f0 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 11 Sep 2024 15:42:20 +0200 Subject: [PATCH 3/4] libnm/vpn-editor-plugin: add a flag to indicate lack of GUI editor --- src/libnm-core-public/nm-vpn-editor-plugin.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libnm-core-public/nm-vpn-editor-plugin.h b/src/libnm-core-public/nm-vpn-editor-plugin.h index 8c8803c6fc..0f9801ef28 100644 --- a/src/libnm-core-public/nm-vpn-editor-plugin.h +++ b/src/libnm-core-public/nm-vpn-editor-plugin.h @@ -49,14 +49,16 @@ NMVpnEditorPlugin *nm_vpn_editor_plugin_factory(GError **error); * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_IMPORT: The plugin can import new connections. * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_EXPORT: The plugin can export connections. * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_IPV6: The plugin supports IPv6 addressing. + * @NM_VPN_EDITOR_PLUGIN_CAPABILITY_NO_EDITOR: The GUI editor plugin is not available. Since: 1.52. * * Flags that indicate certain capabilities of the plugin to editor programs. **/ typedef enum /*< flags >*/ { - NM_VPN_EDITOR_PLUGIN_CAPABILITY_NONE = 0x00, - NM_VPN_EDITOR_PLUGIN_CAPABILITY_IMPORT = 0x01, - NM_VPN_EDITOR_PLUGIN_CAPABILITY_EXPORT = 0x02, - NM_VPN_EDITOR_PLUGIN_CAPABILITY_IPV6 = 0x04 + NM_VPN_EDITOR_PLUGIN_CAPABILITY_NONE = 0x00, + NM_VPN_EDITOR_PLUGIN_CAPABILITY_IMPORT = 0x01, + NM_VPN_EDITOR_PLUGIN_CAPABILITY_EXPORT = 0x02, + NM_VPN_EDITOR_PLUGIN_CAPABILITY_IPV6 = 0x04, + NM_VPN_EDITOR_PLUGIN_CAPABILITY_NO_EDITOR = 0x08, } NMVpnEditorPluginCapability; /* Short display name of the VPN plugin */ From ecf1e8716c1f169134434d208cd62fe9b42fd324 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Wed, 11 Sep 2024 15:35:50 +0200 Subject: [PATCH 4/4] libnm/vpn-editor-plugin: add a comment on a design blunder VPN plugin factory can never fail, it always returns an object, much like g_object_new(). If the (GUI) editor is unavailable, it might be okay for some use cases, notably import()/export(). In such case, the absence of GUI editor is indicated via capability flags. --- src/libnm-core-impl/nm-vpn-editor-plugin.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libnm-core-impl/nm-vpn-editor-plugin.c b/src/libnm-core-impl/nm-vpn-editor-plugin.c index 6181368a5e..fc998b5771 100644 --- a/src/libnm-core-impl/nm-vpn-editor-plugin.c +++ b/src/libnm-core-impl/nm-vpn-editor-plugin.c @@ -299,6 +299,9 @@ _nm_vpn_editor_plugin_load(const char *plugin_name, return NULL; } + /* Note that factory() shouldn't be returning errors or failing. + * We can't change its prototype as it would consistute an ABI break, + * however it returning a failure would indicate a bug in the plugin. */ editor_plugin = factory(&factory_error); if (loaded_before) {