daemon.c broken on OpenBSD

daemon.c broken on OpenBSD

[Randal L. Schwartz]

Wow.

    gcc -o daemon.o -c -g -O2 -Wall -I/usr/local/include -L/usr/local/lib -Dstrcasestr=gitstrcasestr -DNO_STRCASESTR=1 -DSHA1_HEADER='<openssl/sha.h>' daemon.c
    In file included from /usr/include/sys/poll.h:54,
                     from daemon.c:7:
    /usr/include/ctype.h:67: error: syntax error before ']' token
    /usr/include/ctype.h:68: error: syntax error before ']' token
    /usr/include/ctype.h:70: error: syntax error before ']' token
    /usr/include/ctype.h:75: error: syntax error before ']' token
    /usr/include/ctype.h:78: error: syntax error before '(' token
    /usr/include/ctype.h:79: error: syntax error before '(' token
    /usr/include/ctype.h:93: error: syntax error before "c"
    In file included from /usr/include/sys/poll.h:54,
                     from daemon.c:7:
    /usr/include/ctype.h:91:1: unterminated #if
    /usr/include/ctype.h:40:1: unterminated #ifndef
    In file included from daemon.c:7:
    /usr/include/sys/poll.h:53:1: unterminated #ifndef
    /usr/include/sys/poll.h:28:1: unterminated #ifndef
    gmake: *** [daemon.o] Error 1

Apparently something in the early lines of daemon.c (before line 7)
is defining something that breaks the core /usr/include/ctype.h.

Lines 66 to 68 of ctype.h look like:

    #if defined(__GNUC__) || defined(_ANSI_LIBRARY) || defined(lint)
    int	isalnum(int);
    int	isalpha(int);

If that rings a bell, help me out here.  I'm guessing "isalnum" is getting
defined (wrongly).  Yeah, looks like in cache.h.  Why is this getting
defined?

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: daemon.c broken on OpenBSD

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

> Wow.
>...
> If that rings a bell, help me out here.  I'm guessing "isalnum" is getting
> defined (wrongly).  Yeah, looks like in cache.h.  Why is this getting
> defined?

Wow indeed.  It comes from this thread:

	http://marc.theaimsgroup.com/?l=git&m=112917422812418&w=2


Maybe something like this would help?

 ------------
[PATCH] do not override standard ctype macros, but use our own.

...since it can cause breakage in system supplied header files...

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

   It would be easier for *you* to grok if I say this patch is
   essentially this ;-):

	$ git-ls-files |
	xargs perl -i -p -e '
		s/\bis(space|digit|alpha|alnum)\b/is_$1/g;
		s/\bto(lower|upper)\b/to_$1/g;
	'

   except that I did not touch gitk.

