From fb7a6531e67333b22967bf5b96ef22a28f3b2552 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sun, 2 Apr 2006 13:28:27 -0700 Subject: [PATCH 1/4] Fix Solaris stdio signal handling stupidities This uses sigaction() to install the SIGALRM handler with SA_RESTART, so that Solaris stdio doesn't break completely when a signal interrupts a read. Thanks to Jason Riedy for confirming the silly Solaris signal behaviour. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- pack-objects.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/pack-objects.c b/pack-objects.c index 8f352aa6c1..cde4afa798 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -53,7 +53,7 @@ static int nr_objects = 0, nr_alloc = 0; static const char *base_name; static unsigned char pack_file_sha1[20]; static int progress = 1; -static volatile int progress_update = 0; +static volatile sig_atomic_t progress_update = 0; /* * The object names in objects array are hashed with this hashtable, @@ -685,7 +685,6 @@ static int try_delta(struct unpacked *cur, struct unpacked *old, unsigned max_de static void progress_interval(int signum) { - signal(SIGALRM, progress_interval); progress_update = 1; } @@ -820,6 +819,23 @@ static int reuse_cached_pack(unsigned char *sha1, int pack_to_stdout) return 1; } +static void setup_progress_signal(void) +{ + struct sigaction sa; + struct itimerval v; + + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = progress_interval; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESTART; + sigaction(SIGALRM, &sa, NULL); + + v.it_interval.tv_sec = 1; + v.it_interval.tv_usec = 0; + v.it_value = v.it_interval; + setitimer(ITIMER_REAL, &v, NULL); +} + int main(int argc, char **argv) { SHA_CTX ctx; @@ -885,13 +901,8 @@ int main(int argc, char **argv) prepare_packed_git(); if (progress) { - struct itimerval v; - v.it_interval.tv_sec = 1; - v.it_interval.tv_usec = 0; - v.it_value = v.it_interval; - signal(SIGALRM, progress_interval); - setitimer(ITIMER_REAL, &v, NULL); fprintf(stderr, "Generating pack...\n"); + setup_progress_signal(); } while (fgets(line, sizeof(line), stdin) != NULL) { From da93d12b00425a37e81e227671f13130efcfe93f Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sun, 2 Apr 2006 13:31:54 -0700 Subject: [PATCH 2/4] pack-objects: be incredibly anal about stdio semantics This is the "letter of the law" version of using fgets() properly in the face of incredibly broken stdio implementations. We can work around the Solaris breakage with SA_RESTART, but in case anybody else is ever that stupid, here's the "safe" (read: "insanely anal") way to use fgets. It probably goes without saying that I'm not terribly impressed by Solaris libc. Signed-off-by: Linus Torvalds Signed-off-by: Junio C Hamano --- pack-objects.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/pack-objects.c b/pack-objects.c index cde4afa798..084c2006a9 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -905,11 +905,21 @@ int main(int argc, char **argv) setup_progress_signal(); } - while (fgets(line, sizeof(line), stdin) != NULL) { + for (;;) { unsigned int hash; char *p; unsigned char sha1[20]; + if (!fgets(line, sizeof(line), stdin)) { + if (feof(stdin)) + break; + if (!ferror(stdin)) + die("fgets returned NULL, not EOF, not error!"); + if (errno == EINTR) + continue; + die("fgets: %s", strerror(errno)); + } + if (progress_update) { fprintf(stderr, "Counting objects...%d\r", nr_objects); progress_update = 0; From 687dd75c95f9212244b6cf4fe60b40db44de01ba Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 3 Apr 2006 23:41:09 -0700 Subject: [PATCH 3/4] safe_fgets() - even more anal fgets() This is from Linus -- the previous round forgot to clear error after EINTR case. Signed-off-by: Junio C Hamano --- pack-objects.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pack-objects.c b/pack-objects.c index 084c2006a9..7d6247791d 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -915,9 +915,10 @@ int main(int argc, char **argv) break; if (!ferror(stdin)) die("fgets returned NULL, not EOF, not error!"); - if (errno == EINTR) - continue; - die("fgets: %s", strerror(errno)); + if (errno != EINTR) + die("fgets: %s", strerror(errno)); + clearerr(stdin); + continue; } if (progress_update) { From 72fdfb50f721460e4cdff16fbe9c72d4ce6c668c Mon Sep 17 00:00:00 2001 From: Jason Riedy Date: Sun, 2 Apr 2006 15:29:34 -0700 Subject: [PATCH 4/4] Use sigaction and SA_RESTART in read-tree.c; add option in Makefile. Might as well ape the sigaction change in read-tree.c to avoid the same potential problems. The fprintf status output will be overwritten in a second, so don't bother guarding it. Do move the fputc after disabling SIGALRM to ensure we go to the next line, though. Also add a NO_SA_RESTART option in the Makefile in case someone doesn't have SA_RESTART but does restart (maybe older HP/UX?). We want the builder to chose this specifically in case the system both lacks SA_RESTART and does not restart stdio calls; a compat #define in git-compat-utils.h would silently allow broken systems. Signed-off-by: Jason Riedy Signed-off-by: Junio C Hamano --- read-tree.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/read-tree.c b/read-tree.c index da0fcf035e..32fb6faa1c 100644 --- a/read-tree.c +++ b/read-tree.c @@ -273,10 +273,26 @@ static void unlink_entry(char *name) static void progress_interval(int signum) { - signal(SIGALRM, progress_interval); progress_update = 1; } +static void setup_progress_signal(void) +{ + struct sigaction sa; + struct itimerval v; + + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = progress_interval; + sigemptyset(&sa.sa_mask); + sa.sa_flags = SA_RESTART; + sigaction(SIGALRM, &sa, NULL); + + v.it_interval.tv_sec = 1; + v.it_interval.tv_usec = 0; + v.it_value = v.it_interval; + setitimer(ITIMER_REAL, &v, NULL); +} + static void check_updates(struct cache_entry **src, int nr) { static struct checkout state = { @@ -289,8 +305,6 @@ static void check_updates(struct cache_entry **src, int nr) unsigned last_percent = 200, cnt = 0, total = 0; if (update && verbose_update) { - struct itimerval v; - for (total = cnt = 0; cnt < nr; cnt++) { struct cache_entry *ce = src[cnt]; if (!ce->ce_mode || ce->ce_flags & mask) @@ -302,12 +316,8 @@ static void check_updates(struct cache_entry **src, int nr) total = 0; if (total) { - v.it_interval.tv_sec = 1; - v.it_interval.tv_usec = 0; - v.it_value = v.it_interval; - signal(SIGALRM, progress_interval); - setitimer(ITIMER_REAL, &v, NULL); fprintf(stderr, "Checking files out...\n"); + setup_progress_signal(); progress_update = 1; } cnt = 0; @@ -341,8 +351,8 @@ static void check_updates(struct cache_entry **src, int nr) } } if (total) { - fputc('\n', stderr); signal(SIGALRM, SIG_IGN); + fputc('\n', stderr); } }