Re: revised patch for glib compilation

[Tim Janik <timj gtk org>]

On Sat, 24 Feb 2001, Gary V. Vaughan wrote:

> Hi Tim,
> 
> Please excuse the mangled Reference:s, I lost my mail archive along with most 
> of the contents of my hard drive a few weeks ago, so I replied to the oldest 
> message I have left in the thread...
> 
> Anyway, I redid my implementation of gmodule as a wrapper for libltdl to take 
> advantage of the thread locking hooks just committed to libtool CVS, so that 
> we can hopefully unify the two code bases.  The last few months have occupied 
> me with pushing features gmodule had down into libltdl to make this possible, 
> so the wrapper is actually very thin.
> 
> The purpose of this mail is mainly to get some feedback -- I haven't actually 
> tested the code at all.  In fact, I have never programmed with threads in C 
> before, so some pointers to a small threaded gmodule using project would be a 
> huge help!
> 
> Let me know what you think and if you approve I will incorporate any 
> suggestions you have, clean the whole thing up and resubmit.

ok, comments on the patch are below. i think we can really go that route,
however we need to clear up some locking and module naming issues in your
code, and definitely need to retain reference counts on the GModule side.

> 
> Cheers,
> 	Gary.

> http://bugzilla.gnome.org/show_bug.cgi?id=51583
> 
> *** shadow/51583        Sat Mar  3 12:12:03 2001
> --- shadow/51583.tmp.31415      Sat Mar 10 12:31:31 2001
> ***************
> *** 640,642 ****
> --- 640,656 ----
> 
>   That's my abstract thoughts on the issue - I have not looked at the
>   patch or libltdl yet.
> +
> + ------- Additional Comments From gvv techie com  2001-03-10 12:31 -------
> + To address your concerns with another compile time dependency:
> + libltdl ships with libtool and the libltdl code is copied into your
> + project source tree at libtoolize time (much like gettextize adds the
> + internationalisation source code).
> +
> + Deprecation gmodule is a smashing idea, though I'd like to see a
> + source compatibility wrapper modelled after this patch for the next
> + stable release or two for the sake of projects that already use the
> + gmodule API.
> +
> + I am happy to do the work on integration for this patch if I get an
> + approval in principle.

we will definitely _not_ deprecate GModule as a whole. just changing
the backend is not a valid reason to throw away the GModule API, and
it'd also be an additional burden on people to demand they completely
switch to libtool which doesn't work with GLib threads out of the box.


> #include	"gmodule.h"
> #include	"gmoduleconf.h"
> #include	"ltdl.h"
> #include	<errno.h>
> #include	<string.h>
> 
> 
> #define G_CALLERID_UNDEFINED	((lt_dlcaller_id) -1)
> #define G_MODULE_INI_STRING	"g_module_check_init"
> #define G_MODULE_FINI_STRING	"g_module_unload"

s/INI/INIT/ would improve readability ;)

> /* --- prototypes --- */
> static	inline gboolean	g_module_init_once	(void);
> static	inline void	g_module_set_error	(const gchar *error);
> static	const gchar    *g_module_get_error	(void);
> static	void		g_module_mutex_lock	(void);
> static	void		g_module_mutex_unlock	(void);
> 
> /* --- variables --- */
> G_LOCK_DEFINE_STATIC (GModule);

this is a recursive mutex in our current implementation, because modules
might want to load other modules from their g_module_check_init() functions.

> const char           *g_log_domain_gmodule = "GModule";

this should be taken care of by -DG_LOG_DOMAIN=\"GModule\" now.

> static GStaticPrivate module_error_private = G_STATIC_PRIVATE_INIT;
> 
> 
> /* --- inline functions --- */
> static inline void
> g_module_set_error (const gchar *error)
> {
>   g_static_private_set (&module_error_private, error, NULL);
>   errno = 0;
> }
> 
> /* For orthogonality only:  g_module_error already does exactly what
>    is required, but is named according to the supported API. */
> #define g_module_get_error	g_module_error

