Re: Making GModule use GError.

[Sebastian Wilhelmi <wilhelmi ira uka de>]

Hi Tim,

> > While GModule would use GError if written from scratch, I tend to
> > think that the consistency benefits of changing at this point
> > are not worth breaking substantial amounts of existing code;
> > I don't think there is any real gain in functionality from
> > this.
> 
> i don't even think i'd use gerror for gmodule if writing it from
> scratch right now. the reason being that its error message can
> hardly be categrized since we just get strings on some platforms.
> while categorization ala "what function stuff failed in, e.g.
> loading or symbol matches" is possible, it doesn't really provide
> sustantial benefits, since that information is already available
> for the caller.

While I still think g_module_error is bad, now that you voted (and now that we
counted twice both manually and by machine.. ;-) I suppose this change is
dead. 

> > (I guess the one gain in functionality is proper threaded operation,
> > but dlerror() probably uses per-thread data on thread-safe
> > C libraries anyways.)
> 
> gmodule does an immediate copy of the string returned by dlerror(),
> but now you mention it, i'm not sure all platforms are 100% thread
> safe for their dl* functionality. i think adding a global recursive
> lock in gmodule.c is probably a good idea (recursive because of
> the init/unload functions that the lock has to be held across).

Ok, I did that. The patch is attached. [In the course of adding that I also
found another bug in my code that prevented GStaticRecMutex from working when
the thread system hasn't been initialized (yet). That is fixed in CVS]. There
is also another problem fixed (in the patch, that is). You said in reply to
Havoc:

> > One note: even if we add the GError, only g_module_open() sensibly
> > needs it; g_module_symbol() can only really fail because the symbol
> > doesn't exist, so returning NULL is fine, and g_module_close() is not
> > very interesting to check errors on.
> 
> beeep. wrong answer, you got as many washing machines as you can carry :)
> 
> for one, g_module_symbol() already returns success with a boolean
> value, for another, NULL can be returned for a successfully looked
> up symbol so wouldn't be a good error indicator (if that'd be the
> case, it'd already return the value and NULL on error).

That sounds good in theory, but if 'symbol' is NULL, then g_module_symbol will
also always return FALSE (for gmodule-dl.c) ;-) Just look at the source. That
is also fixed in the attached patch.

Bye,
Sebastian.
-- 
Sebastian Wilhelmi
mailto:wilhelmi ira uka de
http://goethe.ira.uka.de/~wilhelmi

Index: gmodule/gmodule-dl.c
===================================================================
RCS file: /cvs/gnome/glib/gmodule/gmodule-dl.c,v
retrieving revision 1.9
diff -u -b -B -r1.9 gmodule-dl.c
--- gmodule/gmodule-dl.c	2000/07/26 11:02:01	1.9
+++ gmodule/gmodule-dl.c	2000/11/13 12:50:33
@@ -71,13 +71,16 @@
 
 /* --- functions --- */
 static gchar*
-fetch_dlerror (void)
+fetch_dlerror (gboolean accept_null)
 {
   gchar *msg = dlerror ();
 
   /* make sure we always return an error message != NULL */
 
-  return msg ? msg : "unknown dl-error";
+  if (!accept_null && !msg)
+    msg = "unknown dl-error";
+
+  return msg;
 }
 
 static gpointer
@@ -88,7 +91,7 @@
   
   handle = dlopen (file_name, RTLD_GLOBAL | (bind_lazy ? RTLD_LAZY : RTLD_NOW));
   if (!handle)
-    g_module_set_error (fetch_dlerror ());
+    g_module_set_error (fetch_dlerror (TRUE));
   
   return handle;
 }
@@ -104,7 +107,7 @@
   
   handle = dlopen (NULL, RTLD_GLOBAL | RTLD_LAZY);
   if (!handle)
-    g_module_set_error (fetch_dlerror ());
+    g_module_set_error (fetch_dlerror (TRUE));
   
   return handle;
 }
@@ -121,7 +124,7 @@
   if (is_unref)
     {
       if (dlclose (handle) != 0)
-	g_module_set_error (fetch_dlerror ());
+	g_module_set_error (fetch_dlerror (TRUE));
     }
 }
 
@@ -133,7 +136,7 @@
   
   p = dlsym (handle, symbol_name);
   if (!p)
-    g_module_set_error (fetch_dlerror ());
+    g_module_set_error (fetch_dlerror (FALSE));
   
   return p;
 }
