{"thread":{"id":"3773","subject":"Solaris cloning woes partly diagnosed","startedAt":"2006-04-02T10:41:51Z","lastAt":"2006-04-04T18:53:45Z","messageCount":20,"participants":["Junio C Hamano","Linus Torvalds","Jason Riedy","H. Peter Anvin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"18215","messageId":"7vy7yol0nk.fsf@assigned-by-dhcp.cox.net","threadId":"3773","inReplyTo":null,"subject":"Solaris cloning woes partly diagnosed","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-02T10:41:51Z","receivedAt":"2006-04-02T10:41:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is just an interim report, but people might have heard that\nOpenSolaris team are in the process of choosing a free DSCM and\nwe are one of the candidates.  They initially wanted to try\n1.2.4 but had trouble using it for local cloning, and the\nevaluation is being done with 1.2.2.\n\nI was on #git tonight with Oejet, and managed to reproduce this\nproblem. The local clone problem seems to disappear if we\ndisable the progress bar in pack-objects.\n\nWe do two funky things when we have progress bar.  We play games\nwith timer signal (setitimer(ITIMER_REAL) and signal(SIGALRM)),\nand we spit out messages to stderr.\n\nIt's too late tonight for me to continue digging this, but if\nsomebody with access to a Solaris box is so inclined, I'd really\nappreciate help on this one.\n"},{"id":"18226","messageId":"Pine.LNX.4.64.0604021118210.3050@g5.osdl.org","threadId":"3773","inReplyTo":"7vy7yol0nk.fsf@assigned-by-dhcp.cox.net","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T18:33:46Z","receivedAt":"2006-04-02T18:33:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Junio C Hamano wrote:\n>\n> We do two funky things when we have progress bar.  We play games\n> with timer signal (setitimer(ITIMER_REAL) and signal(SIGALRM)),\n> and we spit out messages to stderr.\n\nI'd be willing to bet that it's the fact that we take signals.\n\nSuddenly, some system calls will either return -1/EINTR, or they'll return \npartial reads or writes. \n\nWe should be pretty good at handling that, but maybe some place forgets.\n\nOne thing to do might be to make the itimer use a much higher frequency, \nto trigger the problem more easily.\n\nWe do, for example, expect that regular file writing not do that. At least \n\"write_sha1_from_fd()\" will just do a \"write()\" without testing the error \nreturn, which is bad (it would silently create a truncated object if the \n/tmp filesystem filled up). If somebody has their filesystem over NFS \nmounted interruptible, partial writes could also happen.\n\nHo humm.\n\n\t\tLinus\n"},{"id":"18228","messageId":"29336.1144005022@lotus.CS.Berkeley.EDU","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021118210.3050@g5.osdl.org","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"Jason Riedy","fromEmail":"ejr@eecs.berkeley.edu","sentAt":"2006-04-02T19:10:22Z","receivedAt":"2006-04-02T19:10:22Z","isPatch":false,"sender":{"key":"ejr@eecs.berkeley.edu","avatar":"https://gravatar.com/avatar/547fa56f887cab01599edab4e9f813c949c1269e02714f20e0496c56185d9837?d=mp&s=160"},"body":"And Linus Torvalds writes:\n - \n - I'd be willing to bet that it's the fact that we take signals.\n\nUnfortunately, I'm too busy to check into this, but I've\nrun into similar problems in the past.  Just takes a busy \nfile server.\n\n - We do, for example, expect that regular file writing not do that. At least \n - \"write_sha1_from_fd()\" will just do a \"write()\" without testing the error \n - return, [...]\n\nThere is an xwrite in git-compat-util.h...\n\nJason\n"},{"id":"18229","messageId":"Pine.LNX.4.64.0604021159110.3050@g5.osdl.org","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021118210.3050@g5.osdl.org","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T19:18:29Z","receivedAt":"2006-04-02T19:18:29Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Linus Torvalds wrote:\n> \n> Suddenly, some system calls will either return -1/EINTR, or they'll return \n> partial reads or writes. \n\nHmm. If I read the IRC logs right, the bad pack is still a _valid_ pack, \nand passes git-verify-pack with flying colors.\n\nThat certainly implies that we had no problems with write-out: not only \nmust the SHA1 of the resulting file match itself, but it must match the \nindex too, and the number of objects there must match the index.\n\nSo the only way I see the pack being bad (if it does indeed pass \ngit-verify-pack) is if the object list we generated was bad.\n\nHowever, \"-q\" only affects git-pack-file itself, not the generation of the \nlist. Which would imply that we have trouble _reading_ the list as it \ncomes in through a pipe. Which is just insane, because we use just a \nbog-standard \"fgets(... stdin)\" for that. And no _way_ can stdio have \nproblems with a few SIGALRM's, that would break a lot of other problems.\n\nBut Oeje1 seems to be saying (in http://pastebin.com/635566):\n\n\tgit rev-list --objects --all | git pack-objects pack\n\tGenerating pack...\n\tDone counting 15 objects.\n\tDeltifying 15 objects.\n\t 100% (15/15) done\n\tWriting 15 objects.\n\t 100% (15/15) done\n\t806439fdfa5e9990b03f9301bd68e243795fff50\n\nwhere the result _should_ be 16385 objects, not 15.\n\nAnd the thing is, the _only_ thing we do there is that\n\n\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n\t\t...\n\t\tadd_object_entry(sha1, name_hash(NULL, line+41), 0);\n\nso it really really looks like fgets() would have problems with a SIGALRM \ncoming in and doesn't just re-try on EINTR. Can Solaris stdio _really_ be \nthat broken? (Yeah, yeah, it may be \"conforming\". It's also so incredibly \nprogrammer-unfriendly that it's not even funny)\n\nThat would be truly insane. Can somebody with Solaris check what the \nfollowing patch results in...\n\n\t\tLinus\n\n----\ndiff --git a/pack-objects.c b/pack-objects.c\nindex ccfaa5f..daba5de 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -1099,8 +1099,18 @@ int main(int argc, char **argv)\n \t\tfprintf(stderr, \"Generating pack...\\n\");\n \t}\n \n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n+\tfor (;;) {\n \t\tunsigned char sha1[20];\n+\n+\t\tif (!fgets(line, sizeof(line), stdin)) {\n+\t\t\tif (feof(stdin))\n+\t\t\t\tbreak;\n+\t\t\tif (!ferror(stdin))\n+\t\t\t\tdie(\"fgets returned NULL, not EOF, not error!\");\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tdie(\"fgets: %s\", strerror(errno));\n+\t\t}\n \n \t\tif (line[0] == '-') {\n \t\t\tif (get_sha1_hex(line+1, sha1))\n"},{"id":"18230","messageId":"Pine.LNX.4.64.0604021218490.3050@g5.osdl.org","threadId":"3773","inReplyTo":"29336.1144005022@lotus.CS.Berkeley.EDU","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T19:22:01Z","receivedAt":"2006-04-02T19:22:01Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Jason Riedy wrote:\n\n> And Linus Torvalds writes:\n>  - \n>  - I'd be willing to bet that it's the fact that we take signals.\n> \n> Unfortunately, I'm too busy to check into this, but I've\n> run into similar problems in the past.  Just takes a busy \n> file server.\n> \n>  - We do, for example, expect that regular file writing not do that. At least \n>  - \"write_sha1_from_fd()\" will just do a \"write()\" without testing the error \n>  - return, [...]\n> \n> There is an xwrite in git-compat-util.h...\n\nWell, git itself is actually fairly good about these things. Right now I'm \nseriously suspecting Solaris stdio as being just horribly impolite.\n\ngit tends to not just use xwrite() in most places, but check the return \nvalue for partial sizes etc. I tried to grep for places where we were \nlazy, and there really seems to be just a very small handful, and they \nshouldn't impact this case at all (you have to have a seriously broken \nsetup for them to matter, but we should fix them nonetheless.\n\n\t\tLinus\n"},{"id":"18232","messageId":"824.1144007555@lotus.CS.Berkeley.EDU","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021159110.3050@g5.osdl.org","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"Jason Riedy","fromEmail":"ejr@eecs.berkeley.edu","sentAt":"2006-04-02T19:52:35Z","receivedAt":"2006-04-02T19:52:35Z","isPatch":false,"sender":{"key":"ejr@eecs.berkeley.edu","avatar":"https://gravatar.com/avatar/547fa56f887cab01599edab4e9f813c949c1269e02714f20e0496c56185d9837?d=mp&s=160"},"body":"And Linus Torvalds writes:\n - \n - so it really really looks like fgets() would have problems with a SIGALRM \n - coming in and doesn't just re-try on EINTR. Can Solaris stdio _really_ be \n - that broken? (Yeah, yeah, it may be \"conforming\". It's also so incredibly \n - programmer-unfriendly that it's not even funny)\n\nYes, it is that broken.  I haven't encountered the problem \nconsistently in git myself, so I can't tell you if the patch \nworks.  Google finds similar reports and patches for BOINC, ruby,\nand a few other projects.\n\nSolaris folks will say you should be using sigaction with\nSA_RESTART.  IIRC, SA_RESTART isn't guaranteed to be there \nor work, but all the systems I deal with right now have it.\nSo an alternate patch for this one use is appended...  Other\nuses of signal could be changed to sigaction, too.  And\nprogress_update \"should\" be sig_atomic_t.\n\nPasses the pack-objects tests, but I can't make the problem \nhappen on demand.  (I have seen it occur before, but never\nduring make test, and I'd not tracked it down...)\n\nJason\n----\ndiff --git a/pack-objects.c b/pack-objects.c\nindex ccfaa5f..1faa0bb 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -877,10 +877,21 @@ static int try_delta(struct unpacked *cu\n \treturn 0;\n }\n \n-static void progress_interval(int signum)\n+static void progress_interval(int);\n+\n+static void setup_progress_signal(void)\n+{\n+\tstruct sigaction sa;\n+\tsa.sa_handler = progress_interval;\n+\tsigemptyset(&sa.sa_mask);\n+\tsa.sa_flags = SA_RESTART;\n+\tsigaction(SIGALRM, &sa, NULL);\n+}\n+\n+void progress_interval(int signum)\n {\n-\tsignal(SIGALRM, progress_interval);\n \tprogress_update = 1;\n+\tsetup_progress_signal();\n }\n \n static void find_deltas(struct object_entry **list, int window, int depth)\n@@ -1094,7 +1105,7 @@ int main(int argc, char **argv)\n \t\tv.it_interval.tv_sec = 1;\n \t\tv.it_interval.tv_usec = 0;\n \t\tv.it_value = v.it_interval;\n-\t\tsignal(SIGALRM, progress_interval);\n+\t\tsetup_progress_signal();\n \t\tsetitimer(ITIMER_REAL, &v, NULL);\n \t\tfprintf(stderr, \"Generating pack...\\n\");\n \t}\n"},{"id":"18233","messageId":"Pine.LNX.4.64.0604021312510.3050@g5.osdl.org","threadId":"3773","inReplyTo":"824.1144007555@lotus.CS.Berkeley.EDU","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T20:28:27Z","receivedAt":"2006-04-02T20:28:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Jason Riedy wrote:\n> \n> Solaris folks will say you should be using sigaction with\n> SA_RESTART.  IIRC, SA_RESTART isn't guaranteed to be there \n> or work, but all the systems I deal with right now have it.\n\nI think we might as well do that _too_.\n\nHowever, once you use \"sigaction()\", you don't need to re-arm the signal \nhandler any more, so I'd suggest a simpler patch like this instead..\n\nJunio, I think this confirms/explains the Solaris breakage.\n\nI'll re-send the \"anal stdio semantics\" version of the patch on top of \nthis in the next email.\n\n\t\t\tLinus\n----\nSubject: Fix Solaris stdio signal handling stupidities\n\nThis uses sigaction() to install the SIGALRM handler with SA_RESTART, so\nthat Solaris stdio doesn't break completely when a signal interrupts a\nread.\n\nThanks to Jason Riedy for confirming the silly Solaris signal behaviour.\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n----\n\ndiff --git a/pack-objects.c b/pack-objects.c\nindex ccfaa5f..1817b58 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -58,7 +58,7 @@ static int nr_objects = 0, nr_alloc = 0,\n static const char *base_name;\n static unsigned char pack_file_sha1[20];\n static int progress = 1;\n-static volatile int progress_update = 0;\n+static volatile sig_atomic_t progress_update = 0;\n \n /*\n  * The object names in objects array are hashed with this hashtable,\n@@ -879,7 +879,6 @@ static int try_delta(struct unpacked *cu\n \n static void progress_interval(int signum)\n {\n-\tsignal(SIGALRM, progress_interval);\n \tprogress_update = 1;\n }\n \n@@ -1025,6 +1024,23 @@ static int reuse_cached_pack(unsigned ch\n \treturn 1;\n }\n \n+static void setup_progress_signal(void)\n+{\n+\tstruct sigaction sa;\n+\tstruct itimerval v;\n+\n+\tmemset(&sa, 0, sizeof(sa));\n+\tsa.sa_handler = progress_interval;\n+\tsigemptyset(&sa.sa_mask);\n+\tsa.sa_flags = SA_RESTART;\n+\tsigaction(SIGALRM, &sa, NULL);\n+\n+\tv.it_interval.tv_sec = 1;\n+\tv.it_interval.tv_usec = 0;\n+\tv.it_value = v.it_interval;\n+\tsetitimer(ITIMER_REAL, &v, NULL);\n+}\n+\n int main(int argc, char **argv)\n {\n \tSHA_CTX ctx;\n@@ -1090,13 +1106,8 @@ int main(int argc, char **argv)\n \tprepare_packed_git();\n \n \tif (progress) {\n-\t\tstruct itimerval v;\n-\t\tv.it_interval.tv_sec = 1;\n-\t\tv.it_interval.tv_usec = 0;\n-\t\tv.it_value = v.it_interval;\n-\t\tsignal(SIGALRM, progress_interval);\n-\t\tsetitimer(ITIMER_REAL, &v, NULL);\n \t\tfprintf(stderr, \"Generating pack...\\n\");\n+\t\tsetup_progress_signal();\n \t}\n \n \twhile (fgets(line, sizeof(line), stdin) != NULL) {\n"},{"id":"18234","messageId":"Pine.LNX.4.64.0604021328380.3050@g5.osdl.org","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021312510.3050@g5.osdl.org","subject":"[PATCH 2/2] pack-objects: be incredibly anal about stdio semantics","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T20:31:54Z","receivedAt":"2006-04-02T20:31:54Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis is the \"letter of the law\" version of using fgets() properly in the\nface of incredibly broken stdio implementations.  We can work around the\nSolaris breakage with SA_RESTART, but in case anybody else is ever that\nstupid, here's the \"safe\" (read: \"insanely anal\") way to use fgets.\n\nIt probably goes without saying that I'm not terribly impressed by\nSolaris libc.\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n---\n\nThis is the same one that I already sent out, but re-diffed, and with a \nproper commit message.\n\nNot tested on Solaris.\n\nJunio - I think that I forgot to Cc: you on the 1/2 patch, but you'll see \nit on the git list.\n\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 1817b58..0ea16ad 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -1110,8 +1110,18 @@ int main(int argc, char **argv)\n \t\tsetup_progress_signal();\n \t}\n \n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n+\tfor (;;) {\n \t\tunsigned char sha1[20];\n+\n+\t\tif (!fgets(line, sizeof(line), stdin)) {\n+\t\t\tif (feof(stdin))\n+\t\t\t\tbreak;\n+\t\t\tif (!ferror(stdin))\n+\t\t\t\tdie(\"fgets returned NULL, not EOF, not error!\");\n+\t\t\tif (errno == EINTR)\n+\t\t\t\tcontinue;\n+\t\t\tdie(\"fgets: %s\", strerror(errno));\n+\t\t}\n \n \t\tif (line[0] == '-') {\n \t\t\tif (get_sha1_hex(line+1, sha1))\n"},{"id":"18236","messageId":"7vmzf3k7m9.fsf@assigned-by-dhcp.cox.net","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021328380.3050@g5.osdl.org","subject":"Re: [PATCH 2/2] pack-objects: be incredibly anal about stdio semantics","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-02T21:09:02Z","receivedAt":"2006-04-02T21:09:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> This is the \"letter of the law\" version of using fgets() properly in the\n> face of incredibly broken stdio implementations.  We can work around the\n> Solaris breakage with SA_RESTART, but in case anybody else is ever that\n> stupid, here's the \"safe\" (read: \"insanely anal\") way to use fgets.\n\nThanks.\n\nIt's good that I can say \"Oh, I think this is the part that is\nbroken, but I am going to bed\" to find the problem solved by\ncapable others when I wake up the next day.  Global distributed\ndevelopment process at the finest, although I suspect Jason,\nLinus and myself are all in the same timezone ;-)\n\nDid you mean this as a real change or a demonstration?  The\nsigaction change is a real fix, but somehow I find this one\nsimilar to the \"(void*) NULL\" thing you objected earlier (which\nwas not merged because I agreed with your argument)...\n"},{"id":"18238","messageId":"Pine.LNX.4.64.0604021417301.23419@g5.osdl.org","threadId":"3773","inReplyTo":"7vmzf3k7m9.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/2] pack-objects: be incredibly anal about stdio semantics","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T21:21:06Z","receivedAt":"2006-04-02T21:21:06Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@osdl.org> writes:\n> \n> > This is the \"letter of the law\" version of using fgets() properly in the\n> > face of incredibly broken stdio implementations.  We can work around the\n> > Solaris breakage with SA_RESTART, but in case anybody else is ever that\n> > stupid, here's the \"safe\" (read: \"insanely anal\") way to use fgets.\n> \n> Did you mean this as a real change or a demonstration?  The\n> sigaction change is a real fix, but somehow I find this one\n> similar to the \"(void*) NULL\" thing you objected earlier (which\n> was not merged because I agreed with your argument)...\n\nI don't have any really strong opinions on it. I think that any libc that \nneeds the \"ferror()\" test + EINTR loopback is totally broken. I would \nhappily say that people should just not use a development platform that is \nthat horrible.\n\nBut the fact that Solaris actually had that as a real problem (never mind \nthat we could work around it another way) just makes me go \"Hmm..\".\n\nSo I _think_ we're safe with just the \"sigaction()\" diff.  Neither of the \npatches _should_ make any difference at all on a sane platform. \n\n\t\tLinus\n"},{"id":"18241","messageId":"15051.1144015974@lotus.CS.Berkeley.EDU","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021417301.23419@g5.osdl.org","subject":"Re: [PATCH 2/2] pack-objects: be incredibly anal about stdio semantics","fromName":"Jason Riedy","fromEmail":"ejr@eecs.berkeley.edu","sentAt":"2006-04-02T22:12:54Z","receivedAt":"2006-04-02T22:12:54Z","isPatch":true,"sender":{"key":"ejr@eecs.berkeley.edu","avatar":"https://gravatar.com/avatar/547fa56f887cab01599edab4e9f813c949c1269e02714f20e0496c56185d9837?d=mp&s=160"},"body":"And Linus Torvalds writes:\n - \n - I don't have any really strong opinions on it. I think that any libc that \n - needs the \"ferror()\" test + EINTR loopback is totally broken. I would \n - happily say that people should just not use a development platform that is \n - that horrible.\n\nIf you consider stdio to be a low-level wrapper over syscalls\nthat only adds buffering and simple parsing, then passing EINTR\nback to the application is a sensible choice.  I wouldn't be\ntoo surprised if L4, VxWorks, etc. do something similar.\n\n - So I _think_ we're safe with just the \"sigaction()\" diff.  Neither of the \n - patches _should_ make any difference at all on a sane platform. \n\nAnyone with an older HP/UX care to try these patches?  HP/UX \nmay not be sane, but I think it may lack SA_RESTART.  I don't \nknow if stdio calls need restarted, though.\n\nJason\n"},{"id":"18245","messageId":"17063.1144016974@lotus.CS.Berkeley.EDU","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021312510.3050@g5.osdl.org","subject":"[PATCH] Use sigaction and SA_RESTART in read-tree.c; add option in Makefile.","fromName":"Jason Riedy","fromEmail":"ejr@eecs.berkeley.edu","sentAt":"2006-04-02T22:29:34Z","receivedAt":"2006-04-02T22:29:34Z","isPatch":true,"sender":{"key":"ejr@eecs.berkeley.edu","avatar":"https://gravatar.com/avatar/547fa56f887cab01599edab4e9f813c949c1269e02714f20e0496c56185d9837?d=mp&s=160"},"body":"Might as well ape the sigaction change in read-tree.c to avoid\nthe same potential problems.  The fprintf status output will\nbe overwritten in a second, so don't bother guarding it.  Do\nmove the fputc after disabling SIGALRM to ensure we go to the\nnext line, though.\n\nAlso add a NO_SA_RESTART option in the Makefile in case someone\ndoesn't have SA_RESTART but does restart (maybe older HP/UX?).\nWe want the builder to chose this specifically in case the\nsystem both lacks SA_RESTART and does not restart stdio calls;\na compat #define in git-compat-utils.h would silently allow\nbroken systems.\n\nSigned-off-by: Jason Riedy <ejr@cs.berkeley.edu>\n\n---\n\n Makefile    |    7 +++++++\n read-tree.c |   28 +++++++++++++++++++---------\n 2 files changed, 26 insertions(+), 9 deletions(-)\n\n521dc260ea90a23d58a4e920203af5035ca1a673\ndiff --git a/Makefile b/Makefile\nindex c79d646..f3b1d49 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -63,6 +63,9 @@\n # Define COLLISION_CHECK below if you believe that SHA1's\n # 1461501637330902918203684832716283019655932542976 hashes do not give you\n # sufficient guarantee that no collisions between objects will ever happen.\n+#\n+# Define NO_SA_RESTART if your platform does not have SA_RESTART, _AND_ if\n+# it automatically restarts interrupted stdio calls.\n \n # Define USE_NSEC below if you want git to care about sub-second file mtimes\n # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and\n@@ -425,6 +428,10 @@\n endif\n ifdef NO_ACCURATE_DIFF\n \tALL_CFLAGS += -DNO_ACCURATE_DIFF\n+endif\n+\n+ifdef NO_SA_RESTART\n+\tALL_CFLAGS += -DSA_RESTART=0\n endif\n \n # Shell quote (do not use $(call) to accomodate ancient setups);\ndiff --git a/read-tree.c b/read-tree.c\nindex eaff444..4422dbf 100644\n--- a/read-tree.c\n+++ b/read-tree.c\n@@ -273,10 +273,26 @@\n \n static void progress_interval(int signum)\n {\n-\tsignal(SIGALRM, progress_interval);\n \tprogress_update = 1;\n }\n \n+static void setup_progress_signal(void)\n+{\n+\tstruct sigaction sa;\n+\tstruct itimerval v;\n+\n+\tmemset(&sa, 0, sizeof(sa));\n+\tsa.sa_handler = progress_interval;\n+\tsigemptyset(&sa.sa_mask);\n+\tsa.sa_flags = SA_RESTART;\n+\tsigaction(SIGALRM, &sa, NULL);\n+\n+\tv.it_interval.tv_sec = 1;\n+\tv.it_interval.tv_usec = 0;\n+\tv.it_value = v.it_interval;\n+\tsetitimer(ITIMER_REAL, &v, NULL);\n+}\n+\n static void check_updates(struct cache_entry **src, int nr)\n {\n \tstatic struct checkout state = {\n@@ -289,8 +305,6 @@\n \tunsigned last_percent = 200, cnt = 0, total = 0;\n \n \tif (update && verbose_update) {\n-\t\tstruct itimerval v;\n-\n \t\tfor (total = cnt = 0; cnt < nr; cnt++) {\n \t\t\tstruct cache_entry *ce = src[cnt];\n \t\t\tif (!ce->ce_mode || ce->ce_flags & mask)\n@@ -302,12 +316,8 @@\n \t\t\ttotal = 0;\n \n \t\tif (total) {\n-\t\t\tv.it_interval.tv_sec = 1;\n-\t\t\tv.it_interval.tv_usec = 0;\n-\t\t\tv.it_value = v.it_interval;\n-\t\t\tsignal(SIGALRM, progress_interval);\n-\t\t\tsetitimer(ITIMER_REAL, &v, NULL);\n \t\t\tfprintf(stderr, \"Checking files out...\\n\");\n+\t\t\tsetup_progress_signal();\n \t\t\tprogress_update = 1;\n \t\t}\n \t\tcnt = 0;\n@@ -341,8 +351,8 @@\n \t\t}\n \t}\n \tif (total) {\n-\t\tfputc('\\n', stderr);\n \t\tsignal(SIGALRM, SIG_IGN);\n+\t\tfputc('\\n', stderr);\n \t}\n }\n \n-- \n1.3.0.rc1\n"},{"id":"18246","messageId":"Pine.LNX.4.64.0604021554480.23419@g5.osdl.org","threadId":"3773","inReplyTo":"15051.1144015974@lotus.CS.Berkeley.EDU","subject":"Re: [PATCH 2/2] pack-objects: be incredibly anal about stdio semantics","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-02T22:58:35Z","receivedAt":"2006-04-02T22:58:35Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Jason Riedy wrote:\n> \n> If you consider stdio to be a low-level wrapper over syscalls\n> that only adds buffering and simple parsing, then passing EINTR\n> back to the application is a sensible choice.  I wouldn't be\n> too surprised if L4, VxWorks, etc. do something similar.\n\nNo, returning EINTR is insane, because stdio has to do re-starting of \nsystem calls by hand _anyway_ for the \"short read\" case. \n\nEINTR really _is_ 100% equivalent to \"short read of zero bytes\" (that \nliterally is what it means for a read/write system call - anybody who \ntells you otherwise is just crazy). \n\nSo any library that handles short reads, but doesn't handle EINTR is by \ndefinition doing something totally idiotic and broken.\n\nNow, I agree that somebody else might be broken too. I would not agree \nthat it's \"acceptable\". It's craptastically bad library code.\n\n> Anyone with an older HP/UX care to try these patches?  HP/UX \n> may not be sane, but I think it may lack SA_RESTART.  I don't \n> know if stdio calls need restarted, though.\n\nI'd assume that older HPUX is so BSD-based that all signals end up \nrestarting. That was the BSD signal() semantics, after all.\n\n\t\t\tLinus\n"},{"id":"18251","messageId":"Pine.LNX.4.64.0604021752500.23419@g5.osdl.org","threadId":"3773","inReplyTo":"17063.1144016974@lotus.CS.Berkeley.EDU","subject":"Re: [PATCH] Use sigaction and SA_RESTART in read-tree.c; add option in Makefile.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-03T01:02:28Z","receivedAt":"2006-04-03T01:02:28Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Jason Riedy wrote:\n>\n> Might as well ape the sigaction change in read-tree.c to avoid\n> the same potential problems.\n\nLooks good. I didn't realize we had the exact same code duplicated. I \nguess it's small enough that there isn't a huge win in moving it to some \ncommon file..\n\nDoes somebody have access to Solaris to verify that this all actually does \nfix it? I obviously believe it will, since this just explains the symptoms \nto a tee, but it would still be good to have an actual confirmation by \nsomebody who has access to a Solaris environment.\n\n> Also add a NO_SA_RESTART option in the Makefile in case someone\n> doesn't have SA_RESTART but does restart (maybe older HP/UX?).\n> We want the builder to chose this specifically in case the\n> system both lacks SA_RESTART and does not restart stdio calls;\n> a compat #define in git-compat-utils.h would silently allow\n> broken systems.\n\nI do believe that we already require POSIX.2 functionality (regex, \nfnmatch, C90 compiler), which implies that git probably wouldn't compile \nanyway on things that are _really_ ancient. \n\nI think SA_RESTART was part of the original POSIX.1 specs, so anybody that \ndoesn't have it is likely to not have a lot of other things we rely on \ntoo. There are other SA_* flags that aren't as standard, but I'd expect \nSA_RESTART to be everywhere (or it likely doesn't have sigaction() at \nall..).\n\nBut hey, I certainly don't have really old HP-UX to test either.\n\n\t\tLinus\n"},{"id":"18254","messageId":"Pine.LNX.4.64.0604021951310.23419@g5.osdl.org","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021159110.3050@g5.osdl.org","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-03T03:06:00Z","receivedAt":"2006-04-03T03:06:00Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Linus Torvalds wrote:\n>  \n> -\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n> +\tfor (;;) {\n>  \t\tunsigned char sha1[20];\n> +\n> +\t\tif (!fgets(line, sizeof(line), stdin)) {\n> +\t\t\tif (feof(stdin))\n> +\t\t\t\tbreak;\n> +\t\t\tif (!ferror(stdin))\n> +\t\t\t\tdie(\"fgets returned NULL, not EOF, not error!\");\n> +\t\t\tif (errno == EINTR)\n> +\t\t\t\tcontinue;\n> +\t\t\tdie(\"fgets: %s\", strerror(errno));\n\nWhile it shouldn't actually matter, I just realized that this incredibly \nanal sequence isn't actually quite anal enough, sinceit really should have \na \"clearerr(stdin)\" for the continue case when we ignore the EINTR thing.\n\nOtherwise, some stdio implementation might just decide to continue to \nreturn NULL from fgets(), since the error indicator is technically sticky. \nI don't know if they do so, but it's entirely possible.\n\nSo I'd almost suggest something like\n\n\tchar *safe_fgets(char *s, int size, FILE *stream)\n\t{\n\t\tfor (;;) {\n\t\t\tif (fgets(s, size, stream))\n\t\t\t\treturn s;\n\t\t\tif (feof(stream))\n\t\t\t\treturn NULL;\n\t\t\tif (!ferror(stream))\n\t\t\t\tdie(\"fgets returned NULL, not EOF, not error!\");\n\t\t\tif (errno != EINTR)\n\t\t\t\tdie(\"fgets: %s\", strerror(errno));\n\t\t\tclearerr(stream);\n\t\t}\n\t}\n\nwhich sbould then hopefully work as a sane fgets()-replacement for broken \nenvironments.\n\n(And then we can just do\n\n\twhile (safe_fgets(..))\n\nlike normal people again, and not be afraid of strange stdio \nimplementations).\n\n\t\tLinus\n"},{"id":"18260","messageId":"7v7j67i93f.fsf@assigned-by-dhcp.cox.net","threadId":"3773","inReplyTo":"17063.1144016974@lotus.CS.Berkeley.EDU","subject":"Re: [PATCH] Use sigaction and SA_RESTART in read-tree.c; add option in Makefile.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-03T04:20:04Z","receivedAt":"2006-04-03T04:20:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jason Riedy <ejr@EECS.Berkeley.EDU> writes:\n\n> Also add a NO_SA_RESTART option in the Makefile in case someone\n> doesn't have SA_RESTART but does restart (maybe older HP/UX?).\n> We want the builder to chose this specifically in case the\n> system both lacks SA_RESTART and does not restart stdio calls;\n> a compat #define in git-compat-utils.h would silently allow\n> broken systems.\n\nWhat am I missing...?\n"},{"id":"18266","messageId":"Pine.LNX.4.64.0604022139100.3781@g5.osdl.org","threadId":"3773","inReplyTo":"7v7j67i93f.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Use sigaction and SA_RESTART in read-tree.c; add option in Makefile.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-03T04:40:10Z","receivedAt":"2006-04-03T04:40:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 2 Apr 2006, Junio C Hamano wrote:\n\n> Jason Riedy <ejr@EECS.Berkeley.EDU> writes:\n> \n> > Also add a NO_SA_RESTART option in the Makefile in case someone\n> > doesn't have SA_RESTART but does restart (maybe older HP/UX?).\n> > We want the builder to chose this specifically in case the\n> > system both lacks SA_RESTART and does not restart stdio calls;\n> > a compat #define in git-compat-utils.h would silently allow\n> > broken systems.\n> \n> What am I missing...?\n\nI don't think this part is worth it at least for now.\n\nIf there really are systems without SA_RESTART, they probably don't have \nsigaction() either, so #defining SA_RESTART to zero likely won't help.\n\nBut somebody can prove me wrong..\n\n\t\tLinus\n"},{"id":"18329","messageId":"7vu099916v.fsf@assigned-by-dhcp.cox.net","threadId":"3773","inReplyTo":"7vy7yol0nk.fsf@assigned-by-dhcp.cox.net","subject":"[RFH] Solaris cloning woes...","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-04T08:47:52Z","receivedAt":"2006-04-04T08:47:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The sigaction() patch cooked by Linus and Jason are in \"next\",\nto fix pack-objects breakage started between 1.2.2 and 1.2.3.\n\nOnce this is confirmed to fix the problem on Solaris boxes, I'd\nlike to include and do an 1.2.5, just in case OpenSolaris folks\nwould want to play with it, and also put them in the \"master\"\nbranch.\n\nI have an access to a not-so-well-maintained Solaris box at\nwork, and built the relevant parts with somewhat stripped down\nconfiguration to run the test.  The \"master\" version without the\nsigaction() patches exhibits the symptom Oejet and I observed\nthe other night, and \"next\" seems to fix it.  I am reasonably\nhappy.\n"},{"id":"18352","messageId":"4432B923.7040202@zytor.com","threadId":"3773","inReplyTo":"Pine.LNX.4.64.0604021118210.3050@g5.osdl.org","subject":"Re: Solaris cloning woes partly diagnosed","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2006-04-04T18:21:23Z","receivedAt":"2006-04-04T18:21:23Z","isPatch":false,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Linus Torvalds wrote:\n> \n> One thing to do might be to make the itimer use a much higher frequency, \n> to trigger the problem more easily.\n> \n> We do, for example, expect that regular file writing not do that. At least \n> \"write_sha1_from_fd()\" will just do a \"write()\" without testing the error \n> return, which is bad (it would silently create a truncated object if the \n> /tmp filesystem filled up). If somebody has their filesystem over NFS \n> mounted interruptible, partial writes could also happen.\n> \n\nThere seems to be a whole bunch of places where we use naked write()s \nwhere xwrite or fwrite would be a lot more appropriate.  The ssh-* files \nseem to be particularly offensive in that way.\n\nThere are also a number of places which call xwrite with the apparent \nbelief that returning short is an error (e.g. blame.c).  This as far as \nI know the more common definition of xwrite(), but is *not* the one used \nin git -- the one in git only guarantees that at least one character is \nwritten.\n\n\t-hpa\n"},{"id":"18355","messageId":"24378.1144176825@lotus.CS.Berkeley.EDU","threadId":"3773","inReplyTo":"7vu099916v.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFH] Solaris cloning woes...","fromName":"Jason Riedy","fromEmail":"ejr@eecs.berkeley.edu","sentAt":"2006-04-04T18:53:45Z","receivedAt":"2006-04-04T18:53:45Z","isPatch":false,"sender":{"key":"ejr@eecs.berkeley.edu","avatar":"https://gravatar.com/avatar/547fa56f887cab01599edab4e9f813c949c1269e02714f20e0496c56185d9837?d=mp&s=160"},"body":"And Junio C Hamano writes:\n - Once this is confirmed to fix the problem on Solaris boxes, I'd\n - like to include and do an 1.2.5, just in case OpenSolaris folks\n - would want to play with it, and also put them in the \"master\"\n - branch.\n\nI haven't run into the problem since, but I almost never saw\nit in the first place.  Have you been able to trigger it\nreliably?  Maybe I'm \"lucky\" that my Sun's so slow...\n\nJason\n"}]}