Re: About the dll search algorithm of dlopen (patch-r3)

[Michael Haubenwallner <michael dot haubenwallner at ssi-schaefer dot com>]

On 08/26/2016 01:49 PM, Corinna Vinschen wrote:
> On Aug 26 13:18, Corinna Vinschen wrote:
> 
> Apart from these minor bits and pieces, I really like this new
> pathfinder class, btw.

Thank you! :)

While at it: Combining dlopen thoughts with the "forkables" background
around replacing dlls-in-use leads me to this thought for redundant
calls of dlopen:

As we've already agreed that GetModuleHandleEx might make sense,
I'm thinking whether pathfinder could be useful even for calling
GetModuleHandleEx before actually testing for the existing file.

First, we need to agree on which search order is correct for a
specific (redundant) dlopen usage. IMO, this should be for:

dlopen ("libz.so"):
  1) GetModuleHandleEx ("libz.so")
  2) GetModuleHandleEx ("cygz.dll")
  3) GetModuleHandleEx ("libz.dll")
  4) path_conv.exists-based only, as with current pathfinder

dlopen ("/usr/lib/libz.so"):
  1) GetModuleHandleEx ("\winpath\of\usr\bin\libz.so")
  2) GetModuleHandleEx ("\winpath\of\usr\bin\cygz.dll")
  3) GetModuleHandleEx ("\winpath\of\usr\bin\libz.dll")

  4) GetModuleHandleEx ("\winpath\of\usr\lib\libz.so")
  5) GetModuleHandleEx ("\winpath\of\usr\lib\cygz.dll")
  6) GetModuleHandleEx ("\winpath\of\usr\lib\libz.dll")

  7) path_conv.exists ("\winpath\of\usr\bin\libz.so")
  8) path_conv.exists ("\winpath\of\usr\bin\cygz.dll")
  9) path_conv.exists ("\winpath\of\usr\bin\libz.dll")

 10) path_conv.exists ("\winpath\of\usr\lib\libz.so")
 11) path_conv.exists ("\winpath\of\usr\lib\cygz.dll")
 12) path_conv.exists ("\winpath\of\usr\lib\libz.dll")

Thing is that with the latter (predefined path), in a first iteration
we need the same filenames to test for already-loaded as in a second
iteration where we test for real files.

Attaching a patch draft for a pathfinder criterion interface...

Thoughts?

/haubi/

diff --git a/winsup/cygwin/dlfcn.cc b/winsup/cygwin/dlfcn.cc
index 46348bb..c2cd740 100644
--- a/winsup/cygwin/dlfcn.cc
+++ b/winsup/cygwin/dlfcn.cc
@@ -168,7 +168,9 @@ get_full_path_of_dll (const char* str, path_conv &real_filename)
       finder.add_lib_searchdir ("/usr/lib", -1);
     }
 
