From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Mark H Weaver Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] do not augment environment Date: Sun, 30 Sep 2012 22:38:36 -0400 Message-ID: <5069022C.9040600@netris.org> References: <505CF9FC.9060007@gmail.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1349059133 18354 80.91.229.3 (1 Oct 2012 02:38:53 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 1 Oct 2012 02:38:53 +0000 (UTC) Cc: Guile Development To: Bruce Korb Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Mon Oct 01 04:38:58 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 1TIVuL-00067r-1L for guile-devel@m.gmane.org; Mon, 01 Oct 2012 04:38:57 +0200 Original-Received: from localhost ([::1]:58781 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TIVuF-0000tw-GU for guile-devel@m.gmane.org; Sun, 30 Sep 2012 22:38:51 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:44275) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TIVuC-0000to-VA for guile-devel@gnu.org; Sun, 30 Sep 2012 22:38:50 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TIVuB-0006Of-LR for guile-devel@gnu.org; Sun, 30 Sep 2012 22:38:48 -0400 Original-Received: from world.peace.net ([96.39.62.75]:55504) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TIVuB-0006OZ-Fu for guile-devel@gnu.org; Sun, 30 Sep 2012 22:38:47 -0400 Original-Received: from 209-6-91-212.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com ([209.6.91.212] helo=[192.168.1.176]) by world.peace.net with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.72) (envelope-from ) id 1TIVu2-0002Jq-Nj; Sun, 30 Sep 2012 22:38:38 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.7) Gecko/20120922 Icedove/10.0.7 In-Reply-To: <505CF9FC.9060007@gmail.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 96.39.62.75 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:14918 Archived-At: 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 > - > - 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