Re: [PATCH 0/2] reload pluign

[Iago Toral <itoral igalia com>]

On Fri, 20 Aug 2010 12:33:05 +0200, Víctor M. Jáquez L.
<vjaquez igalia com> wrote:
> On Fri, Aug 20, 2010 at 11:43:01AM +0200, Víctor M. Jáquez L. wrote:
>> This patches (basically the second one) adds the functionality of
>> plugins reloading in the grilo-test-ui application.
>>
>> I think this is useful when one is developing a plugin and want to
>> test it without wasting time restarting the test-ui app.
> 
> I'd tested the functionality and it didn't work :( I guess extra bits are
> needed in order to reload the module on-the-fly

I was looking into this as well, and reviewing the plugin unloading
functionality I found a few obvious issues. This is a set of patches
improving plugin shutdown, the  first calls g_module_close when a plugin
is to be unloaded (ups!), the second one makes sure that unloading a
module also unloads all the sources spawned by it.

The third patch is for the test-ui, replaces the non-functional
plugin-reloading feature by a simple plugin-shutdown feature that at
least can be used to test plugin and source shutdown.

Iago

From 5b7b9e7c089c44bce9cbc73c88f54b0530fe279d Mon Sep 17 00:00:00 2001
From: Iago Toral Quiroga <itoral igalia com>
Date: Wed, 1 Sep 2010 14:43:48 +0200
Subject: [PATCH] core: unload plugins properly by calling g_module_close when appropriate.

---
 src/grl-plugin-registry.c |    5 +++++
 src/grl-plugin-registry.h |    3 +++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/grl-plugin-registry.c b/src/grl-plugin-registry.c
index b450a60..28ce7d1 100644
--- a/src/grl-plugin-registry.c
+++ b/src/grl-plugin-registry.c
@@ -447,6 +447,8 @@ grl_plugin_registry_load (GrlPluginRegistry *registry, const gchar *path)
     return FALSE;
   }
 
+  plugin->module = module;
+
   g_debug ("Loaded plugin '%s' from '%s'", plugin->info.id, path);
 
   return TRUE;
@@ -647,6 +649,9 @@ grl_plugin_registry_unload (GrlPluginRegistry *registry,
   if (plugin->plugin_deinit) {
     plugin->plugin_deinit ();
   }
+  if (plugin->module) {
+    g_module_close (plugin->module);
+  }
 }
 
 GrlKeyID
diff --git a/src/grl-plugin-registry.h b/src/grl-plugin-registry.h
index 45aa114..c82a956 100644
--- a/src/grl-plugin-registry.h
+++ b/src/grl-plugin-registry.h
@@ -85,6 +85,7 @@
     .info = { id, NULL, NULL, 0 },					\
     .plugin_init = init,						\
     .plugin_deinit = deinit,						\
+    .module = NULL							\
   }
 
 /* Plugin descriptor */
@@ -116,6 +117,7 @@ typedef struct _GrlPluginDescriptor  GrlPluginDescriptor;
 * the #GrlMediaPlugins provided
 * @plugin_deinit: function to execute when the registry needs
 * to dispose the module.
+* @module: the #GModule instance.
 *
 * This structure is used for the module loader
 */
@@ -123,6 +125,7 @@ struct _GrlPluginDescriptor {
   GrlPluginInfo info;
   gboolean (*plugin_init) (GrlPluginRegistry *, const GrlPluginInfo *, GList *);
   void (*plugin_deinit) (void);
+  GModule *module;
 };
 
 /* Plugin ranks */
-- 
1.6.0.4

From 109b6c42c0e8c0f3fdc6b7129b043a8de93667ec Mon Sep 17 00:00:00 2001
From: Iago Toral Quiroga <itoral igalia com>
Date: Thu, 2 Sep 2010 14:49:00 +0200
Subject: [PATCH] core: when unloading a plugin, shutdown any sources it may have spawned.

---
 src/grl-plugin-registry.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/src/grl-plugin-registry.c b/src/grl-plugin-registry.c
