Re: NLS/strerror efficiency

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, '_at_')) != NULL)
> _at__at_ -166,7 +225,7 _at__at_
>  			if (stat(path, &sbuf) == 0) {
>  				free(plang);
>  				free(base);
> -				return (load_msgcat(path));
> +				return (load_msgcat(path, name, lang));
>  			}
>  		} else {
>  			tmpptr = (char *)name;
> _at__at_ -182,20 +241,20 _at__at_
>  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;
> _at__at_ -228,7 +287,7 _at__at_
>  				} else {
>  					l = i + 1;
>  				}
> -}
> +			}
>  
>  			/* not found */
>  			goto notfound;
> _at__at_ -238,25 +297,41 _at__at_
>  		} 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);
>  }
>  
> _at__at_ -265,19 +340,38 _at__at_
>   */
>  
>  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);
>  	}
> _at__at_ -286,22 +380,39 _at__at_
>  	    (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