Date: Fri, 29 Jan 2010 21:03:57 +0100 From: =?ISO-8859-1?Q?G=E1bor_K=F6vesd=E1n?= <gabor@FreeBSD.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: Xin LI <delphij@delphij.net>, Hajimu UMEMOTO <ume@freebsd.org>, current@freebsd.org Subject: Re: NLS/strerror efficiency Message-ID: <4B633F2D.6010804@FreeBSD.org> In-Reply-To: <20100126222338.GA40281@stack.nl> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4B633F2D.6010804>