GNOME Bugzilla – Bug 628898
Grilo could do with a function to load a plugin by ID
Last modified: 2011-04-13 10:00:34 UTC
If you want to load a specific plugin in Grilo, from the default plugin directory, your only choice is to load all plugins (via grl_plugin_registry_load_all) and then lookup the plugin you wanted. Instead, it'd be good to have a grl_plugin_registry_load_by_id function, or something similar, to be able to load just a single plugin from the default plugins directory.
Funny, I suggested to add that yesterday in the mailing list :)
Created attachment 185538 [details] [review] core: Preload available plugins' information When initializing Grilo, load all plugins information stored in XML files. This will allow to know in advance what are the plugins available in the system. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185539 [details] [review] core: Check if plugin is already loaded When loading a plugin, check if the plugin is already loaded. If so then skip it and return an error. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185540 [details] [review] core: Add function to load plugin by ID Add grl_plugin_registry_load_by_id(), which allows to load a specific plugin by its identifier. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=628898 Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185541 [details] [review] all: Add "module" information Add module filename information so plugins can be loaded later by its ID. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Review of attachment 185538 [details] [review]: ::: src/grl-plugin-registry.c @@ +506,3 @@ + if (!plugin_info) { + plugin_info = g_new0 (GrlPluginInfo, 1); + plugin_info->id = plugin->plugin_id; what if plugin_info != NULL? I understand that would only happen if someone attempts to load the same plugin twice. I would fail with an error in that case. @@ +518,3 @@ + if (plugin_info->filename) { + g_free (plugin_info->filename); + } Assuming we do not let users load the same plugin twice this check does not make sense and we should remove the "if (plugin_ifo->filename)" part. @@ +804,3 @@ plugin->plugin_deinit (); } Where do you free plugin_info->filename now?
(In reply to comment #6) > Review of attachment 185538 [details] [review]: > > ::: src/grl-plugin-registry.c > @@ +506,3 @@ > + if (!plugin_info) { > + plugin_info = g_new0 (GrlPluginInfo, 1); > + plugin_info->id = plugin->plugin_id; > > what if plugin_info != NULL? I understand that would only happen if someone > attempts to load the same plugin twice. I would fail with an error in that > case. Never mind this, I had missed the info preloading part in my previous review.
Review of attachment 185540 [details] [review]: ::: src/grl-plugin-registry.c @@ +56,3 @@ +#define GRL_PLUGIN_INFO_MODULE_KEY "module" + I'd remove the "_KEY" suffix (info on the rationale below) @@ +667,3 @@ + * @plugin_id: plugin identifier + * @error: eror return location or @NULL to ignore + * eror -> error @@ +672,3 @@ + * This requires the XML plugin information to define a "module" key with the + * name of the module which contains the plugin. + I think this is not descriptive enough. I'd say: This requires the XML plugin information file to define a "module" key with the name of the module that provides the plugin or the absolute path of the actual module file. @@ +696,3 @@ + GRL_CORE_ERROR, + GRL_CORE_ERROR_LOAD_PLUGIN_FAILED, + "Unknown plugin '%s'", plugin_id); I'd use the following error message instead: "There is no information about a plugin with id '%s'" The reason is that the plugin may be known (that is, it may be in GRL_PLUGIN_PATH), but we may not have its xml data, so the problem is not really that we don't know the plugin, it is that we do not have the required xml file. @@ +710,3 @@ + GRL_PLUGIN_INFO_MODULE_KEY); + } + Let's not use the _KEY suffix for everything, when we talk about "keys" we usually mean the metadata keys so I would rather keep that suffix only for those. I think that would be less misleading in general. @@ +719,3 @@ + return FALSE; + } + I would use this error string instead: "Unknown module file for plugin with id '%s'"
(In reply to comment #6) > @@ +518,3 @@ > + if (plugin_info->filename) { > + g_free (plugin_info->filename); > + } > > Assuming we do not let users load the same plugin twice this check does not > make sense and we should remove the "if (plugin_ifo->filename)" part. > Yes, we do not allow to load the same plugin twice. But user can load all plugins, then unload one of them and load a specific version for the unloaded one (a .so in a specific place). That's why filename is freed (is pointing to "standard" module) and assigned again to the new module. But user > @@ +804,3 @@ > plugin->plugin_deinit (); > } > > > Where do you free plugin_info->filename now? plugin_info->filename is only freed when the plugin is loaded again (which only happens when plugin is previously unloaded). The reason to keep filename is to speed up the "load_by_id()": if filename has already a value, it is not needed to search for the plugin throughout the plugin directories.
(In reply to comment #8) > Review of attachment 185540 [details] [review]: I'm fine with them. I'll do the changes.
(In reply to comment #9) > (In reply to comment #6) > > @@ +518,3 @@ > > + if (plugin_info->filename) { > > + g_free (plugin_info->filename); > > + } > > > > Assuming we do not let users load the same plugin twice this check does not > > make sense and we should remove the "if (plugin_ifo->filename)" part. > > > > Yes, we do not allow to load the same plugin twice. > > But user can load all plugins, then unload one of them and load a specific > version for the unloaded one (a .so in a specific place). That's why filename > is freed (is pointing to "standard" module) and assigned again to the new > module. > > > But user > > @@ +804,3 @@ > > plugin->plugin_deinit (); > > } > > > > > > Where do you free plugin_info->filename now? > > plugin_info->filename is only freed when the plugin is loaded again (which only > happens when plugin is previously unloaded). > > The reason to keep filename is to speed up the "load_by_id()": if filename has > already a value, it is not needed to search for the plugin throughout the > plugin directories. Note that an alternative would be freeing filename when plugin is unloaded. This way we could get rid of the "if (plugin_info->filename)" sentence as suggested (it would be always NULL in the moment of loading the plugin), and we wouldn't have the filename string in memory when plugin is not loaded. As "drawback", we would always need to search for the plugin when invoking load_by_id() function (with current code, if plugin was previously loaded and unloaded after, we wouldn't need to search for the plugin, as filename will be still there). Taking in account a use-case where the application shows user the available plugins, which can be activated/deactivated by user, I kept the current approach. If you really prefer the other one, that's fine for me.
Created attachment 185728 [details] [review] Add function to load plugins by ID This new patch version that applies comment #8
(In reply to comment #9) > (In reply to comment #6) > > @@ +518,3 @@ > > + if (plugin_info->filename) { > > + g_free (plugin_info->filename); > > + } > > > > Assuming we do not let users load the same plugin twice this check does not > > make sense and we should remove the "if (plugin_ifo->filename)" part. > > > > Yes, we do not allow to load the same plugin twice. > > But user can load all plugins, then unload one of them and load a specific > version for the unloaded one (a .so in a specific place). That's why filename > is freed (is pointing to "standard" module) and assigned again to the new > module. We do not support that use case actually: at the moment we cannot unload a plugin and then load a plugin that defines the same GType. Not that it matters a lot, so feel free to keep this if you want, but I think it is an unnecessary complication for a use case that is currently unsupported. > But user > > @@ +804,3 @@ > > plugin->plugin_deinit (); > > } > > > > > > Where do you free plugin_info->filename now? > > plugin_info->filename is only freed when the plugin is loaded again (which only > happens when plugin is previously unloaded). > > The reason to keep filename is to speed up the "load_by_id()": if filename has > already a value, it is not needed to search for the plugin throughout the > plugin directories. Ok, I see. I still think the complexity it adds is not worth it, since this only comes in handy after a certain plugin has been unloaded and then it is loaded again. But anyway, if you want to keep this, it's fine, but please add a proper comment so others understand the purpose of that code.
Created attachment 185806 [details] [review] core: Preload available plugins' information I've decided to keep as it is, as I don't think it is adding any kind of complication. But I've added also a comment to explain why plugin_info->filename can have a value. Thanks for the comments
Created attachment 185807 [details] [review] core: Fix crash when unloading the plugins Remove the plugin from the list of plugins before closing the module. Otherwise, the key will be freed so removing from hash table will crash. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185808 [details] [review] core: Make modules resident Do not really unload the modules from memory when unloading the plugins. This will prevent problems with glib's types if plugins are reloaded again. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185809 [details] [review] test-ui: allow reloading of plugins Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
(In reply to comment #13) > > But user can load all plugins, then unload one of them and load a specific > > version for the unloaded one (a .so in a specific place). That's why filename > > is freed (is pointing to "standard" module) and assigned again to the new > > module. > > We do not support that use case actually: at the moment we cannot unload a > plugin and then load a plugin that defines the same GType. Not that it matters > a lot, so feel free to keep this if you want, but I think it is an unnecessary > complication for a use case that is currently unsupported. > I didn't know we do not support this case. Indeed, unloading the plugins and reloading them again. Providing a proper fix is too complex for this very specific case, but I still think we should do something to prevent the crash, either with code or adding a big warning in the documentation. In any case, I found a very simple way of preventing this crash, which is actually the patch in comment #16 (the patch in comment #15 fixes a problem with unloading plugins, and patch in comment #17 just allow to reload again the plugins in test-ui): make the modules resident. From grilo/users pov, making modules resident is totally transparent: g_module_open() and g_module_close() work as usual, sources "disappear" when they are unloaded, and appear again when they are loaded. The drawback is that, as modules are resident, they are always loaded in memory, so unloading them will not free the space occupy by the modules. But still I think it is worth to merge this patch, as it will prevent crashes on case of loading/unloading plugins (as a matter of fact, I'm thinking in an application where available plugins are shown to the users so they can be activated/deactivated).
(In reply to comment #13) > > But user can load all plugins, then unload one of them and load a specific > > version for the unloaded one (a .so in a specific place). That's why filename > > is freed (is pointing to "standard" module) and assigned again to the new > > module. > > We do not support that use case actually: at the moment we cannot unload a > plugin and then load a plugin that defines the same GType. Not that it matters > a lot, so feel free to keep this if you want, but I think it is an unnecessary > complication for a use case that is currently unsupported. > I didn't know we do not support this case. Indeed, unloading the plugins and reloading them again leads to a crash. Providing a proper fix is too complex for this very specific case, but I still think we should do something to prevent the crash, either with code or adding a big warning in the documentation. In any case, I found a very simple way of preventing this crash, which is actually the patch in comment #16 (the patch in comment #15 fixes a problem with unloading plugins, and patch in comment #17 just allow to reload again the plugins in test-ui): make the modules resident. From grilo/users pov, making modules resident is totally transparent: g_module_open() and g_module_close() work as usual, sources "disappear" when they are unloaded, and appear again when they are loaded. The drawback is that, as modules are resident, they are always loaded in memory, so unloading them will not free the space occupy by the modules. But still I think it is worth to merge this patch, as it will prevent crashes on case of loading/unloading plugins (as a matter of fact, I'm thinking in an application where available plugins are shown to the users so they can be activated/deactivated).
(In reply to comment #19) > From grilo/users pov, making modules resident is totally transparent: > g_module_open() and g_module_close() work as usual, sources "disappear" when > they are unloaded, and appear again when they are loaded. > > The drawback is that, as modules are resident, they are always loaded in > memory, so unloading them will not free the space occupy by the modules. But > still I think it is worth to merge this patch, as it will prevent crashes on > case of loading/unloading plugins (as a matter of fact, I'm thinking in an > application where available plugins are shown to the users so they can be > activated/deactivated). Not ideal, but I guess we can live with that for now, it is better than what we have at the moment.
Thanks for your reviews. All patches have been pushed in core: a7182f9 test-ui: allow reloading of plugins b653bc9 core: Make modules resident f861c70 core: Fix crash when unloading the plugins 60c3262 core: Add function to load plugin by ID 19ecd89 core: Check if plugin is already loaded 46948e9 core: Preload available plugins' information and in plugins: e8ef9fb all: Add "module" information