-  if (finder.check_path_access (real_filename))
+  if (finder.find (pathfinder::
+		   exists_and_is_file (real_filename,
+				       PC_SYM_FOLLOW | PC_POSIX)))
     return true;
 
   /* If nothing worked, create a relative path from the original incoming
diff --git a/winsup/cygwin/pathfinder.h b/winsup/cygwin/pathfinder.h
index 11bb5fd..c7e79eb 100644
--- a/winsup/cygwin/pathfinder.h
+++ b/winsup/cygwin/pathfinder.h
@@ -109,32 +109,130 @@ public:
       add_lib_searchpath (getenv (envpath));
     }
 
-  /* Within each searchdir registered, try each registered basename to
-     find as executable.  Returns found dir/basename in real_filename.
-     Returns true when found. */
-  bool check_path_access (path_conv& real_filename)
+
+  /* pathfinder::criterion_interface
+     Overload this test method when you need separate dir and basename.  */
+  struct criterion_interface
+  {
+    virtual char const * name () const { return NULL; }
+
+    virtual bool test (searchdirlist::iterator dir,
+		       basenamelist::iterator name) const = 0;
+  };
+
+
+  /* pathfinder::simple_criterion_interface
+     Overload this test method when you need a single filename.  */
+  class simple_criterion_interface
+    : public criterion_interface
+  {
+    virtual bool test (searchdirlist::iterator dir,
+		       basenamelist::iterator name) const
+    {
+      /* Complete the filename path to search for within dir,
+	 We have allocated enough memory above.  */
+      searchdirlist::buffer_iterator dirbuf (dir);
+      memcpy (dirbuf->buffer () + dirbuf->stringlength (),
+	      name->string (), name->stringlength () + 1);
+      bool ret = test (dirbuf->string ());
+      /* reset original dir */
+      dirbuf->buffer ()[dirbuf->stringlength ()] = '\0';
+      return ret;
+    }
+
+  public:
+    virtual bool test (const char * filename) const = 0;
+  };
+
+
+  /* pathfinder::path_conv_criterion_interface
+     Overload this test method when you need a path_conv. */
+  class path_conv_criterion_interface
+    : public simple_criterion_interface
+  {
+    path_conv mypc_;
+    path_conv & pc_;
+    unsigned opt_;
+
+    /* simple_criterion_interface */
+    virtual bool test (const char * filename) const
+    {
+      pc_.check (filename, opt_);
+      return test (pc_);
+    }
+
+  public:
+    path_conv_criterion_interface (unsigned opt = PC_SYM_FOLLOW)
+      : mypc_ ()
+      , pc_ (mypc_)
+      , opt_ (opt)
+    {}
+
+    path_conv_criterion_interface (path_conv & ret, unsigned opt = PC_SYM_FOLLOW)
+      : mypc_ ()
+      , pc_ (ret)
+      , opt_ (opt)
+    {}
+
+    virtual bool test (path_conv & pc) const = 0;
+  };
+
+
+  /* pathfinder::exists_and_is_file
+     Test if path_conv argument does exist and is not a directory. */
+  struct exists_and_is_file
+    : public path_conv_criterion_interface
+  {
+    virtual char const * name () const { return "exists and is file"; }
+
+    exists_and_is_file (path_conv & pc, unsigned opt = PC_SYM_FOLLOW)
+      : path_conv_criterion_interface (pc, opt)
+    {}
+
+    /* path_conv_criterion_interface */
+    virtual bool test (path_conv & pc) const
+    {
+      if (pc.exists () && !pc.isdir ())
+	return true;
+
+      pc.error = ENOENT;
+      return false;
+    }
+  };
+
+
+  /* Find the single dir + basename that matches criterion.
+
+     Calls criterion.test method for each registered dir + basename
+     until returning true:
+       Returns true with found_dir + found_basename set.
+     If criterion.test method never returns true:
+       Returns false, not modifying found_dir nor found_basename.  */
+  bool find (criterion_interface const & criterion,
+	     searchdirlist::member const ** found_dir = NULL,
+	     basenamelist::member const ** found_basename = NULL)
   {
-    for (searchdirlist::buffer_iterator dir(searchdirs_.begin ());
+    char const * critname = criterion.name ();
+    for (searchdirlist::iterator dir(searchdirs_.begin ());
 	 dir != searchdirs_.end ();
 	 ++dir)
       for (basenamelist::iterator name = basenames_.begin ();
 	   name != basenames_.end ();
 	   ++name)
-	{
-	  /* Complete the filename path to search for.
-	     We have allocated enough memory above.  */
-	  memcpy (dir->buffer () + dir->stringlength (),
-		  name->string (), name->stringlength () + 1);
-	  real_filename.check (dir->string (), PC_SYM_FOLLOW | PC_POSIX);
-	  if (real_filename.exists () && !real_filename.isdir ())
-	    {
-	      debug_printf ("found %s", dir->buffer ());
-	      return true;
-	    }
-	  debug_printf ("no %s", dir->buffer ());
-	}
-    real_filename.error = ENOENT;
-    return !real_filename.error;
+	if (criterion.test (dir, name))
+	  {
+	    debug_printf ("(%s) take %s%s", critname,
+			  dir->string(), name->string ());
+	    if (found_dir)
+	      *found_dir = dir.operator -> ();
+	    if (found_basename)
+	      *found_basename = name.operator -> ();
+	    return true;
+	  }
+	else
+	  debug_printf ("(%s) skip %s%s", critname,
+			dir->string(), name->string ());
+    return false;
   }
 };