From: Mark H Weaver <mhw@netris.org>
To: Bruce Korb <bruce.korb@gmail.com>
Cc: Guile Development <guile-devel@gnu.org>
Subject: Re: [PATCH] do not augment environment
Date: Sun, 30 Sep 2012 22:38:36 -0400 [thread overview]
Message-ID: <5069022C.9040600@netris.org> (raw)
In-Reply-To: <505CF9FC.9060007@gmail.com>
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
next prev parent reply other threads:[~2012-10-01 2:38 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-21 23:36 [PATCH] do not augment environment Bruce Korb
2012-10-01 1:13 ` Bruce Korb
2012-10-01 2:38 ` Mark H Weaver [this message]
2012-10-01 14:24 ` Bruce Korb
2012-10-01 14:39 ` Bruce Korb
2012-10-01 16:59 ` Mark H Weaver
2012-10-01 18:27 ` Bruce Korb
2012-10-03 10:31 ` [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment) Mark H Weaver
2012-10-04 20:37 ` Ludovic Courtès
2012-10-06 2:30 ` Mark H Weaver
2012-10-06 12:36 ` Mark H Weaver
2012-10-07 21:16 ` Ludovic Courtès
2012-10-06 12:42 ` Ludovic Courtès
2012-10-06 14:31 ` Mark H Weaver
2012-10-05 9:43 ` Sjoerd van Leent Privé
2012-10-05 21:58 ` Mark H Weaver
2012-11-27 22:54 ` [PATCH] Fix library search order and don't change LD_LIBRARY_PATH Mark H Weaver
2012-11-27 22:59 ` Ludovic Courtès
2012-11-27 23:23 ` Mark H Weaver
2012-11-27 23:01 ` Bruce Korb
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/guile/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5069022C.9040600@netris.org \
--to=mhw@netris.org \
--cc=bruce.korb@gmail.com \
--cc=guile-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).