Re: [PATCH 370] Implement lt_dlopening of only preloaded modules.

[Ralf Wildenhues]

Hi Gary,

a quick nit review (I haven't looked thoroughly yet):

* Gary V. Vaughan wrote on Tue, May 06, 2008 at 07:24:46PM CEST:
> 
> This makes it possible to build a libltdl client with some preloaded
> modules, and limit lt_dlopenadvise to open only the preloaded modules.

> --- a/NEWS
> +++ b/NEWS
> @@ -2,9 +2,10 @@ NEWS - list of user-visible changes between releases of GNU 
> Libtool
>  
>  New in 2.2.??: 2008-06-??: git version 2.2.5a, Libtool team:
>  
> -* Bug fixes:
> +* New features:
>  
> -  - None yet

No need to remove the Bug fixes section, there will certainly be entries
later, no?  ;-)

> --- a/doc/libtool.texi
> +++ b/doc/libtool.texi
> @@ -3816,6 +3816,13 @@ On failure, @code{lt_dladvise_resident} returns 
> non-zero and sets an error
>  message that can be retrieved with @code{lt_dlerror}.
>  @end deftypefun
>  
> address@hidden int lt_dladvise_preload (lt_dladvise address@hidden)
> +Set the @code{preload} hint on @var{advise}.  Passing an @var{advise}
> +parameter to @code{lt_dlopenadvise} with this hint set causes it to
> +load only preloaded modules, so that if a suitable preleaded module is

s/preleaded/preloaded/

> +not found, @code{lt_dlopenadvise} will return @code{NULL}.
> address@hidden deftypefun
> +

> --- a/libltdl/libltdl/lt__private.h
> +++ b/libltdl/libltdl/lt__private.h
> @@ -126,6 +126,7 @@ struct lt__advise {
>                                  subsequently loaded modules.  */
>    unsigned int       is_symlocal:1;  /* module symbols are only available
>                                  locally. */
> +  unsigned int       is_preload:1;   /* only preloaded modules will be 
> tried.  */

s/is_preload/try_preload_only/  ?


> --- a/libltdl/ltdl.c
> +++ b/libltdl/ltdl.c
> @@ -1257,27 +1257,40 @@ try_dlopen (lt_dlhandle *phandle, const char 
> *filename, const char *ext,
>  
>        if (vtable)
>       {
> +       char *archive_name = MALLOC (char, LT_STRLEN (name) + 3);
>         *phandle = (lt_dlhandle) lt__zalloc (sizeof (struct lt__handle));
>  
> -       if (*phandle == NULL)
> +       if ((*phandle == NULL) || (archive_name == NULL))
>           {
>             ++errors;
>             goto cleanup;
>           }
>         newhandle = *phandle;
>  
> -       if (tryall_dlopen (&newhandle, attempt, advise, vtable) == 0)
> +       /* Preloaded modules are always named according to their old
> +          archive name.  */
> +       sprintf (archive_name, "%s.a", name);
> +
> +       if (tryall_dlopen (&newhandle, archive_name, advise, vtable) == 0)
>           {
>             goto register_handle;

Leaking archive_name here.

>           }
>  
>         /* If we're still here, there was no matching preloaded module,
>            so put things back as we found them, and continue searching.  */
> +       FREE (archive_name);
>         FREE (*phandle);
>         newhandle = NULL;
>       }
>      }
>  


> --- a/libltdl/m4/ltdl.m4
> +++ b/libltdl/m4/ltdl.m4
> @@ -264,7 +264,7 @@ if test "x$with_included_ltdl" != xyes; then
>    # decide whether there is a useful installed version we can use.
>    AC_CHECK_HEADER([ltdl.h],
>        [AC_CHECK_DECL([lt_dlinterface_register],
> -        [AC_CHECK_LIB([ltdl], [lt_dlinterface_register],
> +        [AC_CHECK_LIB([ltdl], [lt_dladvise_preload],
>              [with_included_ltdl=no],
>              [with_included_ltdl=yes])],
>          [with_included_ltdl=yes],

I view this as a separate change.  Not sure whether we should have a
policy to always require the newest available feature or not, in an
installed ltdl.

> --- a/tests/lt_dladvise.at
> +++ b/tests/lt_dladvise.at

AFAICS the test doesn't ensure that, if the platform supports
non-preloaded module, lt_dladvise_preload causes trying to load them to
fail.

Cheers,
Ralf