Re: [patch 01/19] 277-gary-rename-remaining-troublesome-ltdl-apis.diff Queue

[Ralf Wildenhues]

Hi Gary,

* Gary V. Vaughan wrote on Mon, Oct 10, 2005 at 12:26:25PM CEST:
>  ChangeLog        |    0 
>  NEWS             |    9 +-
>  doc/libtool.texi |  176 
> +++++++++++++++++++++----------------------------------
>  libltdl/ltdl.c   |   88 ++++++++++-----------------
>  libltdl/ltdl.h   |   10 +--
>  4 files changed, 114 insertions(+), 169 deletions(-)
> 
> Index: libtool--devo--1.0/ChangeLog
> from  Gary V. Vaughan  <address@hidden>
>       * libltdl/ltdl.h, libltdl/ltdl.c (lt_dlhandle_first): Removed.
>       * libltdl/ltdl.h, libltdl/ltdl.c (lt_dlhandle_next)
>       (lt_dlhandle_find, lt_dlforeach): Removed...
>       (lt_dlhandle_iterate, lt_dlhandle_fetch, lt_dlhandle_map): Similar
>       functions that are multi-loader safe, and require a registered
>       interface validator argument.
>       * doc/libtool.texi: Updated.
>       * NEWS: Updated.

OK to apply, with the nits below addressed.
Please also apply the then-freed part of your patch queue,
that should 284, 285, 286 I believe; with 286, please just
take one look at the additional tiny nit I found, see other mail.

Thank you!

Cheers,
Ralf

> Index: libtool--devo--1.0/NEWS
> ===================================================================
> --- libtool--devo--1.0.orig/NEWS
> +++ libtool--devo--1.0/NEWS
> @@ -4,11 +4,12 @@ New in 1.9h: 2005-??-??; CVS version 2.1
>  * New tests for support of Automake subdir-objects.
>  * Fix libltdl on static platforms.
>  * New LT_CONFIG_LTDL_DIR macro.
> -* New lt_dlinterface_register, lt_dlinterface_set_data and
> -  lt_dlinterface_get_data libltdl API calls to maintain separation of
> -  concerns between modules loaded by different libraries.
> +* New multi-module-loader safe libltdl handle iteration APIs:
> +  lt_dlhandle_iterate, lt_dlhandle_fetch, lt_dlhandle_map.
> +* New lt_dlinterface_register to maintain separation of concerns between
> +  modules loaded by different libraries.
>  * Removed deprecated APIs from libltdl: lt_dlcaller_register,
> -  lt_dlcaller_get_data, lt_dlcaller_set_data, lt_dlmutex_register,
> +  lt_dlhandle_next, lt_dlhandle_find, lt_dlforeach, lt_dlmutex_register,
>    lt_dlmutex_lock, lt_dlmutex_unlock, lt_dlmutex_seterror,
>    lt_dlmutex_geterror, lt_dlmalloc, lt_dlrealloc, lt_dlfree.
>  * Support for Portland Group compiler on GNU/Linux.
> Index: libtool--devo--1.0/doc/libtool.texi
> ===================================================================
> --- libtool--devo--1.0.orig/doc/libtool.texi
> +++ libtool--devo--1.0/doc/libtool.texi
> @@ -3734,35 +3734,85 @@ Furthermore, in order to save you from h
>  handles of all the modules you have loaded, these functions allow you to
>  iterate over libltdl's list of loaded modules:
>  
> address@hidden int lt_dlforeach (@w{int (address@hidden) (lt_dlhandle 
> @var{handle}, void * @var{data})}, @w{void * @var{data}})
> -For each loaded module call the function @var{func}.  The argument
> address@hidden is the handle of one of the loaded modules, @var{data} is
> -the @var{data} argument passed to @code{lt_dlforeach}.
> -As soon as @var{func} returns a non-zero value for one of the handles,
> address@hidden will stop calling @var{func} and immediately return 1.
> -Otherwise 0 is returned.
> address@hidden {Type} lt_dlinterface_id
> +The opaque type used to hold the module interface details for each
> +registered libltdl client.
> address@hidden deftp
> +
> address@hidden {Type} int lt_dlhandle_interface (@w{lt_dlhandle 
> @var{handle},} @w{const char address@hidden)

Hmm, this @deftp definition is rendered a bit weird in the DVI, but I
suppose that is either deliberate (by texinfo) or a limitation.  Dunno
what would be better here.

> +Functions of this type are called to check that a handle conforms to a
> +library's expected module interface when iterating over the global
> +handle list.  You should be careful to write a callback function of
> +this type that can correctly identify modules that belong to this
> +client, both to prevent other clients from accidentally finding your
> +loaded modules with the iterator functions below, and vice versa.  The
> +best way of doing this is to check that module @var{handle} conforms

better: `to do this'?  (just a thought)

> +to the interface specification of your loader using @code{lt_dlsym}.

This is pretty inefficient: the user will actually have to store a list
of loaded modules itself.  Hrmpf.

Ah, no, never mind.  Your example below clarifies it.
(Maybe above is a bit difficult to understand?)

> +
> +The callback will be given @strong{every} module loaded by all the
> +libltdl module clients in the current address space, including any

Can we weaken this to: `The callback may be given every module..'?
Rationale: we might change to use a more efficient implementation later
(or have to use it for unrelated reasons), which will allow ltdl itself
to weed out the other modules at less cost; no need to restrict
ourselves not to be able to pass on that improvement.

Just an idea; decide for yourself.

> +modules loaded by other libraries such as libltdl itself, and should
> +return non-zero if that module does not fulfill the interface
> +requirements of your loader.
> +
> address@hidden
> +int
> +my_interface_cb (lt_dlhandle handle, const char *id_string)
> address@hidden
> +  char *(*module_id) (void) = NULL;
> +
> +  /* @r{A valid my_module must provide all of these symbols.}  */
> +  if (!((module_id = (char*(*)(void)) lt_dlsym ("module_version"))
> +        && lt_dlsym ("my_module_entrypoint")))
> +      return 1;
> +
> +  if (strcmp (id_string, module_id()) != 0)
> +      return 1;
> +
> +  return 0;
> address@hidden
> address@hidden example
> address@hidden deftp
> +
> address@hidden lt_dlinterface_id lt_dlinterface_register (@w{const char 
> address@hidden,} @w{lt_dlhandle_interface address@hidden)

  s/},}/}},/

