gst/gst.c: Make init_pre and init_post take the full complement of GOptionFunc args so they can return useful GErrors...

Original commit message from CVS:
* gst/gst.c: (init_pre), (scan_and_update_registry),
(ensure_current_registry_nonforking),
(ensure_current_registry_forking), (ensure_current_registry),
(init_post), (gst_debug_help), (gst_deinit):
Make init_pre and init_post take the full complement of GOptionFunc
args so they can return useful GErrors. Make the registry updating
functions do so.
Call _priv_gst_registry_remove_cache_plugins after scanning files to
ensure that the registry we're about to write out doesn't contain
stale information about old-deleted plugin files.
Make _priv_gst_registry_remove_cache_plugins return a boolean so
that deletion of plugin files is considered a registry change.
* gst/gst_private.h:
* gst/gstregistry.c: (gst_registry_finalize),
(gst_registry_remove_features_for_plugin_unlocked),
(gst_registry_remove_plugin), (gst_registry_scan_path_level),
(gst_registry_scan_path),
(_priv_gst_registry_remove_cache_plugins),
(_priv_gst_registry_cleanup):
* gst/gstregistry.h:
Rename _gst_registry_remove_cache_plugins and _gst_registry_cleanup
by adding _priv prefix, so that they won't appear in the global
symbol table. They still do atm though because of #318031. Move the
prototypes to gst_private.h
When removing a plugin, remove all features for that plugin too.
Fixes #340878.

ChangeLog

@@ -1,3 +1,37 @@
+2006-09-27  Jan Schmidt  <thaytan@mad.scientist.com>
+
+	* gst/gst.c: (init_pre), (scan_and_update_registry),
+	(ensure_current_registry_nonforking),
+	(ensure_current_registry_forking), (ensure_current_registry),
+	(init_post), (gst_debug_help), (gst_deinit):
+
+	  Make init_pre and init_post take the full complement of GOptionFunc
+	  args so they can return useful GErrors. Make the registry updating
+	  functions do so.
+
+	  Call _priv_gst_registry_remove_cache_plugins after scanning files to
+	  ensure that the registry we're about to write out doesn't contain
+	  stale information about old-deleted plugin files.
+
+	  Make _priv_gst_registry_remove_cache_plugins return a boolean so
+	  that deletion of plugin files is considered a registry change.
+
+	* gst/gst_private.h:
+	* gst/gstregistry.c: (gst_registry_finalize),
+	(gst_registry_remove_features_for_plugin_unlocked),
+	(gst_registry_remove_plugin), (gst_registry_scan_path_level),
+	(gst_registry_scan_path),
+	(_priv_gst_registry_remove_cache_plugins),
+	(_priv_gst_registry_cleanup):
+	* gst/gstregistry.h:
+	Rename _gst_registry_remove_cache_plugins and _gst_registry_cleanup
+	by adding _priv prefix, so that they won't appear in the global
+	symbol table. They still do atm though because of #318031. Move the
+	prototypes to gst_private.h
+
+	When removing a plugin, remove all features for that plugin too. 
+	Fixes #340878.
+
 2006-09-27  Wim Taymans  <wim@fluendo.com>
 
 	* docs/random/moving-plugins:

gst/gst.c

@@ -142,8 +142,10 @@ gboolean _gst_disable_segtrap = FALSE;
 static gboolean _gst_enable_registry_fork = DEFAULT_FORK;
 
 static void load_plugin_func (gpointer data, gpointer user_data);
-static gboolean init_pre (void);
+static gboolean init_pre (GOptionContext * context, GOptionGroup * group,
-static gboolean init_post (void);
+    gpointer data, GError ** error);
+static gboolean init_post (GOptionContext * context, GOptionGroup * group,
+    gpointer data, GError ** error);
 static gboolean parse_goption_arg (const gchar * s_opt,
     const gchar * arg, gpointer data, GError ** err);
 
