Re: NLS/strerror efficiency

Jilles Tjoelker wrote:
> On Fri, Jan 29, 2010 at 09:03:57PM +0100, Gábor Kövesdán wrote:
>   
>>>> +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.
>>     
>
> If you look in <pthread.h> you will notice that
> PTHREAD_RWLOCK_INITIALIZER is simply NULL. Also, pthread_rwlock_t is
> just a pointer. However, this may well change later on to allow rwlocks
> in shared memory, making pthread_rwlock_t a struct and
> PTHREAD_RWLOCK_INITIALIZER something more complicated. It already works
> that way in various other implementations, and sem_t has already been
> changed similarly in 9-CURRENT.
>
>   
>>> 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.
>>     
>
> strcmp(3) and the POSIX spec do not specifically allow passing NULL to
> strcmp(), so it is not valid to do so. It seems that gcc treats a
> literal strcmp(NULL, NULL) specially, replacing it with 0, but any real
> strcmp call involving NULL segfaults.
>
> This probably needs to become something more complicated like
> (np->lang == NULL || lang == NULL ? np->lang == lang :
> strcmp(np->lang, lang) == 0)
>
>   
>> _at__at_ -374,8 +376,8 _at__at_
>>  	}
>>  
>>  	if (_fstat(fd, &st) != 0) {
>> +		_close(fd);
>>  		SAVEFAIL(name, errno);
>> -		_close(fd);
>>  		return (NLERR);
>>  	}
>>     
>
> Be careful that cleanup actions like these might overwrite errno.
> munmap() and _close() are system calls and they should not fail in this
> case (read-only file), so it should be safe. It is cleaner to save errno
> immediately after the failing call, though.
>
>   
>> _at__at_ -390,8 +392,8 _at__at_
>>  
>>  	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);
>>  	}
>>     
>
> The errno value seems garbage. SAVEFAIL with EINVAL, or perhaps use
> EFTYPE (in both places).
>
> The cast to size_t reminds me to ask what happens in the pathological
> case of a catalog file bigger than a size_t can describe, which may
> happen on 32-bit systems. I think this should fail rather than mapping
> the initial size%(1<<32) part.
>   
Hi Jilles,

thanks for pointing out these special cases. I've found some more 
myself, as well and I've made a patch. For this one I've tested all of 
the major cases I could think of and it seems to work for those ones. 
Patch is attached.

Gabor