From owner-freebsd-current@FreeBSD.ORG Fri Jan 29 19:02:09 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 2892E106566C; Fri, 29 Jan 2010 19:02:09 +0000 (UTC) (envelope-from gabor@FreeBSD.org) Received: from server.mypc.hu (server.mypc.hu [87.229.73.95]) by mx1.freebsd.org (Postfix) with ESMTP id BAB6E8FC12; Fri, 29 Jan 2010 19:02:08 +0000 (UTC) Received: from server.mypc.hu (localhost [127.0.0.1]) by server.mypc.hu (Postfix) with ESMTP id AF8E714DA436; Fri, 29 Jan 2010 20:02:04 +0100 (CET) X-Virus-Scanned: amavisd-new at server.mypc.hu Received: from server.mypc.hu ([127.0.0.1]) by server.mypc.hu (server.mypc.hu [127.0.0.1]) (amavisd-new, port 10024) with LMTP id qQA6lqIteMzz; Fri, 29 Jan 2010 20:01:55 +0100 (CET) Received: from [192.168.1.121] (catv-89-132-179-104.catv.broadband.hu [89.132.179.104]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by server.mypc.hu (Postfix) with ESMTPSA id 1CBB414DA435; Fri, 29 Jan 2010 20:01:55 +0100 (CET) Message-ID: <4B633F2D.6010804@FreeBSD.org> Date: Fri, 29 Jan 2010 21:03:57 +0100 From: =?ISO-8859-1?Q?G=E1bor_K=F6vesd=E1n?= User-Agent: Thunderbird 2.0.0.23 (X11/20100119) MIME-Version: 1.0 To: Jilles Tjoelker 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> In-Reply-To: <20100126222338.GA40281@stack.nl> Content-Type: multipart/mixed; boundary="------------090104020009000102060209" Cc: Xin LI , Hajimu UMEMOTO , 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 19:02:09 -0000 This is a multi-part message in MIME format. --------------090104020009000102060209 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi Jilles, thanks for your observations, I've made a new patch to improve the parts you mentioned. It is attached. And I comment on some things below. > > These may return from the function if locking operations fail. They do > this without setting errno to a sensible value. > This was already fixed in the committed version. > >> -static nl_catd load_msgcat(const char *); >> +static nl_catd load_msgcat(const char *, const char *, const char *); >> >> +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. > > 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. > >> + /* Found cached successful entry */ >> + np->refcount++; >> + UNLOCK(NLERR); >> > > np leaked if unlock fails. > We discussed this with delphij@ and unlock cannot actually fail, because it can only fail in two cases: 1, uninitialized rwlock descriptor 2, lock wasn't actually acquired None of these can happen in this code, so we removed the return from there. > > Hmm, why not simply if (catd == np->catd) here? > Ok, done. > >> + np->refcount--; >> + if (np->refcount == 0) { >> + munmap(catd->__data, (size_t)catd->__size); >> + free(catd); >> + SLIST_REMOVE(&flist, np, catentry, list); >> + free(np); >> > > The two or three strings in np need to be freed too. > Done. > > np leaked if unlock fails. > > Same applies here, won't fail, return removed. >> + return (np->catd); >> + } >> + } >> + UNLOCK(NLERR); >> + >> + if ((fd = _open(path, O_RDONLY)) == -1) { >> + SAVEFAIL(name, errno); >> return (NLERR); >> + } >> >> if (_fstat(fd, &st) != 0) { >> + SAVEFAIL(name, errno); >> > > fd not closed if locking fails. > [...] > data still mapped if locking fails. > > [..] > > data still mapped if locking fails. > > I changed the order of free and caching here. > > > np leaked if unlock fails. > Same as above. Please take a look if the new patch seems good to you. Cheers, Gabor --------------090104020009000102060209 Content-Type: text/x-patch; name="msgcat.c.diff" Content-Transfer-Encoding: 8bit Content-Disposition: inline; filename="msgcat.c.diff" Index: nls/msgcat.c =================================================================== --- nls/msgcat.c (revisión: 203174) +++ nls/msgcat.c (copia de trabajo) @@ -89,7 +89,7 @@ static nl_catd load_msgcat(const char *, const char *, const char *); -static pthread_rwlock_t rwlock; +static pthread_rwlock_t rwlock = PTHREAD_RWLOCK_INITIALIZER; struct catentry { SLIST_ENTRY(catentry) list; @@ -135,12 +135,12 @@ /* Try to get it from the cache first */ RLOCK(NLERR); SLIST_FOREACH(np, &cache, list) { - if (strcmp(np->name, name) == 0) { + if ((strcmp(np->name, name) == 0) && (strcmp(np->lang, lang) == 0)) { if (np->caterrno != 0) { /* Found cached failing entry */ UNLOCK; NLRETERR(np->caterrno); - } else if (strcmp(np->lang, lang) == 0) { + } else { /* Found cached successful entry */ np->refcount++; UNLOCK; @@ -325,13 +325,15 @@ /* Remove from cache if not referenced any more */ WLOCK(-1); SLIST_FOREACH(np, &cache, list) { - if ((np->catd->__size == catd->__size) && - memcmp((const void *)np->catd, (const void *)catd, np->catd->__size) == 0) { + if (catd == np->catd) { np->refcount--; if (np->refcount == 0) { munmap(catd->__data, (size_t)catd->__size); free(catd); SLIST_REMOVE(&cache, np, catentry, list); + free(np->name); + free(np->path); + free(np->lang); free(np); } break; @@ -374,8 +376,8 @@ } if (_fstat(fd, &st) != 0) { + _close(fd); SAVEFAIL(name, errno); - _close(fd); return (NLERR); } @@ -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); } --------------090104020009000102060209--