/* please use this style for 
 * multiline comments
 */

> static inline gboolean
> g_module_init_once (void)
> {
>   static gint initialized = 0;
> 
>   /* Clear error message for this thread.  */
>   g_module_set_error (NULL);
> 
>   G_LOCK (GModule);
>   if (++initialized == 1)
>     {
>       gint errors = 0;		/* Number of errors from libltdl calls.  */

is it really required to increment initialized everytime this function is
called? ;)
i think:
gboolean initialized = FALSE;

if (!initialized)
  {
    initialized = TRUE;
is good enough.

>       /* Make sure libltdl is using our memory managment API. */
>       lt_dlmalloc = (lt_ptr(*)(size_t)) g_malloc;
>       lt_dlfree	  = (void  (*)(lt_ptr)) g_free;

is this required? it's safe to use malloc/free besides the glib API,
as long as things aren't mixed ala malloc/g_free or g_malloc/free.

>       /* Register the callbacks to make all lt_*() functions MT safe.  */
>       lt_dlmutex_register (g_module_mutex_lock, g_module_mutex_unlock,
> 			   g_module_set_error,  g_module_get_error);
> 
>       /* Initialise ltdl library.  */
>       errors = lt_dlinit ();
>       if (!errors)
> 	{
> 	  gchar *path = g_getenv ("GMODULE_PATH");
> 
> 	  /* Create a gmodule specific key for subsequent access to
> 	     per-module data stored by gmodule functions.  */
> 	  g_module_caller_id = lt_dlcaller_register ();
> 
> 	  /* Initialise libltdl module search directory list.  */
> 	  if (path != NULL)
> 	    errors = lt_dladdsearchdir (path);

can this be reset later? some module loadig APIs only want a specific search
path for their modules. i think in this point, our API still has issues, as
for gtk modules, another search path is required as e.g. for loading application
specific plugins. using a common search path might not always be desirable.

> 	}
>       
>       G_UNLOCK (GModule);
>       return (errors == 0);
>     }
> 
>   G_UNLOCK (GModule);
>   return TRUE;
> }
> 
> /* --- static functions --- */
> static void
> g_module_mutex_lock (void)
> {
>   G_LOCK (GModule);
> }
> 
> static void
> g_module_mutex_unlock (void)
> {
>   G_UNLOCK (GModule);
> }
> 
> 
> /* --- exported functions --- */
> 
> /**
>  * g_module_supported:
>  *
>  * This function is deprecated.
>  * Check if modules are supported on the current platform.  Now that
>  * the gmodule API is a wrapper for libltdl, this function will always
>  * return %TRUE.
>  *
>  * Return value: %TRUE if modules are supported.
>  **/
> gboolean
> g_module_supported (void)
> {
> #if G_ENABLE_DEBUG
>   static gboolean first_call = TRUE;
> 
>   if (first_call)
>     {
>       g_warning ("g_module_supported is deprecated.");
>       g_warning ("modules are always supported by the underlying libltdl.");
>       first_call = FALSE;
>     }
> #endif /* G_ENABLE_DEBUG */
>   
>   return TRUE;
> }

multiline warnings shouldn't be spit out as two seperate warnings, but as:
g_warning ("g_module_supported is deprecated. "
           "modules are always supported by the underlying libltdl.");
