Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)

[ludo@gnu.org (Ludovic Courtès)]

Hi Mark!

Thanks for being quicker and more active than me!  ;-)

Overall, the approach of mimicking what the lookup procedure of
‘lt_dlopenext’ sounds good to me.

Mark H Weaver <mhw@netris.org> skribis:

> Following Bruce's suggestion, it causes 'sysdep_dynl_link' to manually
> search additional directories if 'lt_dlopenext' fails to find the
> library in the default paths.

Thus, that doesn’t solve the problem described at
<http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>,
right?

To solve it, we’d have to do our own lookup unconditionally.

> So what additional directories does it search?
>
> If GUILE_SYSTEM_EXTENSIONS_PATH is set (even if it's empty), then it
> specifies the additional directories to search.  If it's unset, then the
> default is to search SCM_LIB_DIR and SCM_EXTENSIONS_DIR.
>
> *** Note that this changes the search order in the case where
> GUILE_SYSTEM_EXTENSIONS_PATH is set to a non-empty string.
>
> Currently, a non-empty GUILE_SYSTEM_EXTENSIONS_PATH is passed to
> lt_dladdsearchdir, so it is searched before LTDL_LIBRARY_PATH and
> LD_LIBRARY_PATH, but this patch causes GUILE_SYSTEM_EXTENSIONS_PATH to
> be searched last, to be consistent with the handling of the default
> directories SCM_LIB_DIR and SCM_EXTENSIONS_DIR.  This seems sensible to
> me.  Does anyone see a problem with this change?

The point of $GUILE_SYSTEM_EXTENSIONS_PATH is to avoid using installed
Guile extensions while building Guile itself.  Wouldn’t the change
defeat that when $LTDL_LIBRARY_PATH or $LD_LIBRARY_PATH point to
previously installed extensions?  I’d rather not change anything.

Also, could you test compare the actual searches for both the patch and
unpatched dynl.c with strace?  For instance, with:

  $ LTDL_LIBRARY_PATH= LD_LIBRARY_PATH= strace -f -o x1 /before-patch/meta/guile -c '(use-modules (ice-9 readline))'
  $ LTDL_LIBRARY_PATH= LD_LIBRARY_PATH= strace -f -o x2 /after-patch/meta/guile -c '(use-modules (ice-9 readline))'
  $ LTDL_LIBRARY_PATH=/path/to/common/extensiondir strace -f -o y1 /before-patch/meta/guile -c '(use-modules (ice-9 readline))'
  $ LTDL_LIBRARY_PATH=/path/to/common/extensiondir strace -f -o y2 /after-patch/meta/guile -c '(use-modules (ice-9 readline))'

It’s an area where it’s very easy to introduce hard-to-find bugs, so I’m
a bit wary.

Note that the final patch will also need to revert the configury added
in e66ff09a.


Minor stylistic comments:

> +/* 'system_extensions_path' is used by 'sysdep_dynl_link' to search for
> +   dynamic libraries as a last resort, when they cannot be found in the
> +   usual library search paths. */
> +static char *system_extensions_path;

No need to repeat the variable name, nor to say where it’s used IMO.

> +  if (fname == NULL)
> +    {
> +      /* Return a handle for the program as a whole.  */
> +      handle = lt_dlopen (NULL);
> +    }

No extra brace.

> +          /* 'fname' contains no directory separators and was not in the
> +             usual library search paths, so now we search for it in the
> +             directories specified in 'system_extensions_path'. */

Should be FNAME and SYSTEM_EXTENSIONS_PATH (capitals) when referring to
the value of these variables (info "(standards) Comments").

> +          char *fname_attempt = malloc (strlen (system_extensions_path)
> +                                        + strlen (fname)
> +                                        + 1   /* for directory separator */
> +                                        + 1); /* for null terminator */

Use scm_gc_malloc_pointerless, and remove the corresponding
dynwind_free.

“+ 2” with no comment would be fine.

> +              /* Iterate over the components of 'system_extensions_path' */

Capitalize too (other occurrences omitted).

> +      system_extensions_path = (char *) malloc (strlen (SCM_LIB_DIR)
> +                                                + strlen (SCM_EXTENSIONS_DIR)
> +                                                + 1   /* for path separator */
> +                                                + 1); /* for null terminator */
> +      assert (system_extensions_path != NULL);

Use scm_gc_malloc_pointerless, no cast, and remove the assert.

Thanks!

Ludo’.