[illumos-Developer] Problems in setlocale and codereview

[Joerg Schilling]

"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