diff --git a/apply.c b/apply.c
index e5c0b7d..9608f92 100644
--- a/apply.c
+++ b/apply.c
@@ -113,7 +113,7 @@ static unsigned long linelen(const char 
 
 static int is_dev_null(const char *str)
 {
-	return !memcmp("/dev/null", str, 9) && isspace(str[9]);
+	return !memcmp("/dev/null", str, 9) && is_space(str[9]);
 }
 
 #define TERM_SPACE	1
@@ -167,7 +167,7 @@ static char * find_name(const char *line
 	for (;;) {
 		char c = *line;
 
-		if (isspace(c)) {
+		if (is_space(c)) {
 			if (c == '\n')
 				break;
 			if (name_terminate(start, line-start, c, terminate))
@@ -447,7 +447,7 @@ static char *git_header_name(char *line,
 		/* second points at one past closing dq of name.
 		 * find the second name.
 		 */
-		while ((second < line + llen) && isspace(*second))
+		while ((second < line + llen) && is_space(*second))
 			second++;
 
 		if (line + llen <= second)
@@ -508,7 +508,7 @@ static char *git_header_name(char *line,
 			len = strlen(np);
 			if (len < cp - name &&
 			    !strncmp(np, name, len) &&
-			    isspace(name[len])) {
+			    is_space(name[len])) {
 				/* Good */
 				memmove(sp, np, len + 1);
 				return sp;
@@ -615,7 +615,7 @@ static int parse_num(const char *line, u
 {
 	char *ptr;
 
-	if (!isdigit(*line))
+	if (!is_digit(*line))
 		return 0;
 	*p = strtoul(line, &ptr, 10);
 	return ptr - line;
diff --git a/cache.h b/cache.h
index d776016..3f6ff0d 100644
--- a/cache.h
+++ b/cache.h
@@ -388,23 +388,17 @@ extern char git_default_email[MAX_GITNAM
 extern char git_default_name[MAX_GITNAME];
 
 /* Sane ctype - no locale, and works with signed chars */
-#undef isspace
-#undef isdigit
-#undef isalpha
-#undef isalnum
-#undef tolower
-#undef toupper
 extern unsigned char sane_ctype[256];
 #define GIT_SPACE 0x01
 #define GIT_DIGIT 0x02
 #define GIT_ALPHA 0x04
 #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
-#define isspace(x) sane_istest(x,GIT_SPACE)
-#define isdigit(x) sane_istest(x,GIT_DIGIT)
-#define isalpha(x) sane_istest(x,GIT_ALPHA)
-#define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
-#define tolower(x) sane_case((unsigned char)(x), 0x20)
-#define toupper(x) sane_case((unsigned char)(x), 0)
+#define is_space(x) sane_istest(x,GIT_SPACE)
+#define is_digit(x) sane_istest(x,GIT_DIGIT)
+#define is_alpha(x) sane_istest(x,GIT_ALPHA)
+#define is_alnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
+#define to_lower(x) sane_case((unsigned char)(x), 0x20)
+#define to_upper(x) sane_case((unsigned char)(x), 0)
 
 static inline int sane_case(int x, int high)
 {
diff --git a/commit.c b/commit.c
index 8f40318..4d094b5 100644
--- a/commit.c
+++ b/commit.c
@@ -379,7 +379,7 @@ static int add_user_info(const char *wha
 
 static int is_empty_line(const char *line, int len)
 {
-	while (len && isspace(line[len-1]))
+	while (len && is_space(line[len-1]))
 		len--;
 	return !len;
 }
diff --git a/compat/strcasestr.c b/compat/strcasestr.c
index b96414d..d1a4253 100644
--- a/compat/strcasestr.c
+++ b/compat/strcasestr.c
@@ -12,7 +12,7 @@ char *gitstrcasestr(const char *haystack
 		for (j = 0; j < nlen; j++) {
 			unsigned char c1 = haystack[i+j];
 			unsigned char c2 = needle[j];
-			if (toupper(c1) != toupper(c2))
+			if (to_upper(c1) != to_upper(c2))
 				goto next;
 		}
 		return (char *) haystack + i;
diff --git a/config.c b/config.c
index 519fecf..24b017f 100644
--- a/config.c
+++ b/config.c
@@ -40,7 +40,7 @@ static char *parse_value(void)
 		}
 		if (comment)
 			continue;
-		if (isspace(c) && !quote) {
+		if (is_space(c) && !quote) {
 			space = 1;
 			continue;
 		}
@@ -97,9 +97,9 @@ static int get_value(config_fn_t fn, cha
 		c = get_next_char();
 		if (c == EOF)
 			break;
-		if (!isalnum(c))
+		if (!is_alnum(c))
 			break;
-		name[len++] = tolower(c);
+		name[len++] = to_lower(c);
 		if (len >= MAXNAME)
 			return -1;
 	}
@@ -128,11 +128,11 @@ static int get_base_var(char *name)
 			return -1;
 		if (c == ']')
 			return baselen;
-		if (!isalnum(c))
+		if (!is_alnum(c))
 			return -1;
 		if (baselen > MAXNAME / 2)
 			return -1;
-		name[baselen++] = tolower(c);
+		name[baselen++] = to_lower(c);
 	}
 }
 
@@ -151,7 +151,7 @@ static int git_parse_file(config_fn_t fn
 			comment = 0;
 			continue;
 		}
-		if (comment || isspace(c))
+		if (comment || is_space(c))
 			continue;
 		if (c == '#' || c == ';') {
 			comment = 1;
@@ -165,9 +165,9 @@ static int git_parse_file(config_fn_t fn
 			var[baselen] = 0;
 			continue;
 		}
-		if (!isalpha(c))
+		if (!is_alpha(c))
 			break;
-		var[baselen] = tolower(c);
+		var[baselen] = to_lower(c);
 		if (get_value(fn, var, baselen+1) < 0)
 			break;
 	}
diff --git a/convert-objects.c b/convert-objects.c
index a892013..621c1bb 100644
--- a/convert-objects.c
+++ b/convert-objects.c
@@ -166,7 +166,7 @@ static unsigned long parse_oldstyle_date
 	const char **fmt = formats;
 
 	p = buffer;
-	while (isspace(c = *buf))
+	while (is_space(c = *buf))
 		buf++;
 	while ((c = *buf++) != '\n')
 		*p++ = c;
@@ -181,7 +181,7 @@ static unsigned long parse_oldstyle_date
 			buf = next;
 		} else {
 			const char **p = timezones;
-			while (isspace(*buf))
+			while (is_space(*buf))
 				buf++;
 			while (*p) {
 				if (!memcmp(buf, *p, strlen(*p))) {
@@ -217,7 +217,7 @@ static int convert_date_line(char *dst, 
 	dst += len;
 
 	/* Is it already in new format? */
-	if (isdigit(*date)) {
+	if (is_digit(*date)) {
 		int datelen = next - date;
 		memcpy(dst, date, datelen);
 		return len + datelen;
diff --git a/date.c b/date.c
index 63f5a09..340052d 100644
--- a/date.c
+++ b/date.c
@@ -131,9 +131,9 @@ static int match_string(const char *date
 	for (i = 0; *date; date++, str++, i++) {
 		if (*date == *str)
 			continue;
-		if (toupper(*date) == toupper(*str))
+		if (to_upper(*date) == to_upper(*str))
 			continue;
-		if (!isalnum(*date))
+		if (!is_alnum(*date))
 			break;
 		return 0;
 	}
@@ -145,7 +145,7 @@ static int skip_alpha(const char *date)
 	int i = 0;
 	do {
 		i++;
-	} while (isalpha(date[i]));
+	} while (is_alpha(date[i]));
 	return i;
 }
 
@@ -229,7 +229,7 @@ static int match_multi_number(unsigned l
 
 	num2 = strtol(end+1, &end, 10);
 	num3 = -1;
-	if (*end == c && isdigit(end[1]))
+	if (*end == c && is_digit(end[1]))
 		num3 = strtol(end+1, &end, 10);
 
 	/* Time? Date? */
@@ -295,7 +295,7 @@ static int match_digit(const char *date,
 	case ':':
 	case '/':
 	case '-':
-		if (isdigit(end[1])) {
+		if (is_digit(end[1])) {
 			int match = match_multi_number(num, *end, date, end, tm);
 			if (match)
 				return match;
@@ -310,7 +310,7 @@ static int match_digit(const char *date,
 	n = 0;
 	do {
 		n++;
-	} while (isdigit(date[n]));
+	} while (is_digit(date[n]));
 
 	/* Four-digit year or a timezone? */
 	if (n == 4) {
@@ -420,11 +420,11 @@ int parse_date(const char *date, char *r
 		if (!c || c == '\n')
 			break;
 
-		if (isalpha(c))
+		if (is_alpha(c))
 			match = match_alpha(date, &tm, &offset);
-		else if (isdigit(c))
+		else if (is_digit(c))
 			match = match_digit(date, &tm, &offset, &tm_gmt);
-		else if ((c == '-' || c == '+') && isdigit(date[1]))
+		else if ((c == '-' || c == '+') && is_digit(date[1]))
 			match = match_tz(date, &offset);
 
 		if (!match) {
diff --git a/diff-tree.c b/diff-tree.c
index 382011a..f7b16eb 100644
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -137,7 +137,7 @@ static int diff_tree_stdin(char *line)
 	line[len-1] = 0;
 	if (get_sha1_hex(line, commit))
 		return -1;
-	if (isspace(line[40]) && !get_sha1_hex(line+41, parent)) {
+	if (is_space(line[40]) && !get_sha1_hex(line+41, parent)) {
 		line[40] = 0;
 		line[81] = 0;
 		sprintf(this_header, "%s (from %s)\n", line, line+41);
diff --git a/ident.c b/ident.c
index bc89e1d..0a3d735 100644
--- a/ident.c
+++ b/ident.c
@@ -34,7 +34,7 @@ static void copy_gecos(struct passwd *w,
 		}
 		if (len + nlen < sz) {
 			/* Sorry, Mr. McDonald... */
-			*dst++ = toupper(*w->pw_name);
+			*dst++ = to_upper(*w->pw_name);
 			memcpy(dst, w->pw_name + 1, nlen - 1);
 			dst += nlen - 1;
 		}
diff --git a/mailinfo.c b/mailinfo.c
index cb853df..3a06240 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -6,9 +6,10 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <ctype.h>
 #include <iconv.h>
 
+#include "cache.h"
+
 #ifdef NO_STRCASESTR
 extern char *gitstrcasestr(const char *haystack, const char *needle);
 #endif
@@ -61,7 +62,7 @@ static int handle_from(char *line)
 	 */
 	while (at > line) {
 		char c = at[-1];
-		if (isspace(c))
+		if (is_space(c))
 			break;
 		if (c == '<') {
 			at[-1] = ' ';
@@ -72,7 +73,7 @@ static int handle_from(char *line)
 	dst = email;
 	for (;;) {
 		unsigned char c = *at;
-		if (!c || c == '>' || isspace(c)) {
+		if (!c || c == '>' || is_space(c)) {
 			if (c == '>')
 				*at = ' ';
 			break;
@@ -90,7 +91,7 @@ static int handle_from(char *line)
 	at = line + strlen(line);
 	while (at > line) {
 		unsigned char c = *--at;
-		if (!isspace(c)) {
+		if (!is_space(c)) {
 			at[(c == ')') ? 0 : 1] = 0;
 			break;
 		}
@@ -99,7 +100,7 @@ static int handle_from(char *line)
 	at = line;
 	for (;;) {
 		unsigned char c = *at;
-		if (!c || !isspace(c)) {
+		if (!c || !is_space(c)) {
 			if (c == '(')
 				at++;
 			break;
@@ -164,7 +165,7 @@ static int handle_subcontent_type(char *
 	if (*charset) {
 		int i, c;
 		for (i = 0; (c = charset[i]) != 0; i++)
-			charset[i] = tolower(c);
+			charset[i] = to_lower(c);
 	}
 	return 0;
 }
@@ -199,7 +200,7 @@ static int is_multipart_boundary(const c
 static int eatspace(char *line)
 {
 	int len = strlen(line);
-	while (len > 0 && isspace(line[len-1]))
+	while (len > 0 && is_space(line[len-1]))
 		line[--len] = 0;
 	return len;
 }
@@ -211,27 +212,27 @@ static int eatspace(char *line)
 /* First lines of body can have From:, Date:, and Subject: */
 static int handle_inbody_header(int *seen, char *line)
 {
-	if (!memcmp("From:", line, 5) && isspace(line[5])) {
+	if (!memcmp("From:", line, 5) && is_space(line[5])) {
 		if (!(*seen & SEEN_FROM) && handle_from(line+6)) {
 			*seen |= SEEN_FROM;
 			return 1;
 		}
 	}
-	if (!memcmp("Date:", line, 5) && isspace(line[5])) {
+	if (!memcmp("Date:", line, 5) && is_space(line[5])) {
 		if (!(*seen & SEEN_DATE)) {
 			handle_date(line+6);
 			*seen |= SEEN_DATE;
 			return 1;
 		}
 	}
-	if (!memcmp("Subject:", line, 8) && isspace(line[8])) {
+	if (!memcmp("Subject:", line, 8) && is_space(line[8])) {
 		if (!(*seen & SEEN_SUBJECT)) {
 			handle_subject(line+9);
 			*seen |= SEEN_SUBJECT;
 			return 1;
 		}
 	}
-	if (!memcmp("[PATCH]", line, 7) && isspace(line[7])) {
+	if (!memcmp("[PATCH]", line, 7) && is_space(line[7])) {
 		if (!(*seen & SEEN_SUBJECT)) {
 			handle_subject(line);
 			*seen |= SEEN_SUBJECT;
@@ -282,10 +283,10 @@ static void cleanup_space(char *buf)
 	unsigned char c;
 	while ((c = *buf) != 0) {
 		buf++;
-		if (isspace(c)) {
+		if (is_space(c)) {
 			buf[-1] = ' ';
 			c = *buf;
-			while (isspace(c)) {
+			while (is_space(c)) {
 				int len = strlen(buf);
 				memmove(buf, buf+1, len);
 				c = *buf;
@@ -312,7 +313,7 @@ static void check_header(char *line, int
 	for (i = 0; header[i].name; i++) {
 		int len = header[i].namelen;
 		if (!strncasecmp(line, header[i].name, len) &&
-		    line[len] == ':' && isspace(line[len + 1])) {
+		    line[len] == ':' && is_space(line[len + 1])) {
 			header[i].func(line + len + 2);
 			break;
 		}
@@ -491,7 +492,7 @@ static void decode_header_bq(char *it)
 		if (!cp)
 			return; /* no munging */
 		for (sp = ep; sp < cp; sp++)
-			charset_q[sp - ep] = tolower(*sp);
+			charset_q[sp - ep] = to_lower(*sp);
 		charset_q[cp - ep] = 0;
 		encoding = cp[1];
 		if (!encoding || cp[2] != '?')
@@ -499,7 +500,7 @@ static void decode_header_bq(char *it)
 		ep = strstr(cp + 3, "?=");
 		if (!ep)
 			return; /* no munging */
-		switch (tolower(encoding)) {
+		switch (to_lower(encoding)) {
 		default:
 			return; /* no munging */
 		case 'b':
@@ -709,11 +710,6 @@ static void handle_body(void)
 static const char mailinfo_usage[] =
 	"git-mailinfo [-k] [-u] msg patch <mail >info";
 
-static void usage(void) {
-	fprintf(stderr, "%s\n", mailinfo_usage);
-	exit(1);
-}
-
 int main(int argc, char **argv)
 {
 	while (1 < argc && argv[1][0] == '-') {
@@ -722,12 +718,12 @@ int main(int argc, char **argv)
 		else if (!strcmp(argv[1], "-u"))
 			metainfo_utf8 = 1;
 		else
-			usage();
+			usage(mailinfo_usage);
 		argc--; argv++;
 	}
 
 	if (argc != 3)
-		usage();
+		usage(mailinfo_usage);
 	cmitmsg = fopen(argv[1], "w");
 	if (!cmitmsg) {
 		perror(argv[1]);
diff --git a/mailsplit.c b/mailsplit.c
index 189f4ed..de9fb2b 100644
--- a/mailsplit.c
+++ b/mailsplit.c
@@ -33,11 +33,11 @@ static int is_from_line(const char *line
 			break;
 	}
 
-	if (!isdigit(colon[-4]) ||
-	    !isdigit(colon[-2]) ||
-	    !isdigit(colon[-1]) ||
-	    !isdigit(colon[ 1]) ||
-	    !isdigit(colon[ 2]))
+	if (!is_digit(colon[-4]) ||
+	    !is_digit(colon[-2]) ||
+	    !is_digit(colon[-1]) ||
+	    !is_digit(colon[ 1]) ||
+	    !is_digit(colon[ 2]))
 		return 0;
 
 	/* year */
diff --git a/pack-objects.c b/pack-objects.c
index b3e6152..314616d 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -464,7 +464,7 @@ int main(int argc, char **argv)
 		p = line+40;
 		while (*p) {
 			unsigned char c = *p++;
-			if (isspace(c))
+			if (is_space(c))
 				continue;
 			hash = hash * 11 + c;
 		}
diff --git a/patch-id.c b/patch-id.c
index edbc4aa..ba09d2a 100644
--- a/patch-id.c
+++ b/patch-id.c
@@ -21,7 +21,7 @@ static int remove_space(char *line)
 	unsigned char c;
 
 	while ((c = *src++) != '\0') {
-		if (!isspace(c))
+		if (!is_space(c))
 			*dst++ = c;
 	}
 	return dst - line;
diff --git a/refs.c b/refs.c
index 97506a4..4d202ee 100644
--- a/refs.c
+++ b/refs.c
@@ -43,7 +43,7 @@ int validate_symref(const char *path)
 		return -1;
 	buf = buffer + 4;
 	len -= 4;
-	while (len && isspace(*buf))
+	while (len && is_space(*buf))
 		buf++, len--;
 	if (len >= 5 && !memcmp("refs/", buf, 5))
 		return 0;
@@ -103,9 +103,9 @@ const char *resolve_ref(const char *path
 			break;
 		buf = buffer + 4;
 		len -= 4;
-		while (len && isspace(*buf))
+		while (len && is_space(*buf))
 			buf++, len--;
-		while (len && isspace(buf[len-1]))
+		while (len && is_space(buf[len-1]))
 			buf[--len] = 0;
 		path = git_path("%.*s", len, buf);
 	}
diff --git a/stripspace.c b/stripspace.c
index 96cd0a8..a6204b6 100644
--- a/stripspace.c
+++ b/stripspace.c
@@ -1,6 +1,6 @@
 #include <stdio.h>
 #include <string.h>
-#include <ctype.h>
+#include "cache.h"
 
 /*
  * Remove empty lines from the beginning and end.
@@ -15,7 +15,7 @@ static void cleanup(char *line)
 	if (len > 1 && line[len-1] == '\n') {
 		do {
 			unsigned char c = line[len-2];
-			if (!isspace(c))
+			if (!is_space(c))
 				break;
 			line[len-2] = '\n';
 			len--;

Re: daemon.c broken on OpenBSD

[Matthias Urlichs]

Hi, Junio C Hamano wrote:

> Maybe something like this would help?

I agree that this change, though intrusive, is a good idea.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
The thing I like best about Spider-Man is that there's a hyphen in the
title. -- SJM's review of the movie

Re: daemon.c broken on OpenBSD

[Linus Torvalds]

On Sun, 23 Oct 2005, Randal L. Schwartz wrote:
> 
> If that rings a bell, help me out here.  I'm guessing "isalnum" is getting
> defined (wrongly).  Yeah, looks like in cache.h.  Why is this getting
> defined?

We're doing our own ctype.h.

The fix is to make sure that "cache.h" is included _after_ system 
includes.

iow, this should fix it, methinks.

		Linus
---
diff --git a/daemon.c b/daemon.c
index 0c6182f..6e5de11 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,5 +1,3 @@
-#include "cache.h"
-#include "pkt-line.h"
 #include <signal.h>
 #include <sys/wait.h>
 #include <sys/socket.h>
@@ -10,6 +8,9 @@
 #include <arpa/inet.h>
 #include <syslog.h>
 
+#include "cache.h"
+#include "pkt-line.h"
+
 static int log_syslog;
 static int verbose;
 

Re: daemon.c broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Linus" == Linus Torvalds <torvalds@osdl.org> writes:

Linus> We're doing our own ctype.h.

Linus> The fix is to make sure that "cache.h" is included _after_ system 
Linus> includes.

That probably won't work, because on OpenBSD, it's not a #define, but
rather a real function call.  You can't just #undef that (unless my C
is even more rusty).

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: daemon.c broken on OpenBSD

[Linus Torvalds]

On Mon, 24 Oct 2005, Randal L. Schwartz wrote:
> 
> That probably won't work, because on OpenBSD, it's not a #define, but
> rather a real function call.  You can't just #undef that (unless my C
> is even more rusty).

Sure you can. 

The #undef won't necessarily _do_ anything, but it protects against 
systems where it is a #define. Then, the #define makes sure that if it's 
anything else, the #define will take precedence.

So yes, it's ugly, but it definitely is safe in practice, as long as the 
local headers are included before the system ones.

I think Junio's patch (that renames things) is possibly the "cleaner" 
alternative, but on the other hand there's a lot of advantages to just 
using the standard names. No chance of somebody using the wrong version by 
mistake.

			Linus

Re: daemon.c broken on OpenBSD

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> I think Junio's patch (that renames things) is possibly the "cleaner" 
> alternative, but on the other hand there's a lot of advantages to just 
> using the standard names. No chance of somebody using the wrong version by 
> mistake.
> 

But there is also no risk of other unexpected dependencies.  I would 
vote for using git-specific names.

	-hpa

Re: daemon.c broken on OpenBSD

[Linus Torvalds]

On Mon, 24 Oct 2005, Linus Torvalds wrote:
> 
> So yes, it's ugly, but it definitely is safe in practice, as long as the 
> local headers are included before the system ones.

Side note: "safe in practice" isn't necessarily the same as "safe in 
theory".

I think that strictly speaking, if you include <ctype.h>, the "isxxxx()" 
format is always reserved (where "x" is lowercase letters), and in theory 
you could have a compiler that recognizes them even before the 
pre-processor as being something built-in.

In _practice_, such a compiler would be incredibly broken (it would have 
to do the recognition after preprocessing too), and I doubt you'd ever see 
such a thing, but from a pure language-lawyer standpoint it might 
technically be possible.

However, I'm surprised that you see <ctype.h> at all. The OpenBSD headers 
seem to be including <ctype.h> from <sys/poll.h>, which doesn't make any 
sense. It may not be strictly against POSIX, but it's definitely strange 
and bad form to do unnecessary name pollution that the user didn't ask 
for.

So git actually tries to be pretty good about things. Not only does it try 
to handle any existing system <ctype.h> by doing the #undef, it actually 
doesn't include the system ctype.h at _all_ when it includes "cache.h". 
And OpenBSD is being a bit strange.

		Linus

Re: daemon.c broken on OpenBSD

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Side note: "safe in practice" isn't necessarily the same as "safe in 
> theory".
> 
> I think that strictly speaking, if you include <ctype.h>, the "isxxxx()" 
> format is always reserved (where "x" is lowercase letters), and in theory 
> you could have a compiler that recognizes them even before the 
> pre-processor as being something built-in.
> 
> In _practice_, such a compiler would be incredibly broken (it would have 
> to do the recognition after preprocessing too), and I doubt you'd ever see 
> such a thing, but from a pure language-lawyer standpoint it might 
> technically be possible.
> 

A much more likely cause for problems would be system headers that use 
the isxxx() macros as part of other macros.  That would be in violation 
of POSIX, but is reasonably common.

> However, I'm surprised that you see <ctype.h> at all. The OpenBSD headers 
> seem to be including <ctype.h> from <sys/poll.h>, which doesn't make any 
> sense. It may not be strictly against POSIX, but it's definitely strange 
> and bad form to do unnecessary name pollution that the user didn't ask 
> for.

It is strictly against POSIX, but again, fairly common.

> So git actually tries to be pretty good about things. Not only does it try 
> to handle any existing system <ctype.h> by doing the #undef, it actually 
> doesn't include the system ctype.h at _all_ when it includes "cache.h". 
> And OpenBSD is being a bit strange.

I think there is another good reason for doing the renaming bit: we're 
creating new macros with different semantics; in particular, the git 
macros handle signed char input and don't guarantee anything w.r.t. EOF.

	-hpa

Re: daemon.c broken on OpenBSD

[Linus Torvalds]

On Mon, 24 Oct 2005, H. Peter Anvin wrote:
> 
> A much more likely cause for problems would be system headers that use the
> isxxx() macros as part of other macros.  That would be in violation of POSIX,
> but is reasonably common.

Hmm.. Yes, that's a good point. I don't see what thing could do it that we 
care about (and since we do actually do a pretty valid ctype 
implementation, it should all work out right regardless), but yes, it 
might be safer to rename the things.

Especially if we then continue to make sure _not_ to include <ctype.h> 
ourselves, so that only systems with broken headers (like the ones on 
OpenBSD ;) could ever use the standard names anyway. That way we'd not 
have to worry whether somebody uses them by mistake..

Btw, nobody ever replied to my question whether we might be using 
something else that is locale-specific. I'm not actually locate-aware 
enough to even know what else than <ctype.h> might be dangerous to use..

Anybody?

		Linus

Re: daemon.c broken on OpenBSD

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Btw, nobody ever replied to my question whether we might be using 
> something else that is locale-specific. I'm not actually locate-aware 
> enough to even know what else than <ctype.h> might be dangerous to use..
> 

Regexxen are probably the single most problematic thing.

	-hpa

Re: daemon.c broken on OpenBSD

[Morten Welinder]

Locale issues:

* Environment strings have unspecified encoding.
* File names ditto.  (Which matters when they get printed together with other
  strings.)
* gecos information ditto.

If, and if only, you[*] call setlocale:

* ctype stuff.
* strtod, printf %[efg]
* Regexps

(Integer parsing and printing seems to be safe in pratice if not in theory.)

If you call system() you also need to watch out for things like Gnu sort
being locale sensitive.

Morten


[*] That would be not just you, but also all the yous behind all the
     libraries you link.

Re: daemon.c broken on OpenBSD

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> I think there is another good reason for doing the renaming bit: we're 
> creating new macros with different semantics; in particular, the git 
> macros handle signed char input and don't guarantee anything w.r.t. EOF.
>
> 	-hpa

Thanks.  I'll include the renaming bits to "master" branch when
I hear somebody on OpenBSD confirms that the patch fixes it.

Re: daemon.c broken on OpenBSD

[Randal L. Schwartz]

I can confirm that the following patch lets the current origin
compile on OpenBSD.  If you could apply this until you sort out the
rest of the namespace issue, I would be happy.  Thanks.

diff --git a/daemon.c b/daemon.c
index 0c6182f..c3f8641 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,5 +1,3 @@
-#include "cache.h"
-#include "pkt-line.h"
 #include <signal.h>
 #include <sys/wait.h>
 #include <sys/socket.h>
@@ -9,6 +7,8 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <syslog.h>
+#include "pkt-line.h"
+#include "cache.h"
 
 static int log_syslog;
 static int verbose;


-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: daemon.c broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Randal" == Randal L Schwartz <merlyn@stonehenge.com> writes:

Randal> I can confirm that the following patch lets the current origin
Randal> compile on OpenBSD.  If you could apply this until you sort out the
Randal> rest of the namespace issue, I would be happy.  Thanks.

Actually, you probably want this instead:

Subject: [PATCH] openbsd patch

---

 daemon.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

applies-to: 6234aa4f7095054a137e030030f914dc6633f809
7666c6f2cf2fcf41fce1d0665b79e5cc2f7b73b5
diff --git a/daemon.c b/daemon.c
index 0c6182f..c3f8641 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,5 +1,3 @@
-#include "cache.h"
-#include "pkt-line.h"
 #include <signal.h>
 #include <sys/wait.h>
 #include <sys/socket.h>
@@ -9,6 +7,8 @@
 #include <netinet/in.h>
 #include <arpa/inet.h>
 #include <syslog.h>
+#include "pkt-line.h"
+#include "cache.h"
 
 static int log_syslog;
 static int verbose;
---
0.99.8.GIT


-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: daemon.c broken on OpenBSD

[Randal L. Schwartz]

>>>>> "Randal" == Randal L Schwartz <merlyn@stonehenge.com> writes:

Randal> Actually, you probably want this instead:

And this one too, because .gitignore is missing some files:

>From nobody Mon Sep 17 00:00:00 2001
Subject: [PATCH] fix .gitignore to add new progs
From: Charlie <root@blue.stonehenge.com>
Date: 1130283408 -0700

---

 .gitignore |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

applies-to: eba31d473719d9a1f2d00b40b3011460d7af864c
c0964c0160d7b08c0b5dd28d1b9462fd2b01bd50
diff --git a/.gitignore b/.gitignore
index 52cb9e2..758aaf6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -54,6 +54,7 @@ git-merge-recursive
 git-merge-resolve
 git-merge-stupid
 git-mktag
+git-mv
 git-octopus
 git-pack-objects
 git-parse-remote
@@ -78,6 +79,7 @@ git-revert
 git-send-email
 git-send-pack
 git-sh-setup
+git-shell
 git-shortlog
 git-show-branch
 git-show-index
---
0.99.8.GIT


-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!