[illumos-Developer] Problems in setlocale and codereview

[Garrett D'Amore]

So reading your diffs, there is at least one problem, and I can fix the
buf problem and the associated handling for composite locales.

I'll post diffs soon.  Thanks.

	- Garrett

On Thu, 2010-09-02 at 22:33 +0200, Joerg Schilling wrote:
> "Garrett D'Amore" <garrett at nexenta.com> wrote:
> 
> > I think so.  Review the .hgrc in ~ to be sure.  I think there may be two
> > e-mail addresses referenced in there, and I recommend updating them
> > both.  At least one of them can take the human readable name as well,
> > i.e. "Garrett D'Amore <garrett at nexenta.com>" ... that way you get your
> > name in the logs as well as the e-mail address.
> 
> Could you please verify whether this is the tag:
> 
> [ui] 
> username= <joerg at schily.net> 
> 
> So would be 
> 
> username= "J. Schilling <joerg at schily.net>"
> username= "Jörg Schilling <joerg at schily.net>"
> username= "Joerg Schilling <joerg at schily.net>"
> 
> OK and has the quotes at the right place?
> BTW: for a real name tag, I don't like "Joerg Schilling"
> 
> > > > Let me review the code in more depth.  I've been in meetings all morning
> > > > and haven't had a chance to read the actual code yet.
> > > 
> > > Do you like to get a new source?
> >
> > Yes please.  Can you post a webrev up somewhere I can look at it?  I
> > find that is the easiest way for me to review code.  Thanks.
> 
> Doing this would cause a lot of work as I currently only could to this on a 
> copy that also has star integration....
> 
> Here is a recent diff:
> 
> --- setlocale.c.orig	Di Aug 31 20:49:26 2010
> +++ setlocale.c	Do Sep  2 20:16:50 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,14 +154,29 @@
>  				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 {
> +#ifndef	__ready__
> +			errno = EINVAL;
> +			return (NULL);
> +#else
>  			char	*buf;
>  			char	*save;
>  
>  			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) {
> @@ -186,6 +203,7 @@
>  				    ENCODING_LEN);
>  				buf = NULL;	/* for strtok's benefit */
>  			}
> +#endif
>  		}
>  	}
>  
> @@ -192,11 +210,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 +233,9 @@
>  	return (currentlocale());
>  }
>  
> +/*
> + * XXX is it really correct to exclude LC_COLLATE?
> + */
>  static char *
>  currentlocale(void)
>  {
> @@ -218,9 +243,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]);
> @@ -227,6 +260,7 @@
>  			}
>  			break;
>  		}
> +	}
>  	return (current_locale_string);
>  }
>  
> @@ -285,7 +319,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
> 
> -- 
>  EMail:joerg at schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
>        js at cs.tu-berlin.de                (uni)  
>        joerg.schilling at fokus.fraunhofer.de (work) Blog: http://schily.blogspot.com/
>  URL:  http://cdrecord.berlios.de/private/ ftp://ftp.berlios.de/pub/schily
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer