Date: Tue, 26 Jan 2010 23:23:38 +0100 From: Jilles Tjoelker <jilles@stack.nl> To: =?iso-8859-1?Q?G=E1bor_K=F6vesd=E1n?= <gabor@FreeBSD.org> Cc: Hajimu UMEMOTO <ume@freebsd.org>, Peter Jeremy <peterjeremy@acm.org>, attilio@freebsd.org, Kostik Belousov <kostikbel@gmail.com>, Xin LI <delphij@delphij.net>, current@freebsd.org Subject: Re: NLS/strerror efficiency Message-ID: <20100126222338.GA40281@stack.nl> In-Reply-To: <4B5CD916.1060003@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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Jan 25, 2010 at 12:34:46AM +0100, Gábor Kövesdán wrote:
> Kostik Belousov wrote:
> >> I'll fix up my patch to do that.
> >>
> Here's my new patch with a little program to demonstrate that it works
> as expected even if locale is changed between various
> strerror(3)/strsignal(3) calls.
This idea seems sensible, but I have some comments.
> Index: nls/msgcat.c
> ===================================================================
> --- nls/msgcat.c (revisi?n: 202658)
> +++ nls/msgcat.c (copia de trabajo)
> +#define RLOCK(fail) { if (_pthread_rwlock_rdlock(&rwlock) != 0) return (fail); }
> +#define WLOCK(fail) { if (_pthread_rwlock_wrlock(&rwlock) != 0) return (fail); }
> +#define UNLOCK(fail) { if (_pthread_rwlock_unlock(&rwlock) != 0) return (fail); }
> +
> +#define SAVEFAIL(n, e) { WLOCK(NLERR); \
> + np = malloc(sizeof(struct catentry)); \
> + if (np != NULL) { \
> + np->name = strdup(n); \
> + np->caterrno = e; \
> + SLIST_INSERT_HEAD(&flist, np, list); \
> + } \
> + UNLOCK(NLERR); \
> + }
These may return from the function if locking operations fail. They do
this without setting errno to a sensible value.
> -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.
> +struct catentry {
> + SLIST_ENTRY(catentry) list;
> + char *name;
> + char *path;
> + int caterrno;
> + nl_catd catd;
> + char *lang;
> + int refcount;
> +};
> +
> +SLIST_HEAD(listhead, catentry) flist =
> + SLIST_HEAD_INITIALIZER(flist);
> +
> nl_catd
> catopen(const char *name, int type)
> {
> - int spcleft, saverr;
> - char path[PATH_MAX];
> - char *nlspath, *lang, *base, *cptr, *pathP, *tmpptr;
> - char *cptr1, *plang, *pter, *pcode;
> - struct stat sbuf;
> + int spcleft, saverr;
> + char path[PATH_MAX];
> + char *nlspath, *lang, *base, *cptr, *pathP, *tmpptr;
> + char *cptr1, *plang, *pter, *pcode;
> + struct stat sbuf;
> + struct catentry *np;
>
> if (name == NULL || *name == '\0')
> NLRETERR(EINVAL);
>
> - /* is it absolute path ? if yes, load immediately */
> if (strchr(name, '/') != NULL)
> - return (load_msgcat(name));
> + lang = NULL;
> + else {
> + if (type == NL_CAT_LOCALE)
> + lang = setlocale(LC_MESSAGES, NULL);
> + else
> + lang = getenv("LANG");
>
> - if (type == NL_CAT_LOCALE)
> - lang = setlocale(LC_MESSAGES, NULL);
> - else
> - lang = getenv("LANG");
> + if (lang == NULL || *lang == '\0' || strlen(lang) > ENCODING_LEN ||
> + (lang[0] == '.' &&
> + (lang[1] == '\0' || (lang[1] == '.' && lang[2] == '\0'))) ||
> + strchr(lang, '/') != NULL)
> + lang = "C";
> + }
>
> - if (lang == NULL || *lang == '\0' || strlen(lang) > ENCODING_LEN ||
> - (lang[0] == '.' &&
> - (lang[1] == '\0' || (lang[1] == '.' && lang[2] == '\0'))) ||
> - strchr(lang, '/') != NULL)
> - lang = "C";
> + /* Init rwlock used to protect cache */
> + if ((rwlock == (pthread_rwlock_t)0) &&
> + (_pthread_rwlock_init(&rwlock, NULL) != 0))
> + return (NLERR);
>
> + /* Try to get it from the cache first */
> + RLOCK(NLERR);
> + SLIST_FOREACH(np, &flist, list) {
> + if (strcmp(np->name, name) == 0) {
> + if (np->caterrno != 0) {
> + /* Found cached failing entry */
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.
> + 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?
> + /* Found cached successful entry */
> + np->refcount++;
> + UNLOCK(NLERR);
np leaked if unlock fails.
> + return (np->catd);
> + }
> + }
> + }
> + UNLOCK(NLERR);
> +
> + /* is it absolute path ? if yes, load immediately */
> + if (strchr(name, '/') != NULL)
> + return (load_msgcat(name, name, lang));
> +
> if ((plang = cptr1 = strdup(lang)) == NULL)
> return (NLERR);
> if ((cptr = strchr(cptr1, '@')) != NULL)
> @@ -166,7 +225,7 @@
> if (stat(path, &sbuf) == 0) {
> free(plang);
> free(base);
> - return (load_msgcat(path));
> + return (load_msgcat(path, name, lang));
> }
> } else {
> tmpptr = (char *)name;
> @@ -182,20 +241,20 @@
> char *
> catgets(nl_catd catd, int set_id, int msg_id, const char *s)
> {
> - struct _nls_cat_hdr *cat_hdr;
> - struct _nls_set_hdr *set_hdr;
> - struct _nls_msg_hdr *msg_hdr;
> - int l, u, i, r;
> + struct _nls_cat_hdr *cat_hdr;
> + struct _nls_set_hdr *set_hdr;
> + struct _nls_msg_hdr *msg_hdr;
> + int l, u, i, r;
>
> if (catd == NULL || catd == NLERR) {
> errno = EBADF;
> /* LINTED interface problem */
> - return (char *) s;
> -}
> + return ((char *)s);
> + }
>
> cat_hdr = (struct _nls_cat_hdr *)catd->__data;
> set_hdr = (struct _nls_set_hdr *)(void *)((char *)catd->__data
> - + sizeof(struct _nls_cat_hdr));
> + + sizeof(struct _nls_cat_hdr));
>
> /* binary search, see knuth algorithm b */
> l = 0;
> @@ -228,7 +287,7 @@
> } else {
> l = i + 1;
> }
> -}
> + }
>
> /* not found */
> goto notfound;
> @@ -238,25 +297,41 @@
> } else {
> l = i + 1;
> }
> -}
> + }
>
> notfound:
> /* not found */
> errno = ENOMSG;
> /* LINTED interface problem */
> - return (char *) s;
> + return ((char *)s);
> }
>
> int
> catclose(nl_catd catd)
> {
> + struct catentry *np;
> +
> if (catd == NULL || catd == NLERR) {
> errno = EBADF;
> return (-1);
> }
>
> - munmap(catd->__data, (size_t)catd->__size);
> - free(catd);
> + /* Remove from cache if not referenced any more */
> + WLOCK(-1);
> + SLIST_FOREACH(np, &flist, list) {
> + if ((np->catd->__size == catd->__size) &&
> + memcmp((const void *)np->catd, (const void *)catd, np->catd->__size) == 0) {
Hmm, why not simply if (catd == np->catd) here?
> + 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.
> + }
> + break;
> + }
> + }
> + UNLOCK(-1);
> return (0);
> }
>
> @@ -265,19 +340,38 @@
> */
>
> static nl_catd
> -load_msgcat(const char *path)
> +load_msgcat(const char *path, const char *name, const char *lang)
> {
> - struct stat st;
> - nl_catd catd;
> - void *data;
> - int fd;
> + struct stat st;
> + nl_catd catd;
> + struct catentry *np;
> + void *data;
> + int fd;
>
> - /* XXX: path != NULL? */
> + if (path == NULL) {
> + errno = EINVAL;
> + return (NLERR);
> + }
>
> - if ((fd = _open(path, O_RDONLY)) == -1)
> + /* One more try in cache; if it was not found by name,
> + it might still be found by absolute path. */
> + RLOCK(NLERR);
> + SLIST_FOREACH(np, &flist, list) {
> + if (strcmp(np->path, path) == 0) {
> + np->refcount++;
> + UNLOCK(NLERR);
np leaked if unlock fails.
> + 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.
> _close(fd);
> return (NLERR);
> }
> @@ -286,22 +380,39 @@
> (off_t)0);
> _close(fd);
>
> - if (data == MAP_FAILED)
> + if (data == MAP_FAILED) {
> + SAVEFAIL(name, errno);
> return (NLERR);
> + }
>
> if (ntohl((u_int32_t)((struct _nls_cat_hdr *)data)->__magic) !=
> _NLS_MAGIC) {
> + SAVEFAIL(name, errno);
data still mapped if locking fails.
> munmap(data, (size_t)st.st_size);
> NLRETERR(EINVAL);
> }
>
> if ((catd = malloc(sizeof (*catd))) == NULL) {
> + SAVEFAIL(name, errno);
data still mapped if locking fails.
> munmap(data, (size_t)st.st_size);
> return (NLERR);
> }
>
> catd->__data = data;
> catd->__size = (int)st.st_size;
> +
> + /* Caching opened catalog */
> + WLOCK(NLERR);
> + if ((np = malloc(sizeof(struct catentry))) != NULL) {
> + np->name = strdup(name);
> + np->path = strdup(path);
> + np->catd = catd;
> + np->lang = (lang == NULL) ? NULL : strdup(lang);
> + np->refcount = 1;
> + np->caterrno = 0;
> + SLIST_INSERT_HEAD(&flist, np, list);
> + }
> + UNLOCK(NLERR);
np leaked if unlock fails.
> return (catd);
> }
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100126222338.GA40281>