[illumos-Developer] Problems in setlocale and codereview

[Garrett D'Amore]

Thank you; I will review this ... I know that there were some tricky
things that came up during the implementation where the ordering was
sensitive elsewhere, so I want to be careful not to make things worse.

Have you already tested this patch?

I'm going to see if I can figure out the uninitialized buf problem, too.

	- Garrett

On Thu, 2010-09-02 at 17:34 +0200, Joerg Schilling wrote:
> "Garrett D'Amore" <garrett at nexenta.com> wrote:
> 
> > Thanks.  Can you post a bug on this (if not already done)?
> >
> > I had used the order of the array from Solaris, but didn't realize that
> > the implementation was sensitive to the ordering.  So my bad.
> 
> Here is my patch:
> 
> --- setlocale.c.orig	Di Aug 31 20:49:26 2010
> +++ setlocale.c	Do Sep  2 17:33:11 2010
> @@ -125,6 +125,8 @@
>  	if (!*locale) {
>  		if (category == LC_ALL) {
>  			for (i = 0; i < NUM_CATS; ++i) {
> +				if (i == LC_ALL)
> +					continue;
>  				env = __get_locale_env(i);
>  				if (strlen(env) > ENCODING_LEN) {
>  					errno = EINVAL;
> @@ -152,8 +154,11 @@
>  				errno = EINVAL;
>  				return (NULL);
>  			}
> -			for (i = 1; i < NUM_CATS; ++i)
> +			for (i = 0; i < NUM_CATS; ++i) {
> +				if (i == LC_ALL)
> +					continue;
>  				(void) strcpy(new_categories[i], locale);
> +			}
>  		} else {
>  			char	*buf;
>  			char	*save;
> @@ -160,6 +165,14 @@
>  
>  			buf = alloca(strlen(locale) + 1);
>  
> +			/*
> +			 * XXX the for loop starts with buf being uninitialized.
> +			 * XXX it may work after calling strcpy() but what is
> +			 * XXX the correct source r?? locale??
> +			 * XXX LC_ALL is incorrect here, use NUM_CATS instead
> +			 * XXX and check in a way that make the code independent
> +			 * XXX from the order in categories[]
> +			 */
>  			for (i = 0, save = NULL; i <= LC_ALL; i++) {
>  				r = strtok_r(buf, "/", &save);
>  				if (r == NULL) {
> @@ -192,11 +205,15 @@
>  	if (category != LC_ALL)
>  		return (loadlocale(category));
>  
> -	for (i = 0; i < LC_ALL; ++i) {
> +	for (i = 0; i < NUM_CATS; ++i) {
> +		if (i == LC_ALL)
> +			continue;
>  		(void) strcpy(saved_categories[i], current_categories[i]);
>  		if (loadlocale(i) == NULL) {
>  			saverr = errno;
> -			for (j = 1; j < i; j++) {
> +			for (j = 0; j < i; j++) {
> +				if (i == LC_ALL)
> +					continue;
>  				(void) strcpy(new_categories[j],
>  				    saved_categories[j]);
>  				if (loadlocale(j) == NULL) {
> @@ -211,6 +228,9 @@
>  	return (currentlocale());
>  }
>  
> +/*
> + * XXX is it really correct to exclude LC_COLLATE?
> + */
>  static char *
>  currentlocale(void)
>  {
> @@ -218,9 +238,17 @@
>  
>  	(void) strcpy(current_locale_string, current_categories[0]);
>  
> -	for (i = 1; i < LC_ALL; ++i)
> +	for (i = 0; i < NUM_CATS; ++i)
> +		if (i == LC_ALL)
> +			continue;
> +		if (i == LC_COLLATE)
> +			continue;
>  		if (strcmp(current_categories[1], current_categories[i])) {
> -			for (i = 1; i < LC_ALL; ++i) {
> +			for (i = 0; i < NUM_CATS; ++i) {
> +				if (i == LC_ALL)
> +					continue;
> +				if (i == LC_COLLATE)
> +					continue;
>  				(void) strcat(current_locale_string, "/");
>  				(void) strcat(current_locale_string,
>  				    current_categories[i]);
> @@ -285,7 +313,7 @@
>  	const char *env;
>  
>  	/* 1. check LC_ALL. */
> -	env = getenv(categories[0]);
> +	env = getenv(categories[LC_ALL]);
>  
>  	/* 2. check LC_* */
>  	if (env == NULL || !*env)
> 
> 
> Jörg
>