Re: RFC: ldconfig speedup

[Ulrich Drepper <drepper at redhat dot com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andreas Jaeger wrote:
> What do you think now?

Lot's of formatting and little style/semantic problems all over the place.

> +#define AUX_CACHEMAGIC		"glibc-ld.so.auxcache"
> +#define AUX_CACHE_VERSION	"1.0"
> +#define AUX_CACHEMAGIC_VERSION	AUX_CACHEMAGIC AUX_CACHE_VERSION

A nit: you probably want to have a separator (space, dash) between the
strings.


>  /* Save the contents of the cache.  */
>  void
> -save_cache (const char *cache_name)
> +save_cache (const char *cache_name, const char *aux_cache_name)
>  {
>    struct cache_entry *entry;
> +  struct stat64 st;
>    int fd, idx_old, idx_new;
>    size_t total_strlen, len;
>    char *strings, *str, *temp_name;
>    struct cache_file *file_entries = NULL;
>    struct cache_file_new *file_entries_new = NULL;
> +  struct aux_cache_file *aux_file_entries = NULL;
>    size_t file_entries_size = 0;
>    size_t file_entries_new_size = 0;
> +  size_t aux_file_entries_size = 0;
>    unsigned int str_offset;
> +  unsigned int aux_str_offset;
> +  char *dir;
> +

Variables should really be declared right before they are used.  We use C99.


> +  /* Fill in the header of the auxiliary cache.  */
> +  memset (aux_file_entries, 0, sizeof (struct aux_cache_file));

memset's second parameter is a char.

> +  /* Check that directory exists and create if needed.  */
> +  dir = xstrdup (aux_cache_name);

strdupa should be sufficient


> +  /* First remove an old copy if it exists.  */
> +  if (unlink (temp_name) && errno != ENOENT)
> +    error (EXIT_FAILURE, errno,
> +	   _("Can't remove old temporary auxiliary cache file %s"),
> +	   temp_name);
> +
> +  /* Create file.  */
> +  fd = open (temp_name, O_CREAT|O_WRONLY|O_TRUNC|O_NOFOLLOW,
> +	     S_IRGRP|S_IRUSR|S_IWUSR);

This makes no sense.  Either you replace on open (without O_EXCL) or you
unlink first and then use O_EXCL.  The former should be fine here.


> +  /* Make sure user can always read auxiliary cache file.  */
> +  if (chmod (temp_name, S_IRGRP|S_IRUSR|S_IWUSR))

And now this.  Why would you need an extra chmod call to install the
exact same access bits?  Either you use mode 0600 during the file
creation or you remove the chmod.


> +void
> +load_aux_cache (const char *cache_name, const char *aux_cache_name)
> +{
> +  size_t aux_cache_size;
> +  const char *aux_cache_data;
> +  struct stat64 st;
> +  int fd;
> +  unsigned int i;
> +  size_t j;
> +  struct aux_cache_file *aux_cache;
> +  struct cache_entry *new_entry, *last;
> +
> +  uint64_t hwcap;

Declare variables just before use.  And why this spurious empty line?


> +
> +  fd = open (aux_cache_name, O_RDONLY);
> +  if (fd < 0)
> +    return;
> +
> +  if (fstat64 (fd, &st) < 0 || st.st_size == 0)
> +    {
> +      close (fd);
> +      return;
> +    }

Not st_size == 0 but st_size < sizeof(header).


> +  aux_cache = mmap (0, st.st_size, PROT_READ, MAP_SHARED, fd, 0);

Why MAP_SHARED?  The OS will use the same memory unless the file is
written too.  And in this case you don't want the updated data which a)
is always an error and b) can only lead to problems.

Also, the first parameter of mmap is a pointer.


> +  if (aux_cache == MAP_FAILED)
> +    {
> +      close (fd);
> +      return;
> +    }
> +
> +  aux_cache_size = st.st_size;
> +  if (aux_cache_size < sizeof (struct aux_cache_file))
> +    {
> +      close (fd);
> +      return;
> +    }

Reduce code size by unifying the blocks.  gcc unfortunately usually
doesn't do it by itself.  Use goto is necessary.


> +  if (memcmp (aux_cache->magic, AUX_CACHEMAGIC, sizeof AUX_CACHEMAGIC - 1)
> +      || memcmp (aux_cache->version, AUX_CACHE_VERSION,
> +		      sizeof AUX_CACHE_VERSION - 1))

Why have two fields in the first place?  It just bloats the code.


> +  for (i = 0; i < aux_cache->nlibs; i++)
> +    {
> +      new_entry = (struct cache_entry *)
> +	xmalloc (sizeof (struct cache_entry));
> +      new_entry->lib = xstrdup (aux_cache_data + aux_cache->libs[i].key);
> +      new_entry->path = xstrdup (aux_cache_data + aux_cache->libs[i].value);

This is just wasteful.  Use one appropriately sized xmalloc call.


> @@ -632,7 +642,7 @@ search_dir (const struct dir_entry *entr
>    struct dlib_entry *dlibs;
>    struct dlib_entry *dlib_ptr;
>    struct stat64 lstat_buf, stat_buf;
> -  int is_link, is_dir;
> +  int is_link, is_dir, has_soname;

One variable per line.  This applies to many other places as well.  I
haven't spelled them all out.


>  	  is_dir = S_ISDIR (stat_buf.st_mode);
> +#ifndef _DIRENT_HAVE_D_TYPE
> +	  /* lstat is later stored, update contents.  */
> +	  lstat_buf.st_ino = stat_buf.st_ino;
> +	  lstat_buf.st_ctime = stat_buf.st_ctime;
> +	  lstat_buf.st_size = stat_buf.st_size;
> +	  lstat_buf.st_dev = stat_buf.st_dev;

Order all such copy operation so that the structure elements are ordered
according to offset.  Here and in other places.

- --
â Ulrich Drepper â Red Hat, Inc. â 444 Castro St â Mountain View, CA â
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFGirVZ2ijCOnn/RHQRAiy1AJ9OSXTPhdLu7opHND/7SHMJ75O47ACgnHxn
NVQlQ4rsfz3kuyC0LvV5pOk=
=sTr7
-----END PGP SIGNATURE-----