Re: 330-gary-ltdl-vs-need-lib-prefix-unknown

[Ralf Wildenhues]

Review for the remaining stuff:

[ tryall_dlopen ]
> -    const lt_dlvtable *vtable = 0;
> -    lt_dlloader *loader = 0;
> +    lt_dlloader loader = lt_dlloader_next (0);
> +    const lt_dlvtable *loader_vtable;
> 
> -    while ((loader = (lt_dlloader *) lt_dlloader_next (loader)))
> +    do

This change tells me the internal typing system sucks.  If you at one
time use a lt_dlloader and then at the next day use lt_dlloader* in
a purely internal static function, then there is something wrong.

If we need to use pointers to void for hiding in external headers, then
so be it.  But most of the time I don't see why this:
  struct lt_foo;  /* declaration only, no definition */
  typedef struct lt_foo *lt_foo_handle;
  int lt_foofunc (lt_foo_handle);

in an exported header, should a problem.  (Of course where we know
lt_foo will always be a struct.)

I'm not asking to change this right now, although I am pretty certain it
should help.  What I'm asking is that we refrain from adding more of
this nonsense.

Please add necessary casts to this patch so compilation with C++
compiler is not broken again.

> @@ -1225,16 +1285,14 @@ try_dlopen (lt_dlhandle *phandle, const 
>        of libtool */
>        int    installed = 1;
>  
> -
>        /* Now try to open the .la file.  If there is no directory name
>           component, try to find it first in user_search_path and then other
>           prescribed paths.  Otherwise (or in any case if the module was not
>           yet found) try opening just the module name as passed.  */
>        if (!dir)
>       {
> -       const char *search_path;
> +       const char *search_path = user_search_path;
>  
> -       search_path = user_search_path;
>         if (search_path)
>           file = find_file (user_search_path, base_name, &dir);
>  
> @@ -1277,7 +1335,7 @@ try_dlopen (lt_dlhandle *phandle, const 
>        /* read the .la file */
>        if (parse_dotla_file(file, &dlname, &libdir, &deplibs,
>           &old_name, &installed) != 0)
> -     errors++;
> +     ++errors;
>  
>        fclose (file);
>  
> @@ -1347,7 +1405,7 @@ try_dlopen (lt_dlhandle *phandle, const 
>        Otherwise (or in any case if the module was not yet found) try
>        opening just the module name as passed.  */
>        if ((dir || (!find_handle (user_search_path, base_name,
> -                              &newhandle, advise)
> +                              &newhandle, advise)
>                  && !find_handle (getenv (LTDL_SEARCHPATH_VAR), base_name,
>                                   &newhandle, advise)
>  #if defined(LT_MODULE_PATH_VAR)
> @@ -1356,14 +1414,14 @@ try_dlopen (lt_dlhandle *phandle, const 
>  #endif
>  #if defined(LT_DLSEARCH_PATH)
>                  && !find_handle (sys_dlsearch_path, base_name,
> -                                 &newhandle, advise)
> +                                 &newhandle, advise)
>  #endif
>                  )))
>       {
> -          if (tryall_dlopen (&newhandle, filename, advise) != 0)
> +       if (tryall_dlopen (&newhandle, filename, advise, 0) != 0)
>              {
>                newhandle = NULL;
> -            }
> +         }
>       }
>  
>        if (!newhandle)

These hunks are all superfluous, except of course the one functional
change to the tryall_dlopen call, for which one must look closely to not
overlook it.  Please separate functional changes from cleanup changes in
patch postings, that would make the former be much more easily spottable.

Otherwise the changes look ok to me.  Have you done testing on different
systems?  I haven't, only GNU/Linux.  Thanks.

Cheers,
Ralf