Re: RFC: ldconfig speedup

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, Jul 02, 2007 at 09:35:14AM +0200, Andreas Jaeger wrote:
> +  /* Check that directory exists and create if needed.  */
> +  dir = xstrdup (aux_cache_name);
> +  dir = dirname (dir);
> +  if (stat64 (dir, &st) < 0)
> +    {
> +      if (mkdir (dir, 0755) < 0)
> +	error (EXIT_FAILURE, errno, _("Cannot create directory %s"), dir);
> +    }
> +  free (dir);

> +  /* Create file.  */
> +  fd = open (temp_name, O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW,
> +	     S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR);
> +  if (fd < 0)
> +    error (EXIT_FAILURE, errno,
> +	   _("Can't create temporary auxiliary cache file %s"),
> +	   temp_name);

> +  /* Make sure user can always read auxiliary cache file.  */
> +  if (chmod (temp_name, S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR))
> +    error (EXIT_FAILURE, errno,
> +	   _("Changing access rights of %s to %#o failed"), temp_name,
> +	   S_IROTH|S_IRGRP|S_IRUSR|S_IWUSR);
> +

Why do you need /var/cache/ldconfig and /var/cache/ldconfig/ldconfig.aux-cache
world readable?  While /etc/ld.so.cache clearly has to be world readable,
otherwise ld.so couldn't load it, the aux cache can be only readable by
root and although it doesn't contain a lot of sensitive data, it contains
some (ldconfig executed as root can see in non-world readable/executable
directories and the aux cache tells you ctime, ino/dev, sizes of the files
there).  If you run ldconfig as normal user, it would just ignore the aux
cache (it won't be able to write ld.so.cache anyway).
BTW, why the duplication of name?  A file in /var/cache/ldconfig/ directory
is ldconfig related already by the presence in that directory, so it could
be just /var/cache/ldconfig/aux-cache.

> @@ -694,6 +704,7 @@ search_dir (const struct dir_entry *entr
>  #endif
>  	      !is_hwcap_platform (direntry->d_name)))
>  	continue;
> +
>        len = strlen (direntry->d_name);
>        /* Skip temporary files created by the prelink program.  Files with
>  	 names like these are never really DSOs we want to look at.  */
> @@ -726,20 +737,15 @@ search_dir (const struct dir_entry *entr
>  	    }
>  	  sprintf (real_file_name, "%s/%s", dir_name, direntry->d_name);
>  	}
> -#ifdef _DIRENT_HAVE_D_TYPE
> -      if (direntry->d_type != DT_UNKNOWN)
> -	lstat_buf.st_mode = DTTOIF (direntry->d_type);
> -      else
> -#endif
> -	if (__builtin_expect (lstat64 (real_file_name, &lstat_buf), 0))
> -	  {
> -	    error (0, errno, _("Cannot lstat %s"), file_name);
> -	    continue;
> -	  }
> +      if (__builtin_expect (lstat64 (real_file_name, &lstat_buf), 0))
> +	{
> +	  error (0, errno, _("Cannot lstat %s"), file_name);
> +	  continue;
> +	}

Why have you changed this?  It seems to me that you are removing
a useful optimization.  Although before this change lstat_buf fields
other than the file type in lstat_buf.st_mode was only needed for
directories and now is needed also for regular files, still the above change
means you call lstat64 unnecessarily for symbolic links at least (where
you only care about stat_buf contains which you copy over into lstat_buf
fields).  And symbolic links are roughly half of all the directory entries.
In the current code is:
          if (is_link)
            {
              new_entry->ino = stat_buf.st_ino;
              new_entry->dev = stat_buf.st_dev;
            }
          else
            {
              new_entry->ino = lstat_buf.st_ino;
              new_entry->dev = lstat_buf.st_dev;
            }
pointless, as lstat_buf.st_ino and lstat_buf.st_dev were set to
stat_buf.st_ino and stat_buf.st_dev if is_link (of course if
d_type stuff is restored, the previous code would need too and then
the if above is still needed).

	Jakub