Date: Fri, 29 Jan 2010 22:50:38 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: =?iso-8859-1?Q?G=E1bor_K=F6vesd=E1n?= <gabor@FreeBSD.org> Cc: Xin LI <delphij@delphij.net>, current@freebsd.org Subject: Re: NLS/strerror efficiency Message-ID: <20100129215038.GA95021@stack.nl> In-Reply-To: <4B633F2D.6010804@FreeBSD.org> References: <20100119212019.GL59590@deviant.kiev.zoral.com.ua> <4B56CACF.50508@FreeBSD.org> <yge1vhgvdd3.wl%ume@mahoroba.org> <4B5B4F4B.3030201@FreeBSD.org> <20100124091911.GI31243@server.vk2pj.dyndns.org> <4B5C27B9.1080805@FreeBSD.org> <20100124113718.GC3877@deviant.kiev.zoral.com.ua> <4B5CD916.1060003@FreeBSD.org> <20100126222338.GA40281@stack.nl> <4B633F2D.6010804@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@ 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)
> @@ -374,8 +376,8 @@
> }
>
> 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.
> @@ -390,8 +392,8 @@
>
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100129215038.GA95021>