Re: [PATCH] do not augment environment

[Mark H Weaver <mhw@netris.org>]

Hi Bruce,

Thanks for the patch, but I think it needs more work before it can be
committed.  See below for my comments.

> diff --git a/libguile/dynl.c b/libguile/dynl.c
> index 72305a4..999627a 100644
> --- a/libguile/dynl.c
> +++ b/libguile/dynl.c
> @@ -81,7 +81,25 @@ sysdep_dynl_link (const char *fname, const char *subr)
>    lt_dlhandle handle;
>
>    if (fname != NULL)
> -    handle = lt_dlopenext (fname);
> +    {
> +      char * buf;
> +
> +      handle = lt_dlopenext (fname);
> +      if (handle == NULL)
> +        do
> +          {
> +            static char const ext_dir[] = SCM_EXTENSIONS_DIR;
> +            static char const lib_dir[] = SCM_LIB_DIR;
> +            buf = alloca (max(sizeof (ext_dir), sizeof (lib_dir))
> +                          + strlen (fname) + 1); // fault on failure
> +            sprintf (buf, "%s/%s", lib_dir, fname);
> +            handle = lt_dlopenext (buf);
> +            if (handle != NULL)
> +              break;
> +            sprintf (buf, "%s/%s", ext_dir, fname);
> +            handle = lt_dlopenext (buf);
> +          } while (0);
> +    }
>    else
>      /* Return a handle for the program as a whole.  */
>      handle = lt_dlopen (NULL);

I see a few problems above:

* You assume that the directory separator is '/'.

* You should not do the extra search if 'fname' is an absolute
   pathname, and I'm not sure whether it should be done for relative
   pathnames containing directory separators.  Does anyone else have
   thoughts on that?

* As a stylistic issue, I don't like your trick of breaking out of the
   do-while.  I'd prefer something closer to this (but with the above
   problems addressed):

+      handle = lt_dlopenext (fname);
+      if (handle == NULL)
+        {
+          static char const ext_dir[] = SCM_EXTENSIONS_DIR;
+          static char const lib_dir[] = SCM_LIB_DIR;
+          buf = alloca (max(sizeof (ext_dir), sizeof (lib_dir))
+                        + strlen (fname) + 1); // fault on failure
+          sprintf (buf, "%s/%s", lib_dir, fname);
+          handle = lt_dlopenext (buf);
+          if (handle == NULL)
+            {
+              sprintf (buf, "%s/%s", ext_dir, fname);
+              handle = lt_dlopenext (buf);
+            }
+        }

> @@ -152,26 +146,15 @@ sysdep_dynl_init ()
>    lt_dlinit ();
>
>    env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
> -  if (env && strcmp (env, "") == 0)
> -    /* special-case interpret system-ltdl-path=="" as meaning no system path,
> -       which is the case during the build */
> -    ;
> -  else if (env)
> +  if (env == NULL)
> +    return;
> +
> +  /* special-case interpret system-ltdl-path=="" as meaning no system path,
> +     which is the case during the build */
> +  if (*env != '\0')
>      /* FIXME: should this be a colon-separated path? Or is the only point to
>         allow the build system to turn off the installed extensions path? */
>      lt_dladdsearchdir (env);
> -  else
> -    {
> -      /* Add SCM_LIB_DIR and SCM_EXTENSIONS_DIR to the loader's search
> -	 path.  `lt_dladdsearchdir' and $LTDL_LIBRARY_PATH can't be used
> -	 for that because they are searched before the system-dependent
> -	 search path, which is the one `libtool --mode=execute -dlopen'
> -	 fiddles with (info "(libtool) Libltdl Interface").  See
> -	 <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>
> -	 for details.  */
> -      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_LIB_DIR);
> -      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_EXTENSIONS_DIR);
> -    }
>  }

In the current code, SCM_LIB_DIR and SCM_EXTENSIONS_DIR are only
added to the library search path when GUILE_SYSTEM_EXTENSIONS_PATH is 
not set.  Your patch mishandles this, because it _always_ acts as if 
SCM_LIB_DIR and SCM_EXTENSIONS_DIR have been added to the search path.

Would you be willing to produce an updated patch that fixes these problems?

    Thanks,
      Mark