From: ludo@gnu.org (Ludovic Courtès)
To: Mark H Weaver <mhw@netris.org>
Cc: Bruce Korb <bruce.korb@gmail.com>, guile-devel@gnu.org
Subject: Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Date: Thu, 04 Oct 2012 22:37:54 +0200 [thread overview]
Message-ID: <877gr6j7ml.fsf@gnu.org> (raw)
In-Reply-To: <87y5jnn8y4.fsf_-_@tines.lan> (Mark H. Weaver's message of "Wed, 03 Oct 2012 06:31:15 -0400")
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’.
next prev parent reply other threads:[~2012-10-04 20:37 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
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 [this message]
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=877gr6j7ml.fsf@gnu.org \
--to=ludo@gnu.org \
--cc=bruce.korb@gmail.com \
--cc=guile-devel@gnu.org \
--cc=mhw@netris.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).