@@ -486,7 +488,8 @@ split_and_iterate (const gchar * stringlist, gchar * separator, GFunc iterator,
 
 /* we have no fail cases yet, but maybe in the future */
 static gboolean
-init_pre (void)
+init_pre (GOptionContext * context, GOptionGroup * group, gpointer data,
+    GError ** error)
 {
   /* GStreamer was built against a GLib >= 2.8 and is therefore not doing
    * the refcount hack. Check that it isn't being run against an older GLib */
@@ -577,7 +580,7 @@ static GstPluginDesc plugin_desc = {
  */
 static gboolean
 scan_and_update_registry (GstRegistry * default_registry,
-    const gchar * registry_file, gboolean write_changes)
+    const gchar * registry_file, gboolean write_changes, GError ** error)
 {
   const gchar *plugin_path;
   gboolean changed = FALSE;
@@ -644,34 +647,38 @@ scan_and_update_registry (GstRegistry * default_registry,
     g_strfreev (list);
   }
 
+  /* Remove cached plugins so stale info is cleared. */
+  changed |= _priv_gst_registry_remove_cache_plugins (default_registry);
+
   if (!changed) {
-    GST_DEBUG ("registry cache has not changed");
+    GST_INFO ("Registry cache has not changed");
     return TRUE;
   }
 
   if (!write_changes) {
-    GST_DEBUG ("not trying to write registry cache changes to file");
+    GST_INFO ("Registry cached changed, but writing is disabled. Not writing.");
     return TRUE;
   }
 
-  GST_DEBUG ("writing registry cache");
+  GST_INFO ("Registry cache changed. Writing new registry cache");
   if (!gst_registry_xml_write_cache (default_registry, registry_file)) {
-    g_warning ("Problem writing registry cache to %s: %s", registry_file,
+    g_set_error (error, GST_CORE_ERROR, GST_CORE_ERROR_FAILED,
-        g_strerror (errno));
+        _("Error writing registry cache to %s: %s"),
+        registry_file, g_strerror (errno));
     return FALSE;
   }
 
-  GST_DEBUG ("registry cache written successfully");
+  GST_INFO ("Registry cache written successfully");
   return TRUE;
 }
 
 static gboolean
 ensure_current_registry_nonforking (GstRegistry * default_registry,
-    const gchar * registry_file)
+    const gchar * registry_file, GError ** error)
 {
   /* fork() not available */
-  GST_DEBUG ("updating registry cache");
+  GST_DEBUG ("Updating registry cache in-process");
-  scan_and_update_registry (default_registry, registry_file, TRUE);
+  scan_and_update_registry (default_registry, registry_file, TRUE, error);
   return TRUE;
 }
 
@@ -679,14 +686,14 @@ ensure_current_registry_nonforking (GstRegistry * default_registry,
  * TRUE immediatly */
 static gboolean
 ensure_current_registry_forking (GstRegistry * default_registry,
-    const gchar * registry_file)
+    const gchar * registry_file, GError ** error)
 {
 #ifdef HAVE_FORK
   pid_t pid;
 
   /* We fork here, and let the child read and possibly rebuild the registry.
    * After that, the parent will re-read the freshly generated registry. */
-  GST_DEBUG ("forking");
+  GST_DEBUG ("forking to update registry");
   pid = fork ();
   if (pid == -1) {
     GST_ERROR ("Failed to fork()");
@@ -698,8 +705,8 @@ ensure_current_registry_forking (GstRegistry * default_registry,
 
     /* this is the child */
     GST_DEBUG ("child reading registry cache");
-    res = scan_and_update_registry (default_registry, registry_file, TRUE);
+    res =
-    _gst_registry_remove_cache_plugins (default_registry);
+        scan_and_update_registry (default_registry, registry_file, TRUE, NULL);
 
     /* need to use _exit, so that any exit handlers registered don't
      * bring down the main program */
@@ -719,6 +726,9 @@ ensure_current_registry_forking (GstRegistry * default_registry,
     GST_DEBUG ("parent done waiting on child");
     if (ret == -1) {
       GST_ERROR ("error during waitpid: %s", g_strerror (errno));
+      g_set_error (error, GST_CORE_ERROR, GST_CORE_ERROR_FAILED,
+          _("Error re-scanning registry %s: %s"),
+          ", waitpid returned error", g_strerror (errno));
       return FALSE;
     }
 
@@ -726,8 +736,14 @@ ensure_current_registry_forking (GstRegistry * default_registry,
       if (WIFSIGNALED (status)) {
         GST_ERROR ("child did not exit normally, terminated by signal %d",
             WTERMSIG (status));
+        g_set_error (error, GST_CORE_ERROR, GST_CORE_ERROR_FAILED,
+            _("Error re-scanning registry %s: %d"),
+            ", child terminated by signal", WTERMSIG (status));
       } else {
         GST_ERROR ("child did not exit normally, status: %d", status);
+        g_set_error (error, GST_CORE_ERROR, GST_CORE_ERROR_FAILED,
+            _("Error re-scanning registry %s: %d"),
+            ", child did not exit normally, status", status);
       }
       return FALSE;
     }
@@ -739,8 +755,8 @@ ensure_current_registry_forking (GstRegistry * default_registry,
       GST_DEBUG ("parent reading registry cache");
       gst_registry_xml_read_cache (default_registry, registry_file);
     } else {
-      GST_DEBUG ("parent re-scanning registry");
+      GST_DEBUG ("parent re-scanning registry. Ignoring errors.");
-      scan_and_update_registry (default_registry, registry_file, FALSE);
+      scan_and_update_registry (default_registry, registry_file, FALSE, NULL);
     }
   }
 #endif /* HAVE_FORK */
@@ -748,7 +764,7 @@ ensure_current_registry_forking (GstRegistry * default_registry,
 }
 
 static gboolean
-ensure_current_registry (void)
+ensure_current_registry (GError ** error)
 {
   char *registry_file;
   GstRegistry *default_registry;
@@ -777,10 +793,12 @@ ensure_current_registry (void)
   /* now check registry with or without forking */
   if (do_fork) {
     GST_DEBUG ("forking for registry rebuild");
-    ret = ensure_current_registry_forking (default_registry, registry_file);
+    ret = ensure_current_registry_forking (default_registry, registry_file,
+        error);
   } else {
     GST_DEBUG ("requested not to fork for registry rebuild");
-    ret = ensure_current_registry_nonforking (default_registry, registry_file);
+    ret = ensure_current_registry_nonforking (default_registry, registry_file,
+        error);
   }
 
   g_free (registry_file);
@@ -801,7 +819,8 @@ ensure_current_registry (void)
  *   we might and then it's nice to be able to return that
  */
 static gboolean
-init_post (void)
+init_post (GOptionContext * context, GOptionGroup * group, gpointer data,
+    GError ** error)
 {
   GLogLevelFlags llf;
 
@@ -854,7 +873,7 @@ init_post (void)
   gst_initialized = TRUE;
 
 #ifndef GST_DISABLE_REGISTRY
-  if (!ensure_current_registry ())
+  if (!ensure_current_registry (error))
     return FALSE;
 #endif /* GST_DISABLE_REGISTRY */
 
@@ -894,7 +913,8 @@ gst_debug_help (void)
   GSList *list, *walk;
   GList *list2, *g;
 
-  if (!init_post ())
+  /* Need to ensure the registry is loaded to get debug categories */
+  if (!init_post (NULL, NULL, NULL, NULL))
     exit (1);
 
   list2 = gst_registry_plugin_filter (gst_registry_get_default (),
@@ -1074,7 +1094,7 @@ gst_deinit (void)
   gst_object_unref (clock);
   gst_object_unref (clock);
 
-  _gst_registry_cleanup ();
+  _priv_gst_registry_cleanup ();
 
   gst_initialized = FALSE;
   GST_INFO ("deinitialized GStreamer");

gst/gst_private.h

@@ -37,6 +37,9 @@ extern const char             g_log_domain_gstreamer[];
 #include <stdlib.h>
 #include <string.h>
 
+/* Needed for GstRegistry * */
+#include "gstregistry.h"
+
 G_BEGIN_DECLS
 
 gboolean __gst_in_valgrind (void);
@@ -44,6 +47,10 @@ gboolean __gst_in_valgrind (void);
 /* Initialize GStreamer private quark storage */
 void _priv_gst_quarks_initialize (void);
 
+/* Private registry functions */
+gboolean _priv_gst_registry_remove_cache_plugins (GstRegistry *registry);
+void _priv_gst_registry_cleanup (void);
+
 /*** debugging categories *****************************************************/
 
 #ifndef GST_DISABLE_GST_DEBUG

gst/gstregistry.c

@@ -180,7 +180,7 @@ gst_registry_finalize (GObject * object)
     if (plugin) {
       GST_DEBUG_OBJECT (registry, "removing plugin %s",
           gst_plugin_get_name (plugin));
-      gst_registry_remove_plugin (registry, plugin);
+      gst_object_unref (plugin);
     }
     p = g_list_next (p);
   }
@@ -194,7 +194,9 @@ gst_registry_finalize (GObject * object)
     GstPluginFeature *feature = f->data;
 
     if (feature) {
-      gst_registry_remove_feature (registry, feature);
+      GST_DEBUG_OBJECT (registry, "removing feature %p (%s)",
+          feature, gst_plugin_feature_get_name (feature));
+      gst_object_unref (feature);
     }
     f = g_list_next (f);
   }
@@ -344,6 +346,33 @@ gst_registry_add_plugin (GstRegistry * registry, GstPlugin * plugin)
   return TRUE;
 }
 
+static void
+gst_registry_remove_features_for_plugin_unlocked (GstRegistry * registry,
+    GstPlugin * plugin)
+{
+  GList *f;
+
+  g_return_if_fail (GST_IS_REGISTRY (registry));
+  g_return_if_fail (GST_IS_PLUGIN (plugin));
+
+  /* Remove all features for this plugin */
+  f = registry->features;
+  while (f != NULL) {
+    GList *next = g_list_next (f);
+    GstPluginFeature *feature = f->data;
+
+    if (feature && !strcmp (feature->plugin_name, gst_plugin_get_name (plugin))) {
+      GST_DEBUG_OBJECT (registry, "removing feature %p (%s) for plugin %s",
+          feature, gst_plugin_feature_get_name (feature),
+          gst_plugin_get_name (plugin));
+
+      registry->features = g_list_delete_link (registry->features, f);
+      gst_object_unref (feature);
+    }
+    f = next;
+  }
+}
+
 /**
  * gst_registry_remove_plugin:
  * @registry: the registry to remove the plugin from
@@ -359,8 +388,12 @@ gst_registry_remove_plugin (GstRegistry * registry, GstPlugin * plugin)
   g_return_if_fail (GST_IS_REGISTRY (registry));
   g_return_if_fail (GST_IS_PLUGIN (plugin));
 
+  GST_DEBUG_OBJECT (registry, "removing plugin %p (%s)",
+      plugin, gst_plugin_get_name (plugin));
+
   GST_OBJECT_LOCK (registry);
   registry->plugins = g_list_remove (registry->plugins, plugin);
+  gst_registry_remove_features_for_plugin_unlocked (registry, plugin);
   GST_OBJECT_UNLOCK (registry);
   gst_object_unref (plugin);
 }
@@ -844,8 +877,6 @@ gst_registry_scan_path_level (GstRegistry * registry, const gchar * path,
 
   g_dir_close (dir);
 
-  GST_DEBUG_OBJECT (registry, "registry changed? %d", changed);
-
   return changed;
 }
 
@@ -871,19 +902,22 @@ gst_registry_scan_path (GstRegistry * registry, const gchar * path)
   GST_DEBUG_OBJECT (registry, "scanning path %s", path);
   changed = gst_registry_scan_path_level (registry, path, 10);
 
-  GST_DEBUG_OBJECT (registry, "registry changed? %d", changed);
+  GST_DEBUG_OBJECT (registry, "registry changed in path %s: %d", path, changed);
 
   return changed;
 }
 
-void
+/* Unref all plugins marked 'cached', to clear old plugins that no
-_gst_registry_remove_cache_plugins (GstRegistry * registry)
+ * longer exist. Returns TRUE if any plugins were removed */
+gboolean
+_priv_gst_registry_remove_cache_plugins (GstRegistry * registry)
 {
   GList *g;
   GList *g_next;
   GstPlugin *plugin;
+  gboolean changed = FALSE;
 
-  g_return_if_fail (GST_IS_REGISTRY (registry));
+  g_return_val_if_fail (GST_IS_REGISTRY (registry), FALSE);
 
   GST_OBJECT_LOCK (registry);
 
@@ -895,16 +929,16 @@ _gst_registry_remove_cache_plugins (GstRegistry * registry)
     if (plugin->flags & GST_PLUGIN_FLAG_CACHED) {
       GST_DEBUG_OBJECT (registry, "removing cached plugin \"%s\"",
           GST_STR_NULL (plugin->filename));
-      /* seems it would be sufficient just to do a delete_link for o(1) deletion
+      registry->plugins = g_list_delete_link (registry->plugins, g);
-       * -- we have to traverse the whole list anyway, and dup entries (if
-       * possible) should have dup refcounts */
-      registry->plugins = g_list_remove (registry->plugins, plugin);
       gst_object_unref (plugin);
+      changed = TRUE;
     }
     g = g_next;
   }
 
   GST_OBJECT_UNLOCK (registry);
+
+  return changed;
 }
 
 
@@ -935,8 +969,9 @@ gst_registry_get_feature_list_by_plugin (GstRegistry * registry,
       _gst_plugin_feature_filter_plugin_name, FALSE, (gpointer) name);
 }
 
+/* Unref and delete the default registry */
 void
-_gst_registry_cleanup ()
+_priv_gst_registry_cleanup ()
 {
   GstRegistry *registry;
 

gst/gstregistry.h

@@ -109,10 +109,6 @@ GstPluginFeature * 	gst_registry_lookup_feature 	(GstRegistry *registry, const c
 gboolean 		gst_registry_xml_read_cache 	(GstRegistry * registry, const char *location);
 gboolean 		gst_registry_xml_write_cache 	(GstRegistry * registry, const char *location);
 
-void 			_gst_registry_remove_cache_plugins (GstRegistry *registry);
-
-void                    _gst_registry_cleanup           (void);
-
 /* convinience defines for the default registry */
 
 /**