> +Use this function to register your interface validator with libltdl,
> +and in return obtain a unique key to store and retrieve per module data.

Maybe s/per module/per-module/ for easier understanding?

> +You supply an @var{id_string} and @var{iface} so that the resulting
> address@hidden can be used to filter the module handles
> +returned by the iteration functions below.
> address@hidden deftypefun
> +
> address@hidden int lt_dlhandle_map (@w{lt_dlinterface_id @var{iface}}, @w{int 
> (address@hidden) (lt_dlhandle @var{handle}, void * @var{data})}, @w{void * 
> @var{data}})
> +For each module that matches @var{iface}, call the function
> address@hidden  When writing the @var{func} callback function, the
> +argument @var{handle} is the handle of a loaded modules, and
> address@hidden is the @var{data} argument passed to

Easier to understand if the last address@hidden' is replaced by `last',
IMVHO.

> address@hidden As soon as @var{func} returns a non-zero value
> +for one of the handles, @code{lt_dlhandles_map} will stop calling

  s/lt_dlhandles_map/lt_dlhandle_map/

> address@hidden and immediately return 1.  Otherwise 0 is returned.
>  @end deftypefun

Wouldn't it be even more useful if it just returned the error code in
that case?  The user could carry some information that way, without the
hassle of protecting data in a concurrent execution setup, for example.

Incidentally, that would also make the implementation of lt_dlhandle_map
even simpler.