(people also shouldn't use embedded newlines in warnings).

but actually i don't think a warning is really apropriate here, we should
either leave that function in place, always returning TRUE if libtool
supports modules on _every_ platform, or completely get rid of it and
cause a compilation warning.

> /**
>  * g_module_open:
>  * @filename: the name of the file containing the module to be opened.
>  * @flags: not used.
>  *
>  * Opens a module.  If the module has already been opened, its reference
>  * count is incremented.
>  *
>  * First of all g_module_open() tries to open @file_name as a module.  If
>  * that fails and @file_name has the ".la"-suffix (and is a libtool archive)
>  * it tries to open the corresponding module.  If that fails and @file_name
>  * doesn't have the proper module suffix for the host platform, this suffix
>  * will be appended and any corresponding module opened.  If that fails and
>  * @file_name doesn't have the ".la"-suffix, it is appended and
>  * g_module_open() tries to open the corresponding module.  If ultimately
>  * that fails as well, %NULL is returned.
>  *
>  * Return value: a #GModule on success, or %NULL on failure.
>  **/
> GModule*
> g_module_open (const gchar    *file_name,
> 	       GModuleFlags    flags)
> {
>   lt_dlhandle  handle = (lt_dlhandle) 0;
>   
>   g_module_init_once ();
> 
>   /* open the module */
>   handle = lt_dlopenext (file_name);
>   
>   if (handle)
>     {
>       gchar *error = NULL;
>       GModuleCheckInit check_init;
>       GModuleUnload unload;
>       const gchar *check_failed_error = NULL;
>       
>       /* check initialization */
>       if (g_module_symbol ((GModule *) handle, G_MODULE_INI_STRING,
> 			   (gpointer) &check_init))
> 	{
> 	  check_failed_error = check_init ((GModule *) handle);

you do the loading of the module and it's init call not within 
a common lock being held, this is not going to work if two
threads concurrently try to load the same module, as _init
could be called twice here.
also you don't do any reference counting in GModule itself
anymore which is a problem wrg track-keeping of the init function.

a module written solely to be loaded through GModule code is guaranteed
the following semantics:

1) module is loaded
2) g_module_check_init() is called, if it returns a static non-NULL
   string, module loading is considered to have failed, the module
   gets unloaded again
3) arbitrary module code can be called
4) g_module_unload() is called, a module may still make itself resident
   at this time with g_module_make_resident()
5) the module is unloaded

important constraints are:
- before any module coded is used, g_module_check_init() is called
- g_module_check_init() is called only once
- after any module code was executed, g_module_unload() is called
  so the module can do proper cleanup

the way i read you code, if the main program issues:

g_module_open ("foo", 0);
g_module_open ("foo", 0);

_init will be called twice, while the latter should just increment
a GModule ref_count (and in fact, a couple older plugins do
g_module_open (g_module_name (self)); in their _init functions
to make themselves resident).

(another implication just sprang to my mind, GModule doesn't currently
handle static C++ constructors, but if it did, or will do due to
libtool supporting them, they should be called only after (2) and
before (3) which maybe a bit hard, depending on how libtool implements
that (there'd basically have to be a lt_construct() function to call
after g_module_check_init() returned NULL).

> 	}
>       
>       if (check_failed_error != NULL)
> 	{
> 	  error = g_strconcat ("GModule initialization check failed: ",
> 			       check_failed_error, NULL);
> 	}
> 
>       /* Register error diagnostic generated above, if any. */
>       if (error)
> 	{
> 	  g_module_close ((GModule *) handle);
> 	  module = NULL;
> 	  g_module_set_error (error);
> 	  g_free (error);
> 	}
>     }
>   
>   return module;
> }
> 
> /**
>  * g_module_close:
>  * @module: the module to be closed.
>  *
>  * Closes an open module.
>  *
>  * Return value: %TRUE on success.
>  **/
> gboolean
> g_module_close (GModule	*module)
> {
>   lt_dlinfo *info;
>   gboolean success = TRUE;
> 
>   g_return_val_if_fail (module != NULL, FALSE);
> 
>   g_module_init_once ();	/* Is this really neccessary? */

no, you can't have a valid module handle without g_module_open(),
which did initialization already.

>   info = lt_dlgetinfo ((lt_dlhandle) module);
> 
>   g_return_val_if_fail (info->ref_count > 1, FALSE);
>   
>   if ((info->ref_count == 1) && !lt_dlisresident ((lt_dlhandle) module))

eh? i never see you doing a decrement.

>     {
>       GModuleUnload unload;
>       lt_ptr stale = NULL;
> 
>       unload = g_module_symbol (module, G_MODULE_FINI_STRING,
> 				(gpointer) &unload);
> 
>       if (unload)
> 	unload (module);
>       
>       if (lt_dlclose ((lt_dlhandle) module) != 0)
> 	success = FALSE;
> 
>       g_free (module);
>     }
>   
>   return success;
> }

