From 1d8cd418b49f08146d6d47cf8881965f922686cb Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 6 Mar 2010 16:40:38 +0100 Subject: [PATCH 1/8] Modernize t5530-upload-pack-error. Some tests did not use test_must_fail. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- t/t5530-upload-pack-error.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh index a696b8791b..044603c26e 100755 --- a/t/t5530-upload-pack-error.sh +++ b/t/t5530-upload-pack-error.sh @@ -32,9 +32,9 @@ test_expect_success 'fsck fails' ' test_expect_success 'upload-pack fails due to error in pack-objects packing' ' - ! echo "0032want $(git rev-parse HEAD) -00000009done -0000" | git upload-pack . > /dev/null 2> output.err && + printf "0032want %s\n00000009done\n0000" \ + $(git rev-parse HEAD) >input && + test_must_fail git upload-pack . /dev/null 2>output.err && grep "unable to read" output.err && grep "pack-objects died" output.err ' @@ -51,9 +51,9 @@ test_expect_success 'fsck fails' ' ' test_expect_success 'upload-pack fails due to error in rev-list' ' - ! echo "0032want $(git rev-parse HEAD) -0034shallow $(git rev-parse HEAD^)00000009done -0000" | git upload-pack . > /dev/null 2> output.err && + printf "0032want %s\n0034shallow %s00000009done\n0000" \ + $(git rev-parse HEAD) $(git rev-parse HEAD^) >input && + test_must_fail git upload-pack . /dev/null 2>output.err && # pack-objects survived grep "Total.*, reused" output.err && # but there was an error, which must have been in rev-list @@ -62,9 +62,9 @@ test_expect_success 'upload-pack fails due to error in rev-list' ' test_expect_success 'upload-pack fails due to error in pack-objects enumeration' ' - ! echo "0032want $(git rev-parse HEAD) -00000009done -0000" | git upload-pack . > /dev/null 2> output.err && + printf "0032want %s\n00000009done\n0000" \ + $(git rev-parse HEAD) >input && + test_must_fail git upload-pack . /dev/null 2>output.err && grep "bad tree object" output.err && grep "pack-objects died" output.err ' From ebaa79f462c48e0ed0341d9c8f9c97de557afcfd Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 6 Mar 2010 16:40:39 +0100 Subject: [PATCH 2/8] Make report() from usage.c public as vreportf() and use it. There exist already a number of static functions named 'report', therefore, the function name was changed. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- fast-import.c | 8 +++----- git-compat-util.h | 1 + http-backend.c | 5 +---- usage.c | 10 +++++----- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/fast-import.c b/fast-import.c index 309f2c58a2..f2ef20cbf0 100644 --- a/fast-import.c +++ b/fast-import.c @@ -483,14 +483,12 @@ static void dump_marks(void); static NORETURN void die_nicely(const char *err, va_list params) { static int zombie; - char message[2 * PATH_MAX]; - vsnprintf(message, sizeof(message), err, params); - fputs("fatal: ", stderr); - fputs(message, stderr); - fputc('\n', stderr); + vreportf("fatal: ", err, params); if (!zombie) { + char message[2 * PATH_MAX]; + zombie = 1; write_crash_report(message); end_packfile(); diff --git a/git-compat-util.h b/git-compat-util.h index a3c4537366..3cabcdd8c4 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -192,6 +192,7 @@ extern char *gitbasename(char *); #include "compat/bswap.h" /* General helper functions */ +extern void vreportf(const char *prefix, const char *err, va_list params); extern NORETURN void usage(const char *err); extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))); extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2))); diff --git a/http-backend.c b/http-backend.c index 345c12b790..8c7b7d09ea 100644 --- a/http-backend.c +++ b/http-backend.c @@ -538,14 +538,11 @@ static void service_rpc(char *service_name) static NORETURN void die_webcgi(const char *err, va_list params) { - char buffer[1000]; - http_status(500, "Internal Server Error"); hdr_nocache(); end_headers(); - vsnprintf(buffer, sizeof(buffer), err, params); - fprintf(stderr, "fatal: %s\n", buffer); + vreportf("fatal: ", err, params); exit(0); } diff --git a/usage.c b/usage.c index 79856a2b9f..ec4cf53b6b 100644 --- a/usage.c +++ b/usage.c @@ -5,7 +5,7 @@ */ #include "git-compat-util.h" -static void report(const char *prefix, const char *err, va_list params) +void vreportf(const char *prefix, const char *err, va_list params) { char msg[4096]; vsnprintf(msg, sizeof(msg), err, params); @@ -14,24 +14,24 @@ static void report(const char *prefix, const char *err, va_list params) static NORETURN void usage_builtin(const char *err, va_list params) { - report("usage: ", err, params); + vreportf("usage: ", err, params); exit(129); } static NORETURN void die_builtin(const char *err, va_list params) { - report("fatal: ", err, params); + vreportf("fatal: ", err, params); exit(128); } static void error_builtin(const char *err, va_list params) { - report("error: ", err, params); + vreportf("error: ", err, params); } static void warn_builtin(const char *warn, va_list params) { - report("warning: ", warn, params); + vreportf("warning: ", warn, params); } /* If we are in a dlopen()ed .so write to a global variable would segfault From 5f8763a81b96b94f24af6aa728107997adcf60bd Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 6 Mar 2010 16:40:40 +0100 Subject: [PATCH 3/8] Fix signature of fcntl() compatibility dummy Obviously, this function was never called with two arguments in Windows code sections, but this will be the case in a subsequent patch. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- compat/mingw.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/mingw.h b/compat/mingw.h index e81e752ed2..3347362632 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -89,7 +89,7 @@ static inline int getuid() { return 1; } static inline struct passwd *getpwnam(const char *name) { return NULL; } -static inline int fcntl(int fd, int cmd, long arg) +static inline int fcntl(int fd, int cmd, ...) { if (cmd == F_GETFD || cmd == F_SETFD) return 0; From 912b26324dbc1eb9500e49c90d271a330cbcb30b Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 6 Mar 2010 16:40:41 +0100 Subject: [PATCH 4/8] Windows: more pthreads functions This adds: pthread_self pthread_equal pthread_exit pthread_key_create pthread_setspecific pthread_getspecific Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- compat/win32/pthread.c | 8 ++++++++ compat/win32/pthread.h | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c index 0f949fc425..010e875ec4 100644 --- a/compat/win32/pthread.c +++ b/compat/win32/pthread.c @@ -16,6 +16,7 @@ static unsigned __stdcall win32_start_routine(void *arg) { pthread_t *thread = arg; + thread->tid = GetCurrentThreadId(); thread->arg = thread->start_routine(thread->arg); return 0; } @@ -49,6 +50,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr) } } +pthread_t pthread_self(void) +{ + pthread_t t = { 0 }; + t.tid = GetCurrentThreadId(); + return t; +} + int pthread_cond_init(pthread_cond_t *cond, const void *unused) { cond->waiters = 0; diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h index c72f100f40..c7b8241b79 100644 --- a/compat/win32/pthread.h +++ b/compat/win32/pthread.h @@ -52,6 +52,7 @@ typedef struct { HANDLE handle; void *(*start_routine)(void*); void *arg; + DWORD tid; } pthread_t; extern int pthread_create(pthread_t *thread, const void *unused, @@ -65,4 +66,28 @@ extern int pthread_create(pthread_t *thread, const void *unused, extern int win32_pthread_join(pthread_t *thread, void **value_ptr); +#define pthread_equal(t1, t2) ((t1).tid == (t2).tid) +extern pthread_t pthread_self(void); + +static inline int pthread_exit(void *ret) +{ + ExitThread((DWORD)ret); +} + +typedef DWORD pthread_key_t; +static inline int pthread_key_create(pthread_key_t *keyp, void (*destructor)(void *value)) +{ + return (*keyp = TlsAlloc()) == TLS_OUT_OF_INDEXES ? EAGAIN : 0; +} + +static inline int pthread_setspecific(pthread_key_t key, const void *value) +{ + return TlsSetValue(key, (void *)value) ? 0 : EINVAL; +} + +static inline void *pthread_getspecific(pthread_key_t key) +{ + return TlsGetValue(key); +} + #endif /* PTHREAD_H */ From 200a76b74db5c2c75bcf73773cb85c5603ec038e Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 6 Mar 2010 16:40:42 +0100 Subject: [PATCH 5/8] Reimplement async procedures using pthreads On Windows, async procedures have always been run in threads, and the implementation used Windows specific APIs. Rewrite the code to use pthreads. A new configuration option is introduced so that the threaded implementation can also be used on POSIX systems. Since this option is intended only as playground on POSIX, but is mandatory on Windows, the option is not documented. One detail is that on POSIX it is necessary to set FD_CLOEXEC on the pipe handles. On Windows, this is not needed because pipe handles are not inherited to child processes, and the new calls to set_cloexec() are effectively no-ops. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- Makefile | 5 +++++ run-command.c | 41 +++++++++++++++++++++++------------------ run-command.h | 8 ++++++-- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/Makefile b/Makefile index 52f2cc040b..2fe52f8163 100644 --- a/Makefile +++ b/Makefile @@ -979,6 +979,7 @@ ifeq ($(uname_S),Windows) NO_CURL = YesPlease NO_PYTHON = YesPlease BLK_SHA1 = YesPlease + ASYNC_AS_THREAD = YesPlease CC = compat/vcbuild/scripts/clink.pl AR = compat/vcbuild/scripts/lib.pl @@ -1030,6 +1031,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_REGEX = YesPlease NO_PYTHON = YesPlease BLK_SHA1 = YesPlease + ASYNC_AS_THREAD = YesPlease COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \ @@ -1342,6 +1344,9 @@ ifdef NO_PTHREADS else EXTLIBS += $(PTHREAD_LIBS) LIB_OBJS += thread-utils.o +ifdef ASYNC_AS_THREAD + BASIC_CFLAGS += -DASYNC_AS_THREAD +endif endif ifdef DIR_HAS_BSD_GROUP_SEMANTICS diff --git a/run-command.c b/run-command.c index 0cd7f02ffe..77aefff2e2 100644 --- a/run-command.c +++ b/run-command.c @@ -82,6 +82,7 @@ static NORETURN void die_child(const char *err, va_list params) write(child_err, "\n", 1); exit(128); } +#endif static inline void set_cloexec(int fd) { @@ -89,7 +90,6 @@ static inline void set_cloexec(int fd) if (flags >= 0) fcntl(fd, F_SETFD, flags | FD_CLOEXEC); } -#endif static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) { @@ -447,11 +447,12 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const return run_command(&cmd); } -#ifdef WIN32 -static unsigned __stdcall run_thread(void *data) +#ifdef ASYNC_AS_THREAD +static void *run_thread(void *data) { struct async *async = data; - return async->proc(async->proc_in, async->proc_out, async->data); + intptr_t ret = async->proc(async->proc_in, async->proc_out, async->data); + return (void *)ret; } #endif @@ -497,7 +498,7 @@ int start_async(struct async *async) else proc_out = -1; -#ifndef WIN32 +#ifndef ASYNC_AS_THREAD /* Flush stdio before fork() to avoid cloning buffers */ fflush(NULL); @@ -524,12 +525,18 @@ int start_async(struct async *async) else if (async->out) close(async->out); #else + if (proc_in >= 0) + set_cloexec(proc_in); + if (proc_out >= 0) + set_cloexec(proc_out); async->proc_in = proc_in; async->proc_out = proc_out; - async->tid = (HANDLE) _beginthreadex(NULL, 0, run_thread, async, 0, NULL); - if (!async->tid) { - error("cannot create thread: %s", strerror(errno)); - goto error; + { + int err = pthread_create(&async->tid, NULL, run_thread, async); + if (err) { + error("cannot create thread: %s", strerror(err)); + goto error; + } } #endif return 0; @@ -549,17 +556,15 @@ error: int finish_async(struct async *async) { -#ifndef WIN32 - int ret = wait_or_whine(async->pid, "child process", 0); +#ifndef ASYNC_AS_THREAD + return wait_or_whine(async->pid, "child process", 0); #else - DWORD ret = 0; - if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0) - ret = error("waiting for thread failed: %lu", GetLastError()); - else if (!GetExitCodeThread(async->tid, &ret)) - ret = error("cannot get thread exit code: %lu", GetLastError()); - CloseHandle(async->tid); + void *ret = (void *)(intptr_t)(-1); + + if (pthread_join(async->tid, &ret)) + error("pthread_join failed"); + return (int)(intptr_t)ret; #endif - return ret; } int run_hook(const char *index_file, const char *name, ...) diff --git a/run-command.h b/run-command.h index 94619f52d9..40db39cec0 100644 --- a/run-command.h +++ b/run-command.h @@ -1,6 +1,10 @@ #ifndef RUN_COMMAND_H #define RUN_COMMAND_H +#ifdef ASYNC_AS_THREAD +#include +#endif + struct child_process { const char **argv; pid_t pid; @@ -74,10 +78,10 @@ struct async { void *data; int in; /* caller writes here and closes it */ int out; /* caller reads from here and closes it */ -#ifndef WIN32 +#ifndef ASYNC_AS_THREAD pid_t pid; #else - HANDLE tid; + pthread_t tid; int proc_in; int proc_out; #endif From 0ea1c89ba616397ef7e5f6f601ef7a24d2c27b8e Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 6 Mar 2010 16:40:43 +0100 Subject: [PATCH 6/8] Dying in an async procedure should only exit the thread, not the process. Async procedures are intended as helpers that perform a very restricted task, and the caller usually has to manage them in a larger context. Conceptually, the async procedure is not concerned with the "bigger picture" in whose context it is run. When it dies, it is not supposed to destroy this "bigger picture", but rather only its own limit view of the world. On POSIX, the async procedure is run in its own process, and exiting this process naturally had only these limited effects. On Windows (or when ASYNC_AS_THREAD is set), calling die() exited the whole process, destroying the caller (the "big picture") as well. This fixes it to exit only the thread. Without ASYNC_AS_THREAD, one particular effect of exiting the async procedure process is that it automatically closes file descriptors, most notably the writable end of the pipe that the async procedure writes to. The async API already requires that the async procedure closes the pipe ends when it exits normally. But for calls to die() no requirements are imposed. In the non-threaded case the pipe ends are closed implicitly by the exiting process, but in the threaded case, the die routine must take care of closing them. Now t5530-upload-pack-error.sh passes on Windows. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- run-command.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/run-command.c b/run-command.c index 77aefff2e2..66cc4bfa57 100644 --- a/run-command.c +++ b/run-command.c @@ -448,12 +448,35 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const } #ifdef ASYNC_AS_THREAD +static pthread_t main_thread; +static int main_thread_set; +static pthread_key_t async_key; + static void *run_thread(void *data) { struct async *async = data; + + pthread_setspecific(async_key, async); + intptr_t ret = async->proc(async->proc_in, async->proc_out, async->data); return (void *)ret; } + +static NORETURN void die_async(const char *err, va_list params) +{ + vreportf("fatal: ", err, params); + + if (!pthread_equal(main_thread, pthread_self())) { + struct async *async = pthread_getspecific(async_key); + if (async->proc_in >= 0) + close(async->proc_in); + if (async->proc_out >= 0) + close(async->proc_out); + pthread_exit((void *)128); + } + + exit(128); +} #endif int start_async(struct async *async) @@ -525,6 +548,17 @@ int start_async(struct async *async) else if (async->out) close(async->out); #else + if (!main_thread_set) { + /* + * We assume that the first time that start_async is called + * it is from the main thread. + */ + main_thread_set = 1; + main_thread = pthread_self(); + pthread_key_create(&async_key, NULL); + set_die_routine(die_async); + } + if (proc_in >= 0) set_cloexec(proc_in); if (proc_out >= 0) From f6b6098316192475ff0b3fa2ba894d7e555bdfac Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Tue, 9 Mar 2010 21:00:36 +0100 Subject: [PATCH 7/8] Enable threaded async procedures whenever pthreads is available Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- Documentation/technical/api-run-command.txt | 5 +++-- Makefile | 5 ----- run-command.c | 10 +++++----- run-command.h | 4 ++-- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt index 44876fa703..f18b4f4817 100644 --- a/Documentation/technical/api-run-command.txt +++ b/Documentation/technical/api-run-command.txt @@ -231,8 +231,9 @@ The function pointer in .proc has the following signature: There are serious restrictions on what the asynchronous function can do -because this facility is implemented by a pipe to a forked process on -UNIX, but by a thread in the same address space on Windows: +because this facility is implemented by a thread in the same address +space on most platforms (when pthreads is available), but by a pipe to +a forked process otherwise: . It cannot change the program's state (global variables, environment, etc.) in a way that the caller notices; in other words, .in and .out diff --git a/Makefile b/Makefile index 2fe52f8163..52f2cc040b 100644 --- a/Makefile +++ b/Makefile @@ -979,7 +979,6 @@ ifeq ($(uname_S),Windows) NO_CURL = YesPlease NO_PYTHON = YesPlease BLK_SHA1 = YesPlease - ASYNC_AS_THREAD = YesPlease CC = compat/vcbuild/scripts/clink.pl AR = compat/vcbuild/scripts/lib.pl @@ -1031,7 +1030,6 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_REGEX = YesPlease NO_PYTHON = YesPlease BLK_SHA1 = YesPlease - ASYNC_AS_THREAD = YesPlease COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \ @@ -1344,9 +1342,6 @@ ifdef NO_PTHREADS else EXTLIBS += $(PTHREAD_LIBS) LIB_OBJS += thread-utils.o -ifdef ASYNC_AS_THREAD - BASIC_CFLAGS += -DASYNC_AS_THREAD -endif endif ifdef DIR_HAS_BSD_GROUP_SEMANTICS diff --git a/run-command.c b/run-command.c index 66cc4bfa57..61b153987b 100644 --- a/run-command.c +++ b/run-command.c @@ -447,7 +447,7 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const return run_command(&cmd); } -#ifdef ASYNC_AS_THREAD +#ifndef NO_PTHREADS static pthread_t main_thread; static int main_thread_set; static pthread_key_t async_key; @@ -455,10 +455,10 @@ static pthread_key_t async_key; static void *run_thread(void *data) { struct async *async = data; + intptr_t ret; pthread_setspecific(async_key, async); - - intptr_t ret = async->proc(async->proc_in, async->proc_out, async->data); + ret = async->proc(async->proc_in, async->proc_out, async->data); return (void *)ret; } @@ -521,7 +521,7 @@ int start_async(struct async *async) else proc_out = -1; -#ifndef ASYNC_AS_THREAD +#ifdef NO_PTHREADS /* Flush stdio before fork() to avoid cloning buffers */ fflush(NULL); @@ -590,7 +590,7 @@ error: int finish_async(struct async *async) { -#ifndef ASYNC_AS_THREAD +#ifdef NO_PTHREADS return wait_or_whine(async->pid, "child process", 0); #else void *ret = (void *)(intptr_t)(-1); diff --git a/run-command.h b/run-command.h index 40db39cec0..56491b9f23 100644 --- a/run-command.h +++ b/run-command.h @@ -1,7 +1,7 @@ #ifndef RUN_COMMAND_H #define RUN_COMMAND_H -#ifdef ASYNC_AS_THREAD +#ifndef NO_PTHREADS #include #endif @@ -78,7 +78,7 @@ struct async { void *data; int in; /* caller writes here and closes it */ int out; /* caller reads from here and closes it */ -#ifndef ASYNC_AS_THREAD +#ifdef NO_PTHREADS pid_t pid; #else pthread_t tid; From 3e333036ccbb97fddf54bd8fe74b12ba46f1687b Mon Sep 17 00:00:00 2001 From: Thomas Rast Date: Fri, 11 Jun 2010 17:02:50 +0200 Subject: [PATCH 8/8] fast-import: die_nicely() back to vsnprintf (reverts part of ebaa79f) ebaa79f (Make report() from usage.c public as vreportf() and use it., 2010-03-06) changed fast-import's die_nicely() to use vreportf(). Unfortunately this is not possible: we need the message again for write_report(), and vreportf() uses vsnprintf(), which invalidates the va_list. As pointed out by Erik Faye-Lund, va_copy is C99 and thus not an option. So revert the part of ebaa79f that pertains to die_nicely(). Signed-off-by: Thomas Rast Signed-off-by: Junio C Hamano --- fast-import.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/fast-import.c b/fast-import.c index f2ef20cbf0..309f2c58a2 100644 --- a/fast-import.c +++ b/fast-import.c @@ -483,12 +483,14 @@ static void dump_marks(void); static NORETURN void die_nicely(const char *err, va_list params) { static int zombie; + char message[2 * PATH_MAX]; - vreportf("fatal: ", err, params); + vsnprintf(message, sizeof(message), err, params); + fputs("fatal: ", stderr); + fputs(message, stderr); + fputc('\n', stderr); if (!zombie) { - char message[2 * PATH_MAX]; - zombie = 1; write_crash_report(message); end_packfile();