index 28ce7d1..867c895 100644
--- a/src/grl-plugin-registry.c
+++ b/src/grl-plugin-registry.c
@@ -635,16 +635,34 @@ grl_plugin_registry_unload (GrlPluginRegistry *registry,
                             const gchar *plugin_id)
 {
   GrlPluginDescriptor *plugin;
+  GrlMediaPlugin **sources;
+  gint i;
+
+  g_debug ("grl_plugin_registry_unload: %s", plugin_id);
 
   g_return_if_fail (GRL_IS_PLUGIN_REGISTRY (registry));
   g_return_if_fail (plugin_id != NULL);
 
+  /* First check the plugin is valid  */
   plugin = g_hash_table_lookup (registry->priv->plugins, plugin_id);
   if (!plugin) {
     g_warning ("Could not deinit plugin '%s'. Plugin not found.", plugin_id);
     return;
   }
 
+  /* Second, shut down any sources spawned by this plugin */
+  g_debug ("Shutting down sources spawned by '%s'", plugin_id);
+  sources = grl_plugin_registry_get_sources (registry, FALSE);
+  for (i=0; sources[i] != NULL; i++) {
+    const gchar *id; 
+    id = grl_media_plugin_get_id (sources[i]);
+    if (!strcmp (plugin_id, id)) {
+      grl_plugin_registry_unregister_source (registry, sources[i]);
+    }
+  }
+  g_free (sources);
+
+  /* Third, shut down the plugin */
   g_debug ("Unloading plugin '%s'", plugin_id);
   if (plugin->plugin_deinit) {
     plugin->plugin_deinit ();
-- 
1.6.0.4

From 1009e7c34fd4e5f66989d98401ef397299e11940 Mon Sep 17 00:00:00 2001
From: Iago Toral Quiroga <itoral igalia com>
Date: Thu, 2 Sep 2010 15:27:32 +0200
Subject: [PATCH] test-ui: reloading functionality does not work as intended. This commit
 replaces plugin-reloading functionality by plugin-shutdown.

---
 tools/grilo-test-ui/main.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/tools/grilo-test-ui/main.c b/tools/grilo-test-ui/main.c
index f3896ea..6331ab7 100644
--- a/tools/grilo-test-ui/main.c
+++ b/tools/grilo-test-ui/main.c
@@ -171,7 +171,7 @@ static const gchar *ui_definition =
 " <menubar name='MainMenu'>"
 "  <menu name='FileMenu' action='FileMenuAction' >"
 "   <menuitem name='Authorize Flickr' action='AuthorizeFlickrAction' />"
-"   <menuitem name='Reload plugins' action='ReloadPluginsAction' />"
+"   <menuitem name='Shutdown plugins' action='ShutdownPluginsAction' />"
 "   <menuitem name='Quit' action='QuitAction' />"
 "  </menu>"
 " </menubar>"
@@ -183,15 +183,15 @@ static void quit_cb (GtkAction *action);
 static gchar *authorize_flickr (void);
 static void authorize_flickr_cb (GtkAction *action);
 
-static void reload_plugins_cb (GtkAction *action);
-static void reload_plugins (void);
+static void shutdown_plugins_cb (GtkAction *action);
+static void shutdown_plugins (void);
 
 static GtkActionEntry entries[] = {
   { "FileMenuAction", NULL, "_File" },
   { "AuthorizeFlickrAction", GTK_STOCK_CONNECT, "_Authorize Flickr", NULL,
     "AuthorizeFlickr", G_CALLBACK (authorize_flickr_cb) },
-  { "ReloadPluginsAction", GTK_STOCK_REFRESH, "_Reload Plugins", "<control>R",
-    "ReloadPlugins", G_CALLBACK (reload_plugins_cb) },
+  { "ShutdownPluginsAction", GTK_STOCK_REFRESH, "_Shutdown Plugins", NULL,
+    "ShutdownPlugins", G_CALLBACK (shutdown_plugins_cb) },
   { "QuitAction", GTK_STOCK_QUIT, "_Quit", "<control>Q",
     "Quit", G_CALLBACK (quit_cb) }
 };
@@ -209,9 +209,9 @@ authorize_flickr_cb (GtkAction *action)
 }
 
 static void
-reload_plugins_cb (GtkAction *action)
+shutdown_plugins_cb (GtkAction *action)
 {
-  reload_plugins ();
+  shutdown_plugins ();
 }
 
 static GtkTreeModel *
@@ -1701,7 +1701,7 @@ source_removed_cb (GrlPluginRegistry *registry,
 {
   g_debug ("Source '%s' is gone",
 	   grl_metadata_source_get_name (GRL_METADATA_SOURCE (source)));
-
+  
   if (!ui_state->cur_source && !ui_state->cur_container) {
     /* If showing the plugin list, refresh it */
     show_plugins ();
@@ -1732,7 +1732,7 @@ load_plugins (void)
 }
 
 static void
-reload_plugins (void)
+shutdown_plugins (void)
 {
   GrlMediaPlugin **sources;
   GrlPluginRegistry *registry;
@@ -1741,19 +1741,20 @@ reload_plugins (void)
   /* Cancel previous operation, if any */
   cancel_current_operation ();
 
+  /* Let's make sure we don't have references to stuff
+     we are about to shut down */
+  clear_panes ();
+
+  /* Shut down the plugins now */
   registry = grl_plugin_registry_get_default ();
   sources = grl_plugin_registry_get_sources (registry, FALSE);
-
   for (i = 0; sources[i]; i++) {
-    grl_plugin_registry_unload (registry,
-                                grl_media_plugin_get_name (sources[i]));
-    grl_plugin_registry_unregister_source (registry, sources[i]);
+    const gchar *plugin_id;
+    plugin_id = grl_media_plugin_get_id (sources[i]);
+    grl_plugin_registry_unload (registry, plugin_id);
   }
-
   g_free (sources);
 
-  load_plugins ();
-
   reset_ui ();
 }
 
-- 
1.6.0.4