> address@hidden lt_dlhandle lt_dlhandle_next (@w{lt_dlhandle place})
> -Iterate over the loaded module handles, returning the first handle in the
> -list if @var{place} is @code{NULL}, and the next one on subsequent calls.
> -If @var{place} is the last element in the list of loaded modules, this
> -function returns @code{NULL}.
> address@hidden lt_dlhandle lt_dlhandle_iterate (@w{lt_dlinterface_id 
> @var{iface}}, @w{lt_dlhandle place})

  s/place/@var{&}/

> +Iterate over the module handles loaded by @var{iface}, returning the
> +first matching handle in the list if @var{place} is @code{NULL}, and
> +the next one on subsequent calls.  If @var{place} is the last element
> +in the list of eligible modules, this function returns @code{NULL}.
>  
>  @example
>  lt_dlhandle handle = 0;
> +lt_dlinterface_id iface = my_interface_id;
>  
> -while ((handle = lt_dlhandle_next (handle)))
> +while ((handle = lt_dlhandle_iterate (iface, handle)))
>    @{
>      @dots{}
>    @}
>  @end example
>  @end deftypefun
>  
> address@hidden lt_dlhandle lt_dlhandle_find (@w{const char address@hidden)
> -Search through the loaded module handles for a module named
> address@hidden lt_dlhandle lt_dlhandle_fetch (@w{lt_dlinterface_id 
> @var{iface}}, @w{const char address@hidden)
> +Search through the module handles loaded by @var{iface} for a module named
>  @var{module_name}, returning its handle if found or else @code{NULL}
> -if no such named module has been loaded.
> +if no such named module has been loaded by @var{iface}.
>  @end deftypefun
>  
>  However, you might still need to maintain your own list of loaded
> @@ -3770,26 +3820,10 @@ module handles (in parallel with the lis
>  if there were any other data that your application wanted to associate
>  with each open module.  Instead, you can use the following @sc{api}
>  calls to do that for you.  You must first obtain a unique interface id
> -from libltdl, and subsequently always use it to retrieve the data you
> -stored earlier.  This allows different libraries to each store their
> -own data against loaded modules, without interfering with one another.
> -
> address@hidden {Type} lt_dlinterface_id
> -The opaque type used to hold individual data set keys.
> address@hidden deftp
> -
> address@hidden {Type} int lt_dlhandle_interface (@w{lt_dlhandle 
> @var{handle},} @w{const char address@hidden)
> -Functions of this type are called to check that a handle conforms to a
> -library's expected module interface when iterating over the global
> -handle list.
> address@hidden deftp
> -
> address@hidden lt_dlinterface_id lt_dlinterface_register (@w{const char 
> address@hidden,} @w{lt_dlhandle_interface address@hidden)
> -Use this to obtain a unique key to store and retrieve per module data,
> -if you supply an @var{id_string} and @var{iface}, then the resulting
> address@hidden can be used to filter the module handles
> -returned by @samp{lt_dlhandle_next}.
> address@hidden deftypefun
> +from libltdl as described above, and subsequently always use it to
> +retrieve the data you stored earlier.  This allows different libraries
> +to each store their own data against loaded modules, without
> +interfering with one another.
>  
>  @deftypefun {void *} lt_dlcaller_set_data (@w{lt_dlinterface_id @var{key}}, 
> @w{lt_dlhandle @var{handle}}, @w{void * @var{data}})
>  Set @var{data} as the set of data uniquely associated with @var{key} and
> @@ -3824,76 +3858,6 @@ Return the address of the data associate
>  @var{handle}, or else @code{NULL} if there is none.
>  @end deftypefun
>  
> -The preceding functions can be combined with @code{lt_dlforeach} to
> -implement search and apply operations without the need for your
> -application to track the modules that have been loaded and unloaded:
> -
> address@hidden
> -int
> -my_dlinterface_callback (lt_dlhandle handle, void *key)
> address@hidden
> -  struct my_module_data *my_data;
> -
> -  my_data = lt_dlcaller_get_data (handle, (lt_dlinterface_id) key);
> -
> -  return process (my_data);
> address@hidden
> -
> -int
> -my_dlinterface_foreach (lt_dlinterface_id key)
> address@hidden
> -  lt_dlforeach (my_dlinterface_callback, (void *) key);
> address@hidden
> address@hidden example
> -
> address@hidden lt_dlhandle lt_dlhandle_first (@w{lt_dlinterface_id @var{key}})
> -Normally, you can fetch each of the loaded module handles in turn with
> -successive calls to @samp{lt_dlhandle_next} as shown in the example
> -above.  In that example, the loop iterates over every libltdl loaded
> -module in your application, including the modules used by libltdl
> -itself!  This is useful from within a module loader for example.
> -
> address@hidden
> -Often, your application doesn't want to concern itself with modules
> -loaded by the libraries it uses, or for libltdl's internal use.  In
> -order to do that, you need to specify an interface validator callback:
> -
> address@hidden
> -/* @r{Return non-zero if} @var{handle} @r{doesn't conform to my iface.}  */
> -int
> -iface_validator_callback (lt_dlhandle handle, const char *id_string)
> address@hidden
> -    return (lt_sym (handle, "module_entry_point") != 0)
> address@hidden
> address@hidden example
> -
> address@hidden
> -When you register for an interface identification key with
> address@hidden, you log the interface validator.  But
> -this time, when you start the iterator loop over the loaded module
> -handles, if you fetch the first handle with @samp{lt_dlhandle_first},
> -then that and all subsequent calls to @samp{lt_dlhandle_next} will
> -skip any loaded module handles that fail the registered interface
> -validator callback function:
> -
> address@hidden
> -/* @r{Register for an} interface_id @r{to identify ourselves to} libltdl.  */
> -interface_id = lt_dlinterface_register ("example", iface_validator_callback);
> -
> address@hidden
> -/* @r{Iterate over the modules related to my} interface_id.  */
> address@hidden
> -  lt_dlhandle handle;
> -  for (handle = lt_dlhandle_first (interface_id);
> -       handle;
> -       handle = lt_dlhandle_next (handle))
> -    @{
> -      @dots{}
> -    @}
> address@hidden
> address@hidden example
> address@hidden deftypefun
> -
>  Old versions of libltdl also provided a simpler, but similar, @sc{api}
>  based around @code{lt_dlcaller_id}.  Unfortunately, it had no
>  provision for detecting whether a module belonged to a particular
> Index: libtool--devo--1.0/libltdl/ltdl.c
> ===================================================================
> --- libtool--devo--1.0.orig/libltdl/ltdl.c
> +++ libtool--devo--1.0/libltdl/ltdl.c
> @@ -2113,91 +2113,69 @@ lt_dlgetinfo (lt_dlhandle handle)
>  }
>  
>  
> -/* Nasty semantics, necessary for reasonable backwards compatibility:
> -   Either iterate over the whole handle list starting with 
> lt_dlhandle_next(0),
> -   or else iterate over just the handles of modules that satisfy a given
> -   interface by getting the first element using lt_dlhandle_first(iface).  */
> -
> -static lt__interface_id *iterator = 0;
> -
> -lt_dlhandle
> -lt_dlhandle_first (lt_dlinterface_id iface)
> -{
> -  iterator = iface;
> -
> -  return handles;
> -}
> -
> -
>  lt_dlhandle
> -lt_dlhandle_next (lt_dlhandle place)
> +lt_dlhandle_iterate (lt_dlinterface_id iface, lt_dlhandle place)
>  {
>    lt__handle *handle = (lt__handle *) place;
> +  lt__interface_id *iterator = (lt__interface_id *) iface;
> +
> +  assert (iface); /* iface is a required argument */
>  
>    if (!handle)
> -    {
> -      /* old style iteration across all handles */
> -      iterator = 0;
> -      handle = (lt__handle *) handles;
> -    }
> -  else
> -    {
> -      /* otherwise start at the next handle after the passed one */
> -      handle = handle->next;
> -    }
> +    handle = (lt__handle *) handles;
>  
> -  /* advance until the interface check (if we have one) succeeds */
> -  while (handle && iterator && iterator->iface
> +  /* advance while the interface check fails */
> +  while (handle && iterator->iface
>        && ((*iterator->iface) (handle, iterator->id_string) != 0))
>      {
>        handle = handle->next;
>      }
>  
> -  if (!handle)
> -    {
> -      /* clear the iterator after the last handle */
> -      iterator = 0;
> -    }
> -
>    return (lt_dlhandle) handle;
>  }
>  
>  
>  lt_dlhandle
> -lt_dlhandle_find (const char *module_name)
> +lt_dlhandle_fetch (lt_dlinterface_id iface, const char *module_name)
>  {
> -  lt__handle *cur = (lt__handle *) handles;
> +  lt_dlhandle handle = 0;
> +
> +  assert (iface); /* iface is a required argument */
>  
> -  if (cur)
> +  while ((handle = lt_dlhandle_iterate (handle, iface)))
>      {
> -      do
> -     {
> -       if (cur->info.name && streq (cur->info.name, module_name))
> -         break;
> -     }
> -      while ((cur = cur->next));
> +      lt__handle *cur = (lt__handle *) handle;
> +      if (cur && cur->info.name && streq (cur->info.name, module_name))
> +     break;
>      }
>  
> -  return cur;
> +  return handle;
>  }
>  
> +
>  int
> -lt_dlforeach (int (*func) (lt_dlhandle handle, void *data), void *data)
> +lt_dlhandle_map (lt_dlinterface_id iface,
> +              int (*func) (lt_dlhandle handle, void *data), void *data)
>  {
> +  lt__interface_id *iterator = (lt__interface_id *) iface;
> +  lt__handle *cur = (lt__handle *) handles;
> +  lt__handle *tmp = 0;
> +  lt__handle *prev = 0;

Please remove tmp and prev, they are unused.

If you agree with my proposal to return the return value of func, you
can simplify this function by defining `errors' only inside the loop,
and returning immediately after calling `func' in the nonzero case.

>    int errors = 0;
> -  lt__handle *cur;
>  
> -  cur = (lt__handle *) handles;
> -  while (cur)
> -    {
> -      lt__handle *tmp = cur;
> +  assert (iface); /* iface is a required argument */
>  
> -      cur = cur->next;
> -      if ((*func) (tmp, data))
> +  while (cur && !errors)
> +    {
> +      /* advance while the interface check fails */
> +      while (cur && iterator->iface
> +          && ((*iterator->iface) (cur, iterator->id_string) != 0))
>       {
> -       ++errors;
> -       break;
> +       cur = cur->next;
>       }
> +
> +      if ((*func) (cur, data))
> +     ++errors;
>      }
>  
>    return errors;
> Index: libtool--devo--1.0/libltdl/ltdl.h
> ===================================================================
> --- libtool--devo--1.0.orig/libltdl/ltdl.h
> +++ libtool--devo--1.0/libltdl/ltdl.h
> @@ -126,10 +126,12 @@ typedef struct {
>  } lt_dlinfo;
>  
>  LT_SCOPE const lt_dlinfo *lt_dlgetinfo           (lt_dlhandle handle);
> -LT_SCOPE lt_dlhandle lt_dlhandle_first   (lt_dlinterface_id key);
> -LT_SCOPE lt_dlhandle lt_dlhandle_next    (lt_dlhandle place);
> -LT_SCOPE lt_dlhandle lt_dlhandle_find    (const char *module_name);
> -LT_SCOPE int         lt_dlforeach        (
> +
> +LT_SCOPE lt_dlhandle lt_dlhandle_iterate (lt_dlinterface_id iface,
> +                                          lt_dlhandle place);
> +LT_SCOPE lt_dlhandle lt_dlhandle_fetch   (lt_dlinterface_id iface,
> +                                          const char *module_name);
> +LT_SCOPE int         lt_dlhandle_map     (lt_dlinterface_id iface,
>                               int (*func) (lt_dlhandle handle, void *data),
>                               void *data);