Re: NLS/strerror efficiency

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_at_ 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_at_ 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