> /**
>  * g_module_make_resident:
>  * @module: the module to make permenantly resident.
>  *
>  * Ensures that a module can never be unloaded.  Any future g_module_close()
>  * calls on the module will have no effect.  If you open the entire process
>  * as a reflexive module, then it is always made resident by default -- there
>  * is no need to call this function manually.
>  **/
> void
> g_module_make_resident (GModule *module)
> {
>   g_return_if_fail (module != NULL);
> 
>   lt_dlmakeresident ((lt_dlhandle) module);
> }

> /**
>  * g_module_error:
>  *
>  * Get a string describing the last error that occured in the gmodule
>  * subsystem.
>  *
>  * Return value: a string describing the last module error.
>  **/
> gchar*
> g_module_error (void)
> {
>   return g_static_private_get (&module_error_private);
> }

> /**
>  * g_module_symbol:
>  * @module: the module.
>  * @symbol_name: the name of the symbol to find.
>  * @symbol: returns the pointer to the symbol value.
>  *
>  * Gets a symbol pointer from a module.
>  *
>  * Return value: %TRUE on success.
>  **/
> gboolean
> g_module_symbol (GModule	*module,
> 		 const gchar	*symbol_name,
> 		 gpointer	*symbol)
> {
>   if (symbol)
>     *symbol = NULL;
> 
>   g_return_val_if_fail (module      != NULL, FALSE);
>   g_return_val_if_fail (symbol_name != NULL, FALSE);
>   g_return_val_if_fail (symbol      != NULL, FALSE);
>   
>   *symbol = lt_dlsym ((lt_dlhandle) module, symbol_name);
>   
>   if (*symbol == NULL)
>     {
>       gchar *module_error = lt_dlerror ();
>       gchar *error;
> 
>       error = g_strconcat ("`", symbol_name, "': ", module_error, NULL);
>       g_module_set_error (error);
>       g_free (error);
> 
>       return FALSE;
>     }
>   
>   return TRUE;
> }
> 
> /**
>  * g_module_name:
>  * @module: the module.
>  *
>  * Gets the file name from a #GModule.
>  *
>  * Return value: the file name of the module, or "main" if the module
>  * is the main program itself.
>  **/
> gchar*
> g_module_name (GModule *module)
> {
>   lt_dlinfo  *info;
>   
>   g_return_val_if_fail (module != NULL, NULL);
>   
>   info   = lt_dlgetinfo ((lt_dlhandle) module);
> 
>   return g_strdup (info->filename ? info->filename : "main");
> }

this did not return a newly allocated string and shouldn't
in your version either.

