From owner-freebsd-current@FreeBSD.ORG Fri Jan 29 21:50:40 2010 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 012F31065672; Fri, 29 Jan 2010 21:50:40 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id B41328FC1A; Fri, 29 Jan 2010 21:50:39 +0000 (UTC) Received: from toad.stack.nl (toad.stack.nl [IPv6:2001:610:1108:5010::135]) by mx1.stack.nl (Postfix) with ESMTP id 9818435988D; Fri, 29 Jan 2010 22:50:38 +0100 (CET) Received: by toad.stack.nl (Postfix, from userid 1677) id 8E84C73F9D; Fri, 29 Jan 2010 22:50:38 +0100 (CET) Date: Fri, 29 Jan 2010 22:50:38 +0100 From: Jilles Tjoelker To: =?iso-8859-1?Q?G=E1bor_K=F6vesd=E1n?= Message-ID: <20100129215038.GA95021@stack.nl> References: <20100119212019.GL59590@deviant.kiev.zoral.com.ua> <4B56CACF.50508@FreeBSD.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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4B633F2D.6010804@FreeBSD.org> User-Agent: Mutt/1.5.18 (2008-05-17) Cc: Xin LI , current@freebsd.org Subject: Re: NLS/strerror efficiency X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Jan 2010 21:50:40 -0000 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 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