Re: NLS/strerror efficiency

On Fri, Jan 29, 2010 at 09:03:57PM +0100, Gábor Kövesdán wrote:
> >> +static pthread_rwlock_t		 rwlock;

> > Use PTHREAD_RWLOCK_INITIALIZER here to avoid problems with initializing
> > the lock.

> I talked to delphij_at_ about this. Shouldn't pthread_rwlock_rdlock() and 
> pthread_rwlock_wrlock() automatically initialize the lock if it is NULL? 
> We even removed the pthread_rwlock_init() call and it just works.

If you look in <pthread.h> you will notice that
PTHREAD_RWLOCK_INITIALIZER is simply NULL. Also, pthread_rwlock_t is
just a pointer. However, this may well change later on to allow rwlocks
in shared memory, making pthread_rwlock_t a struct and
PTHREAD_RWLOCK_INITIALIZER something more complicated. It already works
that way in various other implementations, and sem_t has already been
changed similarly in 9-CURRENT.

> > Hmm, so an error for one language makes it give up for all other
> > languages too? It is possible that a catalog is only available in a few
> > languages.

> Fixed.

> >> +				UNLOCK(NLERR);
> >> +				NLRETERR(np->caterrno);
> >> +			} else if (strcmp(np->lang, lang) == 0) {

> > Some code below can apparently set np->lang = NULL, how does that work?

> NULL means locale-independent open, i.e. catopen() is given an absolute 
> path. We could add more if's to separate those cases more but that would 
> result in more code, while this just works. If name is set to an 
> absolute path, lang will be NULL and strcmp(NULL, NULL) will return 0, 
> so it will match.

strcmp(3) and the POSIX spec do not specifically allow passing NULL to
strcmp(), so it is not valid to do so. It seems that gcc treats a
literal strcmp(NULL, NULL) specially, replacing it with 0, but any real
strcmp call involving NULL segfaults.

This probably needs to become something more complicated like
(np->lang == NULL || lang == NULL ? np->lang == lang :
strcmp(np->lang, lang) == 0)

> _at__at_ -374,8 +376,8 _at__at_
>  	}
>  
>  	if (_fstat(fd, &st) != 0) {
> +		_close(fd);
>  		SAVEFAIL(name, errno);
> -		_close(fd);
>  		return (NLERR);
>  	}

Be careful that cleanup actions like these might overwrite errno.
munmap() and _close() are system calls and they should not fail in this
case (read-only file), so it should be safe. It is cleaner to save errno
immediately after the failing call, though.

> _at__at_ -390,8 +392,8 _at__at_
>  
>  	if (ntohl((u_int32_t)((struct _nls_cat_hdr *)data)->__magic) !=
>  	    _NLS_MAGIC) {
> +		munmap(data, (size_t)st.st_size);
>  		SAVEFAIL(name, errno);
> -		munmap(data, (size_t)st.st_size);
>  		NLRETERR(EINVAL);
>  	}

The errno value seems garbage. SAVEFAIL with EINVAL, or perhaps use
EFTYPE (in both places).

The cast to size_t reminds me to ask what happens in the pathological
case of a catalog file bigger than a size_t can describe, which may
happen on 32-bit systems. I think this should fail rather than mapping
the initial size%(1<<32) part.

-- 
Jilles Tjoelker