> /**
>  * g_module_set_search_path:
>  * @path: a colon delimited list of directories.
>  *
>  * Set the paths of the directories to be searced when trying to
>  * locate a module opened using a relative path.
>  *
>  * Return value: %TRUE if successful.
>  **/
> gboolean
> g_module_set_search_path (gchar *path)
> {
>   return (lt_dlsetsearchpath (path) == 0);
> }
> 
> /**
>  * g_module_add_search_path:
>  * @path: a colon delimited list of directories.
>  *
>  * Append to the list of user paths, the additional @path of directories
>  * to be searched when trying to locate a module opened using a
>  * relative path.
>  *
>  * Return value: %TRUE if successful.
>  **/
> gboolean
> g_module_add_search_path (gchar *path)
> {
>   return (lt_dladdsearchdir (path) == 0);
> }
> 
> /**
>  * g_module_build_path:
>  * @directory: the directory where the module is.
>  * @module_name: the name of the module.
>  *
>  * This function is deprecated.
>  **/
> gchar*
> g_module_build_path (const gchar *directory,
> 		     const gchar *module_name)
> {
> #if G_ENABLE_DEBUG
>   static gboolean first_call = TRUE;
> 
>   if (first_call)
>     {
>       g_warning ("g_module_build_path is deprecated.");
>       g_warning ("Use g_module_set_search_path or g_module_add_search_path.");

how's libtool going to handle "libfoo.la" vs. "libfoo.so" vs. "libfoo.dll" vs.
"libfoo" vs. "foo" arguments being passed into open()?

>       first_call = FALSE;
>     }
> #endif /* G_ENABLE_DEBUG */
> 
> #if (G_MODULE_IMPL == G_MODULE_IMPL_DL)
> 
>   if (directory && *directory) {
>     if (strncmp (module_name, "lib", 3) == 0)
>       return g_strconcat (directory, "/", module_name, NULL);
>     else
>       return g_strconcat (directory, "/lib", module_name, ".so", NULL);
>   } else if (strncmp (module_name, "lib", 3) == 0)
>     return g_strdup (module_name);
>   else
>     return g_strconcat ("lib", module_name, ".so", NULL);
> 
> #elif (G_MODULE_IMPL == G_MODULE_IMPL_DLD)
> 
>   if (directory && *directory)
>     if (strncmp (module_name, "lib", 3) == 0)
>       return g_strconcat (directory, "/", module_name, NULL);
>     else
>       return g_strconcat (directory, "/lib", module_name, ".sl", NULL);
>   else if (strncmp (module_name, "lib", 3) == 0)
>     return g_strdup (module_name);
>   else
>     return g_strconcat ("lib", module_name, ".sl", NULL);
> 
> #elif (G_MODULE_IMPL == G_MODULE_IMPL_BEOS)
> 
>   g_warning ("g_module_build_path() untested for BeOS!");
>   
>   if (directory && *directory)
>     {
>       if (strncmp (module_name, "lib", 3) == 0)
>         return g_strconcat (directory, "/", module_name, NULL);
>       else
>         return g_strconcat (directory, "/lib", module_name, ".so", NULL);
>     }
>   else if (strncmp (module_name, "lib", 3) == 0)
>     return g_strdup (module_name);
>   else
>     return g_strconcat ("lib", module_name, ".so", NULL);
> 
> #elif (G_MODULE_IMPL == G_MODULE_IMPL_WIN32)
> 
>   {
>     gint k = strlen (module_name);
>     if (directory && *directory)
>       if (k > 4 && g_strcasecmp (module_name + k - 4, ".dll") == 0)
> 	return g_strconcat (directory, "\\", module_name, NULL);
>       else
> 	return g_strconcat (directory, "\\", module_name, ".dll", NULL);
>     else if (k > 4 && g_strcasecmp (module_name + k - 4, ".dll") == 0)
>       return g_strdup (module_name);
>     else
>       return g_strconcat (module_name, ".dll", NULL);
>   }
>   
> #elif (G_MODULE_IMPL == G_MODULE_IMPL_OS2)
> 
>   {
>     gchar *suffix = strrchr(module_name, '.');
>     if (directory && *directory)
>       if (suffix && (stricmp (suffix, ".dll") == 0))
> 	return g_strconcat (directory, "/", module_name, NULL);
>       else
> 	return g_strconcat (directory, "/", module_name, ".dll", NULL);
>     else if (suffix && (stricmp (suffix, ".dll") == 0))
>       return g_strdup (module_name);
>     else
>       return g_strconcat (module_name, ".dll", NULL);
>   }
>   
> #endif
> 
>   return NULL;
> }

---
ciaoTJ