Index: gmodule/gmodule.c
===================================================================
RCS file: /cvs/gnome/glib/gmodule/gmodule.c,v
retrieving revision 1.28
diff -u -b -B -r1.28 gmodule.c
--- gmodule/gmodule.c	2000/07/26 11:02:01	1.28
+++ gmodule/gmodule.c	2000/11/13 12:50:33
@@ -71,7 +71,6 @@
 
 
 /* --- variables --- */
-G_LOCK_DEFINE_STATIC (GModule);
 const char           *g_log_domain_gmodule = "GModule";
 static GModule	     *modules = NULL;
 static GModule	     *main_module = NULL;
@@ -85,7 +84,6 @@
   GModule *module;
   GModule *retval = NULL;
   
-  G_LOCK (GModule);
   if (main_module && main_module->handle == handle)
     retval = main_module;
   else
@@ -95,7 +93,6 @@
 	  retval = module;
 	  break;
 	}
-  G_UNLOCK (GModule);
 
   return retval;
 }
@@ -106,14 +103,12 @@
   GModule *module;
   GModule *retval = NULL;
   
-  G_LOCK (GModule);
   for (module = modules; module; module = module->next)
     if (strcmp (name, module->file_name) == 0)
 	{
 	  retval = module;
 	  break;
 	}
-  G_UNLOCK (GModule);
 
   return retval;
 }
@@ -177,6 +172,8 @@
   return TRUE;
 }
 
+static GStaticRecMutex g_module_global_lock = G_STATIC_REC_MUTEX_INIT;
+
 GModule*
 g_module_open (const gchar    *file_name,
 	       GModuleFlags    flags)
@@ -186,9 +183,9 @@
   
   SUPPORT_OR_RETURN (NULL);
   
+  g_static_rec_mutex_lock (&g_module_global_lock);
   if (!file_name)
     {      
-      G_LOCK (GModule);
       if (!main_module)
 	{
 	  handle = _g_module_self ();
@@ -203,8 +200,8 @@
 	      main_module->next = NULL;
 	    }
 	}
-      G_UNLOCK (GModule);
 
+      g_static_rec_mutex_unlock (&g_module_global_lock);
       return main_module;
     }
   
@@ -214,6 +211,7 @@
     {
       module->ref_count++;
       
+      g_static_rec_mutex_unlock (&g_module_global_lock);
       return module;
     }
   
@@ -233,6 +231,7 @@
 	  module->ref_count++;
 	  g_module_set_error (NULL);
 	  
+	  g_static_rec_mutex_unlock (&g_module_global_lock);
 	  return module;
 	}
       
@@ -245,10 +244,8 @@
       module->ref_count = 1;
       module->is_resident = FALSE;
       module->unload = NULL;
-      G_LOCK (GModule);
       module->next = modules;
       modules = module;
-      G_UNLOCK (GModule);
       
       /* check initialization */
       if (g_module_symbol (module, "g_module_check_init", (gpointer) &check_init))
@@ -274,6 +271,7 @@
       g_free (saved_error);
     }
   
+  g_static_rec_mutex_unlock (&g_module_global_lock);
   return module;
 }
 
@@ -285,6 +283,8 @@
   g_return_val_if_fail (module != NULL, FALSE);
   g_return_val_if_fail (module->ref_count > 0, FALSE);
   
+  g_static_rec_mutex_lock (&g_module_global_lock);
+
   module->ref_count--;
   
   if (!module->ref_count && !module->is_resident && module->unload)
@@ -303,7 +303,6 @@
       
       last = NULL;
       
-      G_LOCK (GModule);
       node = modules;
       while (node)
 	{
@@ -319,7 +318,6 @@
 	  node = last->next;
 	}
       module->next = NULL;
-      G_UNLOCK (GModule);
       
       _g_module_close (module->handle, FALSE);
       g_free (module->file_name);
@@ -327,6 +325,7 @@
       g_free (module);
     }
   
+  g_static_rec_mutex_unlock (&g_module_global_lock);
   return g_module_error() == NULL;
 }
 
@@ -359,6 +358,8 @@
   g_return_val_if_fail (symbol_name != NULL, FALSE);
   g_return_val_if_fail (symbol != NULL, FALSE);
   
+  g_static_rec_mutex_lock (&g_module_global_lock);
+
 #ifdef	G_MODULE_NEED_USCORE
   {
     gchar *name;
@@ -380,11 +381,10 @@
       g_module_set_error (error);
       g_free (error);
       *symbol = NULL;
-
-      return FALSE;
     }
   
-  return TRUE;
+  g_static_rec_mutex_unlock (&g_module_global_lock);
+  return !module_error;
 }
 
 gchar*