From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: ludo@gnu.org (Ludovic =?iso-8859-1?Q?Court=E8s?=) Newsgroups: gmane.lisp.guile.devel 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 Message-ID: <877gr6j7ml.fsf@gnu.org> References: <505CF9FC.9060007@gmail.com> <5069022C.9040600@netris.org> <87y5jnn8y4.fsf_-_@tines.lan> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: ger.gmane.org 1349401852 24067 80.91.229.3 (5 Oct 2012 01:50:52 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 5 Oct 2012 01:50:52 +0000 (UTC) Cc: Bruce Korb , guile-devel@gnu.org To: Mark H Weaver Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Oct 05 03:50:54 2012 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1TJx3h-0006Y7-4V for guile-devel@m.gmane.org; Fri, 05 Oct 2012 03:50:33 +0200 Original-Received: from localhost ([::1]:46887 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJsBI-00082E-9Y for guile-devel@m.gmane.org; Thu, 04 Oct 2012 16:38:04 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:46899) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJsBE-0007zW-Iq for guile-devel@gnu.org; Thu, 04 Oct 2012 16:38:01 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJsBD-0006Zi-42 for guile-devel@gnu.org; Thu, 04 Oct 2012 16:38:00 -0400 Original-Received: from xanadu.aquilenet.fr ([88.191.123.111]:45128) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJsBC-0006Zd-QW for guile-devel@gnu.org; Thu, 04 Oct 2012 16:37:59 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by xanadu.aquilenet.fr (Postfix) with ESMTP id 8C08999D6; Thu, 4 Oct 2012 22:37:56 +0200 (CEST) Original-Received: from xanadu.aquilenet.fr ([127.0.0.1]) by localhost (xanadu.aquilenet.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id CkY071aLtI2c; Thu, 4 Oct 2012 22:37:56 +0200 (CEST) Original-Received: from pluto (reverse-83.fdn.fr [80.67.176.83]) by xanadu.aquilenet.fr (Postfix) with ESMTPSA id B0BF87C73; Thu, 4 Oct 2012 22:37:55 +0200 (CEST) X-URL: http://www.fdn.fr/~lcourtes/ X-Revolutionary-Date: 13 =?iso-8859-1?Q?Vend=E9miaire?= an 221 de la =?iso-8859-1?Q?R=E9volution?= X-PGP-Key-ID: 0xEA52ECF4 X-PGP-Key: http://www.fdn.fr/~lcourtes/ludovic.asc X-PGP-Fingerprint: 83C4 F8E5 10A3 3B4C 5BEA D15D 77DD 95E2 EA52 ECF4 X-OS: x86_64-unknown-linux-gnu In-Reply-To: <87y5jnn8y4.fsf_-_@tines.lan> (Mark H. Weaver's message of "Wed, 03 Oct 2012 06:31:15 -0400") User-Agent: Gnus/5.130005 (Ma Gnus v0.5) Emacs/24.2 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 88.191.123.111 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:14927 Archived-At: Hi Mark! Thanks for being quicker and more active than me! ;-) Overall, the approach of mimicking what the lookup procedure of =E2=80=98lt_dlopenext=E2=80=99 sounds good to me. Mark H Weaver 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=E2=80=99t solve the problem described at , right? To solve it, we=E2=80=99d 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=E2=80=99t the change defeat that when $LTDL_LIBRARY_PATH or $LD_LIBRARY_PATH point to previously installed extensions? I=E2=80=99d 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=3D LD_LIBRARY_PATH=3D strace -f -o x1 /before-patch/m= eta/guile -c '(use-modules (ice-9 readline))' $ LTDL_LIBRARY_PATH=3D LD_LIBRARY_PATH=3D strace -f -o x2 /after-patch/me= ta/guile -c '(use-modules (ice-9 readline))' $ LTDL_LIBRARY_PATH=3D/path/to/common/extensiondir strace -f -o y1 /befor= e-patch/meta/guile -c '(use-modules (ice-9 readline))' $ LTDL_LIBRARY_PATH=3D/path/to/common/extensiondir strace -f -o y2 /after= -patch/meta/guile -c '(use-modules (ice-9 readline))' It=E2=80=99s an area where it=E2=80=99s very easy to introduce hard-to-find= bugs, so I=E2=80=99m 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=E2=80=99s used IMO. > + if (fname =3D=3D NULL) > + { > + /* Return a handle for the program as a whole. */ > + handle =3D 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 =3D 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. =E2=80=9C+ 2=E2=80=9D with no comment would be fine. > + /* Iterate over the components of 'system_extensions_path'= */ Capitalize too (other occurrences omitted). > + system_extensions_path =3D (char *) malloc (strlen (SCM_LIB_DIR) > + + strlen (SCM_EXTENSIONS= _DIR) > + + 1 /* for path separa= tor */ > + + 1); /* for null termin= ator */ > + assert (system_extensions_path !=3D NULL); Use scm_gc_malloc_pointerless, no cast, and remove the assert. Thanks! Ludo=E2=80=99.