{"thread":{"id":"11596","subject":"git-commit fatal: Out of memory? mmap failed: Bad file descriptor","startedAt":"2008-01-11T22:11:13Z","lastAt":"2008-01-16T23:28:44Z","messageCount":39,"participants":["Brandon Casey","Charles Bailey","Marco Costalba","Junio C Hamano","Jeff King","Alex Riesen","Linus Torvalds","Kristian Høgsberg","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"65073","messageId":"4787E981.7010200@nrlssc.navy.mil","threadId":"11596","inReplyTo":null,"subject":"git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-11T22:11:13Z","receivedAt":"2008-01-11T22:11:13Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"\nI got this message from git-commit:\n\n$ git commit -a\n\nfatal: Out of memory? mmap failed: Bad file descriptor\nCreate commit \n\nThe exit status was 128.\nLooks like the commit was successful though.\nThe partial message 'Create commit ' comes from print_summary()\nin builtin-commit.c which is _after_ the actual commit.\n\n$ git --version\ngit version 1.5.4.rc2.84.gf85fd-dirty\n\nIt was compiled with NO_CURL=1. The dirtiness comes from the\npatches I submitted for relink earlier today.\n\nThe other possible clue is that this repo is on NFS.\n\n-brandon\n"},{"id":"65074","messageId":"4787EB38.7010600@hashpling.org","threadId":"11596","inReplyTo":"4787E981.7010200@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2008-01-11T22:18:32Z","receivedAt":"2008-01-11T22:18:32Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Brandon Casey wrote:\n> I got this message from git-commit:\n> \n> $ git commit -a\n> \n> fatal: Out of memory? mmap failed: Bad file descriptor\n> Create commit \n> \n> The exit status was 128.\n> Looks like the commit was successful though.\n> The partial message 'Create commit ' comes from print_summary()\n> in builtin-commit.c which is _after_ the actual commit.\n> \n> $ git --version\n> git version 1.5.4.rc2.84.gf85fd-dirty\n> \n> It was compiled with NO_CURL=1. The dirtiness comes from the\n> patches I submitted for relink earlier today.\n> \n> The other possible clue is that this repo is on NFS.\n> \n> -brandon\n\nI have seen this exact type of failure (commit reports possible \noom, but commit appears to have succeeded) with most recent gits.\n\nI had assumed that it was because I was using a very large \nrepository (experimenting with using git for general backup \npurposes) on a machine with not too much memory.\n\nPerhaps there's a real bug in here somewhere after all.\n\nCharles.\n"},{"id":"65075","messageId":"e5bfff550801111419o33a8904ajcf295417d3499bde@mail.gmail.com","threadId":"11596","inReplyTo":"4787E981.7010200@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2008-01-11T22:19:45Z","receivedAt":"2008-01-11T22:19:45Z","isPatch":false,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On Jan 11, 2008 11:11 PM, Brandon Casey wrote:\n>\n> I got this message from git-commit:\n>\n> $ git commit -a\n> \n> fatal: Out of memory? mmap failed: Bad file descriptor\n> Create commit \n>\n> The exit status was 128.\n> Looks like the commit was successful though.\n> The partial message 'Create commit ' comes from print_summary()\n> in builtin-commit.c which is _after_ the actual commit.\n>\n> $ git --version\n> git version 1.5.4.rc2.84.gf85fd-dirty\n>\n\nI had the same message about one week ago for few times, same\nsymptoms, I didn't had the time to dig it out and today it seems no\nmore happening.\n\n\n> It was compiled with NO_CURL=1. The dirtiness comes from the\n> patches I submitted for relink earlier today.\n>\n> The other possible clue is that this repo is on NFS.\n>\n\nI don't use any NFS mount.\n\n\nMarco\n"},{"id":"65077","messageId":"4787F1F5.2010905@nrlssc.navy.mil","threadId":"11596","inReplyTo":"4787E981.7010200@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-11T22:47:17Z","receivedAt":"2008-01-11T22:47:17Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"\n\nIt's reproduceable for me by amending the commit.\n\nAny suggestions?\n\n-brandon\n\n\n\nBrandon Casey wrote:\n> I got this message from git-commit:\n> \n> $ git commit -a\n> \n> fatal: Out of memory? mmap failed: Bad file descriptor\n> Create commit \n> \n> The exit status was 128.\n> Looks like the commit was successful though.\n> The partial message 'Create commit ' comes from print_summary()\n> in builtin-commit.c which is _after_ the actual commit.\n> \n> $ git --version\n> git version 1.5.4.rc2.84.gf85fd-dirty\n> \n> It was compiled with NO_CURL=1. The dirtiness comes from the\n> patches I submitted for relink earlier today.\n> \n> The other possible clue is that this repo is on NFS.\n> \n> -brandon\n> \n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"65083","messageId":"7vve5z2abv.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"4787F1F5.2010905@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-11T23:48:04Z","receivedAt":"2008-01-11T23:48:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> It's reproduceable for me by amending the commit.\n\nReliably reproducible? Can you build with \"-O0 -g\" and run\n\"commit --amend\" under gdb?\n"},{"id":"65098","messageId":"47880D18.8030405@nrlssc.navy.mil","threadId":"11596","inReplyTo":"7vve5z2abv.fsf@gitster.siamese.dyndns.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-12T00:43:04Z","receivedAt":"2008-01-12T00:43:04Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> It's reproduceable for me by amending the commit.\n> \n> Reliably reproducible? Can you build with \"-O0 -g\" and run\n> \"commit --amend\" under gdb?\n> \n\nmake NO_CURL=1 CFLAGS='-O0 -g'\nDone.\n\nI also moved xmmap into commit.c, and turned the inlined definition\nin git-compat-util.h into a declaration.\n\n\nI set a breakpoint on xmmap(). This is the backtrace on the last entry\ninto xmmap() before it die()'ed.\n\nThe fstat message at the end is from a call to fstat that I added to\nprint out the file size (to compare with mmap length). As you can\nsee the fstat also fails with the 'Bad file descriptor' message.\n\n\n#0 xmmap (start=0x0, length=996168, prot=1, flags=2, fd=6, offset=0)\n at commit.c:680\n#1 0x080acf30 in use_pack (p=0x8150650, w_cursor=0xffffc0ac, offset=94828, \n left=0xffffc06c) at sha1_file.c:748\n#2 0x080ae169 in unpack_object_header (p=0x8150650, w_curs=0xffffc0ac, \n curpos=0xffffc0a0, sizep=0xffffc1d0) at sha1_file.c:1333\n#3 0x080ae8bb in unpack_entry (p=0x8150650, obj_offset=94828, \n type=0xffffc1dc, sizep=0xffffc1d0) at sha1_file.c:1595\n#4 0x080ae55d in cache_or_unpack_entry (p=0x8150650, base_offset=94828, \n base_size=0xffffc1d0, type=0xffffc1dc, keep_cache=1) at sha1_file.c:1490\n#5 0x080af057 in read_packed_sha1 (\n sha1=0xffffc1b0 \"��\\034\\f~\\023\\203��E=�$n~��X��@�220\", \n type=0xffffc1dc, size=0xffffc1d0) at sha1_file.c:1815\n#6 0x080af2cf in read_sha1_file (\n sha1=0xffffc1b0 \"��\\034\\f~\\023\\203��E=�$n~��X��@�220\", \n type=0xffffc1dc, size=0xffffc1d0) at sha1_file.c:1881\n#7 0x080af3a0 in read_object_with_reference (\n sha1=0x853cf18 \"��\\034\\f~\\023\\203��E=�$n~��40000 libapsrs\", \n required_type_name=0x80f8488 \"tree\", size=0xffffc20c, \n actual_sha1_return=0x0) at sha1_file.c:1910\n#8 0x080cd2d0 in diff_tree_sha1 (\n old=0x853cf18 \"��\\034\\f~\\023\\203��E=�$n~��40000 libapsrs\", \n new=0x853d108 \"��\\036\\034_�006�\\f\\025\\236{�220StM0�40000 libapsrs\", \n base=0x8534098 \"aps/src/libapsnav/\", opt=0xffffc694) at tree-diff.c:376\n#9 0x080cc9cb in compare_tree_entry (t1=0xffffc330, t2=0xffffc310, \n base=0x815a260 \"aps/src/\", baselen=8, opt=0xffffc694) at tree-diff.c:61\n#10 0x080ccf93 in diff_tree (t1=0xffffc330, t2=0xffffc310, \n base=0x815a260 \"aps/src/\", opt=0xffffc694) at tree-diff.c:278\n#11 0x080cd371 in diff_tree_sha1 (\n old=0x853cbe0 \"N|\\021xH/K to continue, or q to quit---\n#12 0x080cc9cb in compare_tree_entry (t1=0xffffc430, t2=0xffffc410, \n base=0x817d3b8 \"aps/\", baselen=4, opt=0xffffc694) at tree-diff.c:61\n#13 0x080ccf93 in diff_tree (t1=0xffffc430, t2=0xffffc410, \n base=0x817d3b8 \"aps/\", opt=0xffffc694) at tree-diff.c:278\n#14 0x080cd371 in diff_tree_sha1 (\n old=0x853c580 \"\\037\\215��\\217\\200�E�b��232�03640000 avhrr\", \n new=0x853c848 \"g\\230\\032a \\207V�s~��r\\177�23540000 avhrr\", \n base=0x817d3b8 \"aps/\", opt=0xffffc694) at tree-diff.c:384\n#15 0x080cc9cb in compare_tree_entry (t1=0xffffc530, t2=0xffffc510, \n base=0x80faba8 \"\", baselen=0, opt=0xffffc694) at tree-diff.c:61\n#16 0x080ccf93 in diff_tree (t1=0xffffc530, t2=0xffffc510, base=0x80faba8 \"\", \n opt=0xffffc694) at tree-diff.c:278\n#17 0x080cd371 in diff_tree_sha1 (\n old=0x813c3dc \"\\231�2274M�\\236�\\t?�\\225\\t��\", \n new=0x813c43c \"*!�\\200\\006�\\235�?t��:DR\", base=0x80faba8 \"\", \n opt=0xffffc694) at tree-diff.c:384\n#18 0x080d11d0 in log_tree_diff (opt=0xffffc610, commit=0x813c438, \n log=0xffffc5c0) at log-tree.c:378\n#19 0x080d125a in log_tree_commit (opt=0xffffc610, commit=0x813c438)\n at log-tree.c:402\n#20 0x0805d26b in print_summary (prefix=0x0, \n sha1=0xffffd7e0 \"*!�\\200\\006�\\235�?t��:DR\") at builtin-commit.c:709\n#21 0x0805dae7 in cmd_commit (argc=0, argv=0xffffd9e8, prefix=0x0)\n at builtin-commit.c:898\n#22 0x0804b44b in run_command (p=0x80fd468, argc=3, argv=0xffffd9e8)\n at git.c:257\n#23 0x0804b5f9 in handle_internal_command (argc=3, argv=0xffffd9e8)\n at git.c:383\n#24 0x0804b75c in main (argc=3, argv=0xffffd9e8) at git.c:447\n(gdb) c\nContinuing.\nfstat failed: Bad file descriptor\nfatal: Out of memory? mmap failed: Bad file descriptor\nCreated commit \nProgram exited with code 0200.\n(gdb)\n"},{"id":"65101","messageId":"7vtzljzw8n.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"47880D18.8030405@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-12T01:08:24Z","receivedAt":"2008-01-12T01:08:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Junio C Hamano wrote:\n>> Brandon Casey writes:\n>> \n>>> It's reproduceable for me by amending the commit.\n>> \n>> Reliably reproducible? Can you build with \"-O0 -g\" and run\n>> \"commit --amend\" under gdb?\n>> \n>\n> make NO_CURL=1 CFLAGS='-O0 -g'\n> Done.\n>\n> I also moved xmmap into commit.c, and turned the inlined definition\n> in git-compat-util.h into a declaration.\n>\n>\n> I set a breakpoint on xmmap(). This is the backtrace on the last entry\n> into xmmap() before it die()'ed.\n>\n> The fstat message at the end is from a call to fstat that I added to\n> print out the file size (to compare with mmap length). As you can\n> see the fstat also fails with the 'Bad file descriptor' message.\n>\n> #0 xmmap (start=0x0, length=996168, prot=1, flags=2, fd=6, offset=0)\n> at commit.c:680\n> #1 0x080acf30 in use_pack (p=0x8150650, w_cursor=0xffffc0ac, offset=94828, \n> left=0xffffc06c) at sha1_file.c:748\n\nThat's the pack window shuffling code in use_pack(). I presume\nyour additional fstat is inside xmmap(), so if p->pack_fd is\nalready closed when this xmmap() call is made, that would\nexplain the symptom.\n\n\twhile (packed_git_limit < pack_mapped\n\t\t&& unuse_one_window(p, p->pack_fd))\n\t\t; /* nothing */\n\twin->base = xmmap(NULL, win->len,\n\t\tPROT_READ, MAP_PRIVATE,\n\t\tp->pack_fd, win->offset);\n\nI wonder what's the best way to find out who closes file\ndescriptor #6 without clearing p->pack_fd that still holds #6?\nMy reading of unuse_one_window() is that it tried to avoid\nclosing p->pack_fd, so it may already have been closed when we\nget to this codepath.\n\nShawn, does this ring a bell?\n"},{"id":"65120","messageId":"20080112045637.GC5211@coredump.intra.peff.net","threadId":"11596","inReplyTo":"4787EB38.7010600@hashpling.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-01-12T04:56:37Z","receivedAt":"2008-01-12T04:56:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 11, 2008 at 10:18:32PM +0000, Charles Bailey wrote:\n\n> I have seen this exact type of failure (commit reports possible oom, but \n> commit appears to have succeeded) with most recent gits.\n\nThis is almost certainly caused not by the commit action itself (which\nuses very little memory) but by the resulting diffstat to show what\nhappened. So the commit has already been \"committed\" to disk by the time\nit crashes.\n\nThis is at least the case with Brandon's problem (his stack trace shows\nthe diff happening).\n\n-Peff\n"},{"id":"65215","messageId":"20080112201622.GA2992@steel.home","threadId":"11596","inReplyTo":"4787F1F5.2010905@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-01-12T20:16:22Z","receivedAt":"2008-01-12T20:16:22Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Brandon Casey, Fri, Jan 11, 2008 23:47:17 +0100:\n> \n> It's reproduceable for me by amending the commit.\n> \n> Any suggestions?\n\nstrace -o log -f git commit -C HEAD --amend\n\nand post the \"log\" here (assuming it failed)\n"},{"id":"65346","messageId":"Pine.LNX.4.64.0801141715500.31161@torch.nrlssc.navy.mil","threadId":"11596","inReplyTo":"20080112201622.GA2992@steel.home","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-14T23:22:46Z","receivedAt":"2008-01-14T23:22:46Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sat, 12 Jan 2008, Alex Riesen wrote:\n\n> Brandon Casey, Fri, Jan 11, 2008 23:47:17 +0100:\n>>\n>> It's reproduceable for me by amending the commit.\n>>\n>> Any suggestions?\n>\n> strace -o log -f git commit -C HEAD --amend\n>\n> and post the \"log\" here (assuming it failed)\n\nIt does not fail when -C HEAD is used.\n\nSpecifically I did...\n\n Modify Makefile.in (random file).\n git commit -a --amend\n <:wq when vi opens, i.e. save without making changes>\n \n\n Modify Makefile.in\n git commit -a -C HEAD --amend\n \n\n Modify Makefile.in\n git commit -a --amend\n <:wq save without making change>\n \n\n-brandon\n"},{"id":"65356","messageId":"478C1D7A.6090103@nrlssc.navy.mil","threadId":"11596","inReplyTo":"4787E981.7010200@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-15T02:42:02Z","receivedAt":"2008-01-15T02:42:02Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Brandon Casey wrote:\n> I got this message from git-commit:\n> \n> $ git commit -a\n> \n> fatal: Out of memory? mmap failed: Bad file descriptor\n> Create commit \n\nI ran git-bisect and the result is below. Doesn't look like\nmuch help though.\n\nTo reiterate, I only have problems with the builtin-commit,\ni.e. 1.5.4.*, the 1.5.3.* series works correctly. Of course\nif this is a memory corruption issue, then it could just be\nthat the pattern of memory accesses in 1.5.3 does not tweak\nthe problem.\n\nThe other possibly useful info is that running\n'git commit -a -C HEAD --amend' does not cause the error.\n\n\n\n1596456309315befb3fd0a985d50a70ed09493e4 is first bad commit\ncommit 1596456309315befb3fd0a985d50a70ed09493e4\nAuthor: Junio C Hamano \nDate: Sun Dec 16 15:03:58 2007 -0800\n\n builtin-commit: fix summary output.\n\n Because print_summary() forgot to call diff_setup_done() after futzing with\n diff output options, it failed to activate recursive diff, which resulted in\n an incorrect summary.\n\n Signed-off-by: Junio C Hamano \n\n:100644 100644 518ebe0347e631c72f4e2a83b948259ee20fd213 61770ef456ca7f5f8342796e66f7ebfd3e1e7f73 M builtin-commit.c\n\n\nI've spent a number of hours trying to debug this. If there are any other ideas for\ndebugging, let me know. I'll keep the repo for a while.\n\n-brandon\n"},{"id":"65361","messageId":"alpine.LFD.1.00.0801142140560.2806@woody.linux-foundation.org","threadId":"11596","inReplyTo":"478C1D7A.6090103@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-15T05:42:11Z","receivedAt":"2008-01-15T05:42:11Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 14 Jan 2008, Brandon Casey wrote:\n> \n> To reiterate, I only have problems with the builtin-commit,\n> i.e. 1.5.4.*, the 1.5.3.* series works correctly. Of course\n> if this is a memory corruption issue, then it could just be\n> that the pattern of memory accesses in 1.5.3 does not tweak\n> the problem.\n\nCan you do an strace of the failure case and put it up on some public \nplace (it's likely going to be too big to send as email)?\n\n\t\t\tLinus\n"},{"id":"65377","messageId":"7vfxwze0te.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"478C1D7A.6090103@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-15T12:21:49Z","receivedAt":"2008-01-15T12:21:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Brandon Casey wrote:\n>> I got this message from git-commit:\n>> \n>> $ git commit -a\n>> \n>> fatal: Out of memory? mmap failed: Bad file descriptor\n>> Create commit \n\nI think from your earlier reports we already know that the issue\nis that somebody is closing a file descriptor we opened for a\npackfile and makes the code to shuffle the window that is used\nto access the packfile unhappy because it uses the fd to mmap.\n\n> I ran git-bisect and the result is below. Doesn't look like\n> much help though.\n\nThat change alone does look innocuous, but indeed is around the\nplace the code finds that the necessary fd is already closed.\n\n> The other possibly useful info is that running\n> 'git commit -a -C HEAD --amend' does not cause the error.\n\nA huge difference between \"-C HEAD\" and a commit with an editor\nis that the former bypasses the git-status code to fill the\ncommit message template. In addition to that, the latter spawns\na new process.\n\nIt could be the editor codepath may be closing the fd when it\nshouldn't, or some atexit() thing is triggering incorrectly.\nIIRC the fd incorrectly closed was #6, so it is not likely that\nprocess spawning code that may shuffle low fds is the culprit.\n\nAs Linus said already (and Alex suggested earlier in the nearby\nthread), strace output might be a good place to help digging\nthis issue further, instead of us idly speculating.\n\nWhat platform is this on?\n\nDoes it reliably reproduce for any commit in the repository, or\nreliably reproduce for one particular commit, or sometimes\nreprooduce for one particular commit?\n"},{"id":"65396","messageId":"478CECAB.2030906@nrlssc.navy.mil","threadId":"11596","inReplyTo":"alpine.LFD.1.00.0801142140560.2806@woody.linux-foundation.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-15T17:26:03Z","receivedAt":"2008-01-15T17:26:03Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Linus Torvalds wrote:\n> \n> On Mon, 14 Jan 2008, Brandon Casey wrote:\n>> To reiterate, I only have problems with the builtin-commit,\n>> i.e. 1.5.4.*, the 1.5.3.* series works correctly. Of course\n>> if this is a memory corruption issue, then it could just be\n>> that the pattern of memory accesses in 1.5.3 does not tweak\n>> the problem.\n> \n> Can you do an strace of the failure case and put it up on some public \n> place (it's likely going to be too big to send as email)?\n\nI did the strace. Below is the last screenful of lines.\n\nDo you have a suggestion for a public place to upload? I do not have\none of my own, and I've never used any of the 'free' services. The\nstrace log is about 8.5MB, compressed to about 500K.\n\n$ git --version\ngit version 1.5.4.rc3.11.g4e67\n\nNot that it's important, but looks like the file descriptor that\nis closed too soon is 3. I got 6 when running under gdb. This is\nalso using the latest version of git. The results are the same\nwith either version (including the fd#) so I just used this one.\n\nJunio C Hamano wrote:\n> What platform is this on?\n\n$ cat /etc/redhat-release\nCentOS release 4.5 (Final)\n\ni686\n\n> Does it reliably reproduce for any commit in the repository, or\n> reliably reproduce for one particular commit, or sometimes\n> reprooduce for one particular commit?\n\nReliably for one particular commit.\n\nAdditional commits on top of this commit complete successfully.\n\nIf this commit is amended without error by amending with '-C HEAD'\nor by using a 1.5.3 version, then additional amends or commits\nwill not produce the error.\n\n-brandon\n\n\n16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000\n16170 close(3) = 0\n16170 munmap(0xb1699000, 417) = 0\n16170 stat64(\"/home/casey/auto_v3.5/src_temp2/.git/objects/67/981a61208756cf4973\n7ec065e7bc0d7ff1a89d\", {st_mode=S_IFREG|0444, st_size=417, ...}) = 0\n16170 open(\"/home/casey/auto_v3.5/src_temp2/.git/objects/67/981a61208756cf49737e\nc065e7bc0d7ff1a89d\", O_RDONLY|O_LARGEFILE) = 3\n16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000\n16170 close(3) = 0\n16170 munmap(0xb1699000, 417) = 0\n16170 stat64(\"/home/casey/auto_v3.5/src_temp2/.git/objects/4e/7c1178482f4b3c52e8\nafce15db3bc8419530d2\", {st_mode=S_IFREG|0444, st_size=417, ...}) = 0\n16170 open(\"/home/casey/auto_v3.5/src_temp2/.git/objects/4e/7c1178482f4b3c52e8af\nce15db3bc8419530d2\", O_RDONLY|O_LARGEFILE) = 3\n16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000\n16170 close(3) = 0\n16170 munmap(0xb1699000, 417) = 0\n16170 stat64(\"/home/casey/auto_v3.5/src_temp2/.git/objects/02/3777b3c0e5f7697deb\n2ce738c6b38b1ec2b17c\", {st_mode=S_IFREG|0444, st_size=417, ...}) = 0\n16170 open(\"/home/casey/auto_v3.5/src_temp2/.git/objects/02/3777b3c0e5f7697deb2c\ne738c6b38b1ec2b17c\", O_RDONLY|O_LARGEFILE) = 3\n16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000\n16170 close(3) = 0\n16170 munmap(0xb1699000, 417) = 0\n16170 mmap2(NULL, 996168, PROT_READ, MAP_PRIVATE, 3, 0) = -1 EBADF (Bad file des\ncriptor)\n16170 munmap(0xb56bd000, 33554432) = 0\n16170 mmap2(NULL, 996168, PROT_READ, MAP_PRIVATE, 3, 0) = -1 EBADF (Bad file des\ncriptor)\n16170 write(2, \"fatal: Out of memory? mmap faile\"..., 55) = 55\n16170 write(1, \"Created commit \", 15) = 15\n16170 exit_group(128) = ?\n"},{"id":"65397","messageId":"alpine.LFD.1.00.0801150931260.2806@woody.linux-foundation.org","threadId":"11596","inReplyTo":"478CECAB.2030906@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-15T17:36:12Z","receivedAt":"2008-01-15T17:36:12Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 15 Jan 2008, Brandon Casey wrote:\n> \n> Do you have a suggestion for a public place to upload? I do not have\n> one of my own, and I've never used any of the 'free' services. The\n> strace log is about 8.5MB, compressed to about 500K.\n\nCan you just email the compressed one to me as an attachement, I'll put it \nsomewhere..\n\n> Not that it's important, but looks like the file descriptor that\n> is closed too soon is 3.\n\nYes and no. There obviously are several \"close(3)\"s in even that short \nsnippet, but they are for a different kind of close - they are for the \nregular loose object open/mmap/close/munmap sequence which has re-used \nthat file descriptor.\n\nSo the *incorrect* close(3) happened some time much earlier.\n\nThe other alternative is, of course, that the 3 itself is just wrong, and \nsomething corrupted the packfile data structures.\n\n> > Does it reliably reproduce for any commit in the repository, or\n> > reliably reproduce for one particular commit, or sometimes\n> > reprooduce for one particular commit?\n> \n> Reliably for one particular commit.\n> \n> Additional commits on top of this commit complete successfully.\n\nIt would obviously be interesting to see the base repository and the \ncommit you are trying to do - is that possibly publicly available?\n\n\t\tLinus\n"},{"id":"65398","messageId":"478CFAFF.6010006@nrlssc.navy.mil","threadId":"11596","inReplyTo":"alpine.LFD.1.00.0801150931260.2806@woody.linux-foundation.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-15T18:27:11Z","receivedAt":"2008-01-15T18:27:11Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Linus Torvalds wrote:\n> It would obviously be interesting to see the base repository and the \n> commit you are trying to do - is that possibly publicly available?\n\nI wish it was.\n\n-brandon\n"},{"id":"65399","messageId":"alpine.LFD.1.00.0801151036110.2806@woody.linux-foundation.org","threadId":"11596","inReplyTo":"478CFAFF.6010006@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-15T18:50:19Z","receivedAt":"2008-01-15T18:50:19Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 15 Jan 2008, Brandon Casey wrote:\n>\n> Linus Torvalds wrote:\n> > It would obviously be interesting to see the base repository and the \n> > commit you are trying to do - is that possibly publicly available?\n> \n> I wish it was.\n\nIt's ok, I found the bug in your full strace.\n\nThe bug really is pretty stupid:\n\n - prepare_index() does a \n\n\tfd = hold_lock_file_for_update(&false_lock, ...\n\t...\n\tif (write_cache(fd, active_cache, active_nr) || close(fd))\n\t\tdie(\"unable to write temporary index file\");\n\nand the magic here is that *it*closes*the*fd*.\n\nBut that's not how \"hold_lock_file_for_update()\" works. It still has that \nfd squirrelled away in it's \"false_lock.fd\", and later on, when we do \n\n\trollback_lock_file(&false_lock);\n\n(in the COMMIT_PARTIAL case of either \"commit_index_files()\" or \n\"rollback_index_files()\"), that rollback_lock_file() will do:\n\n\tvoid rollback_lock_file(struct lock_file *lk)\n\t{\n\t if (lk->filename[0]) {\n\t close(lk->fd);\n\t unlink(lk->filename);\n\t }\n\t lk->filename[0] = 0;\n\t}\n\nand now it's trying to close that fd *again* and would normally get a \nEBADF there. But in the meantime, somebody already re-used it for \nsomething else, and what rollback_lock_file() ends up doing is to just \nclose some random file descriptor.\n\nIn other words, I'm pretty sure that the bug goes away with this really \nugly hack. The real problem is that that \"false_lockfile\" thing simply \nmis-uses the whole lockfile interface. So this is not a pretty fix, but it \nat least should hide the effects of the mis-use of the interface.\n\nIn other words: the file descriptor that is returned by the lock_file \ninterface functions *MUST*NOT* be closed. But if you violate that rule, \nyou'd better make sure that you also fix the effects.\n\n\t\tLinus\n\n---\n builtin-commit.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 265ba6b..7a52224 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -308,6 +308,9 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \n \tif (write_cache(fd, active_cache, active_nr) || close(fd))\n \t\tdie(\"unable to write temporary index file\");\n+\n+\t/* We closed the false lock-file fd, make sure we don't do anything else to it */\n+\tfalse_lock.fd = -1;\n \treturn false_lock.filename;\n }\n \n"},{"id":"65402","messageId":"478D0CDA.5050709@nrlssc.navy.mil","threadId":"11596","inReplyTo":"alpine.LFD.1.00.0801151036110.2806@woody.linux-foundation.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-15T19:43:22Z","receivedAt":"2008-01-15T19:43:22Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Linus Torvalds wrote:\n> \n> On Tue, 15 Jan 2008, Brandon Casey wrote:\n>> Linus Torvalds wrote:\n>>> It would obviously be interesting to see the base repository and the \n>>> commit you are trying to do - is that possibly publicly available?\n>> I wish it was.\n> \n> It's ok, I found the bug in your full strace.\n\nGood catch, but that wasn't it. Still getting the same error.\n\n> and now it's trying to close that fd *again*\n\nIn that same vein, just above your changes in prepare_index() is:\n\n\tif (!pathspec || !*pathspec) {\n\t\tfd = hold_locked_index(&index_lock, 1);\n\t\trefresh_cache(REFRESH_QUIET);\n\t\tif (write_cache(fd, active_cache, active_nr) ||\n\t\t close(fd) || commit_locked_index(&index_lock))\n\t\t\tdie(\"unable to write new_index file\");\n\t\tcommit_style = COMMIT_AS_IS;\n\t\treturn get_index_file();\n\t}\n\nIf I followed hold_locked_index() correctly, then fd and index_lock.fd\nare equal, and commit_locked_index() does a close(lk->fd) making the\nclose(fd) above, redundant (or vice-versa).\n\nProbably not causing the error at hand, but not good.\n\n-brandon\n\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 1e55c2e..3b4f4e2 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -256,7 +256,7 @@ static char *prepare_index(int argc, const char **argv, cons\n\t\tfd = hold_locked_index(&index_lock, 1);\n\t\trefresh_cache(REFRESH_QUIET);\n\t\tif (write_cache(fd, active_cache, active_nr) ||\n-\t\t close(fd) || commit_locked_index(&index_lock))\n+\t\t commit_locked_index(&index_lock))\n\t\t\tdie(\"unable to write new_index file\");\n\t\tcommit_style = COMMIT_AS_IS;\n\t\treturn get_index_file();\n"},{"id":"65404","messageId":"1200427202.5821.7.camel@gaara.boston.redhat.com","threadId":"11596","inReplyTo":"478D0CDA.5050709@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2008-01-15T20:00:02Z","receivedAt":"2008-01-15T20:00:02Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"On Tue, 2008-01-15 at 13:43 -0600, Brandon Casey wrote:\n> Linus Torvalds wrote:\n> > \n> > On Tue, 15 Jan 2008, Brandon Casey wrote:\n> >> Linus Torvalds wrote:\n> >>> It would obviously be interesting to see the base repository and the \n> >>> commit you are trying to do - is that possibly publicly available?\n> >> I wish it was.\n> > \n> > It's ok, I found the bug in your full strace.\n> \n> Good catch, but that wasn't it. Still getting the same error.\n> \n> > and now it's trying to close that fd *again*\n> \n> In that same vein, just above your changes in prepare_index() is:\n> \n> \tif (!pathspec || !*pathspec) {\n> \t\tfd = hold_locked_index(&index_lock, 1);\n> \t\trefresh_cache(REFRESH_QUIET);\n> \t\tif (write_cache(fd, active_cache, active_nr) ||\n> \t\t close(fd) || commit_locked_index(&index_lock))\n> \t\t\tdie(\"unable to write new_index file\");\n> \t\tcommit_style = COMMIT_AS_IS;\n> \t\treturn get_index_file();\n> \t}\n> \n> If I followed hold_locked_index() correctly, then fd and index_lock.fd\n> are equal, and commit_locked_index() does a close(lk->fd) making the\n> close(fd) above, redundant (or vice-versa).\n\nTo my defense, the lockfile API is used a little inconsitently in git.\nMany places in git does a close(fd) and the call commit_locked_index(),\nwhich will close the fd again. Normally that will just cause an EBADFD\nwhich we ignore, but the problem here is that there's a longer time\nbetween close(fd) and the commit/rollback of the lock file. I guess the\ncorrect way to use the API is to never close the fd manually, but I\ncopied and pasted the lockfile use in builtin-commit.c from somewhere\nelse and along with it the double close.\n\nThere's four close(fd) calls in prepare_index() and they're all\nincorrect. The open fd's are cleaned up in rollback_index_files() and\nshouldn't be closed manually. The patch below gets rid of the extra\nclose() calls and should fix the problem.\n\ncheers,\nKristian\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 73f1e35..4494c9c 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -212,7 +212,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \t\tint fd = hold_locked_index(&index_lock, 1);\n \t\tadd_files_to_cache(0, also ? prefix : NULL, pathspec);\n \t\trefresh_cache(REFRESH_QUIET);\n-\t\tif (write_cache(fd, active_cache, active_nr) || close(fd))\n+\t\tif (write_cache(fd, active_cache, active_nr))\n \t\t\tdie(\"unable to write new_index file\");\n \t\tcommit_style = COMMIT_NORMAL;\n \t\treturn index_lock.filename;\n@@ -231,7 +231,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \t\tfd = hold_locked_index(&index_lock, 1);\n \t\trefresh_cache(REFRESH_QUIET);\n \t\tif (write_cache(fd, active_cache, active_nr) ||\n-\t\t close(fd) || commit_locked_index(&index_lock))\n+\t\t commit_locked_index(&index_lock))\n \t\t\tdie(\"unable to write new_index file\");\n \t\tcommit_style = COMMIT_AS_IS;\n \t\treturn get_index_file();\n@@ -273,7 +273,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \tfd = hold_locked_index(&index_lock, 1);\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n-\tif (write_cache(fd, active_cache, active_nr) || close(fd))\n+\tif (write_cache(fd, active_cache, active_nr))\n \t\tdie(\"unable to write new_index file\");\n \n \tfd = hold_lock_file_for_update(&false_lock,\n@@ -289,7 +289,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \n-\tif (write_cache(fd, active_cache, active_nr) || close(fd))\n+\tif (write_cache(fd, active_cache, active_nr))\n \t\tdie(\"unable to write temporary index file\");\n \treturn false_lock.filename;\n }\n"},{"id":"65406","messageId":"alpine.LFD.1.00.0801151207150.2806@woody.linux-foundation.org","threadId":"11596","inReplyTo":"478D0CDA.5050709@nrlssc.navy.mil","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-15T20:09:16Z","receivedAt":"2008-01-15T20:09:16Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 15 Jan 2008, Brandon Casey wrote:\n> \n> In that same vein, just above your changes in prepare_index() is:\n> \n> \tif (!pathspec || !*pathspec) {\n> \t\tfd = hold_locked_index(&index_lock, 1);\n> \t\trefresh_cache(REFRESH_QUIET);\n> \t\tif (write_cache(fd, active_cache, active_nr) ||\n> \t\t close(fd) || commit_locked_index(&index_lock))\n> \t\t\tdie(\"unable to write new_index file\");\n> \t\tcommit_style = COMMIT_AS_IS;\n> \t\treturn get_index_file();\n> \t}\n\nYeah, I think that may be the one that got you. I obviously couldn't \nfollow the exact path through the code, I was just looking at the trace of \nsystem calls and found that one thing that looked like it was your case, \nbut it's entirely possible that it was another path of the index lock file \nthat causes it.\n\nYour patch seems \"ObviouslyCorrect(tm)\".\n\n\t\tLinus\n"},{"id":"65410","messageId":"alpine.LFD.1.00.0801151219530.2806@woody.linux-foundation.org","threadId":"11596","inReplyTo":"alpine.LFD.1.00.0801151207150.2806@woody.linux-foundation.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-15T20:20:31Z","receivedAt":"2008-01-15T20:20:31Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 15 Jan 2008, Linus Torvalds wrote:\n> \n> Your patch seems \"ObviouslyCorrect(tm)\".\n\nAnd Kristian's more extensive patch that finds a few more cases looks \nbetter yet. Does that fix it for you?\n\n\t\tLinus\n"},{"id":"65411","messageId":"478D1741.8040807@nrlssc.navy.mil","threadId":"11596","inReplyTo":"1200427202.5821.7.camel@gaara.boston.redhat.com","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-15T20:27:45Z","receivedAt":"2008-01-15T20:27:45Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Kristian Høgsberg wrote:\n\n> There's four close(fd) calls in prepare_index() and they're all\n> incorrect. The open fd's are cleaned up in rollback_index_files() and\n> shouldn't be closed manually. The patch below gets rid of the extra\n> close() calls and should fix the problem.\n\nIt does. Thanks.\n\n-brandon\n"},{"id":"65416","messageId":"478D19F7.9020308@nrlssc.navy.mil","threadId":"11596","inReplyTo":"1200427202.5821.7.camel@gaara.boston.redhat.com","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-15T20:39:19Z","receivedAt":"2008-01-15T20:39:19Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Kristian Høgsberg wrote:\n\n> To my defense, the lockfile API is used a little inconsitently in git.\n> Many places in git does a close(fd) and the call commit_locked_index(),\n> which will close the fd again.\n\nI bet they did that so that the return status of close() could be checked\nsince commit_lock_file() doesn't currently check it.\n\n-brandon\n"},{"id":"65433","messageId":"7vzlv6d6sa.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"alpine.LFD.1.00.0801151036110.2806@woody.linux-foundation.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-15T23:10:29Z","receivedAt":"2008-01-15T23:10:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Tue, 15 Jan 2008, Brandon Casey wrote:\n>>\n>> Linus Torvalds wrote:\n>> > It would obviously be interesting to see the base repository and the \n>> > commit you are trying to do - is that possibly publicly available?\n>> \n>> I wish it was.\n>\n> It's ok, I found the bug in your full strace.\n>\n> The bug really is pretty stupid:\n>\n> - prepare_index() does a \n>\n> \tfd = hold_lock_file_for_update(&false_lock, ...\n> \t...\n> \tif (write_cache(fd, active_cache, active_nr) || close(fd))\n> \t\tdie(\"unable to write temporary index file\");\n>\n> and the magic here is that *it*closes*the*fd*.\n\nWhile I think the ones that are immediately followed by\ncommit_locked_index() can drop the close(fd) safely, I am not\nsure about Kristian's changes to the other ones that we\ncurrently close(fd) but do not commit nor rollback immediately.\nThese indices are now shown to the hook with open fd to it if\nyou choose not to close them. Is that okay for Windows guys? I\nsomehow had an impression that the other process may have\ntrouble accessing a file that is still open elsewhere for\nwriting.\n\nSo I think the approach along the lines of your \"hack\" to close\nand tell lockfile API not to double-close is more appropriate.\nWe would perhaps want \"close_lock_file(struct lock_file *)\" that\ncalls close(lk->fd) and does lk->fd = -1 without rename/unlink,\nand replace these close() with that.\n\nI am sick today, feeling feverish, and not thinking straight,\nso I may be talking total nonsense...\n"},{"id":"65454","messageId":"7vmyr6bluy.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"7vzlv6d6sa.fsf@gitster.siamese.dyndns.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T01:27:49Z","receivedAt":"2008-01-16T01:27:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Linus Torvalds writes:\n>\n>> On Tue, 15 Jan 2008, Brandon Casey wrote:\n>>>\n>>> Linus Torvalds wrote:\n>>> > It would obviously be interesting to see the base repository and the \n>>> > commit you are trying to do - is that possibly publicly available?\n>>> \n>>> I wish it was.\n>>\n>> It's ok, I found the bug in your full strace.\n>>\n>> The bug really is pretty stupid:\n>>\n>> - prepare_index() does a \n>>\n>> \tfd = hold_lock_file_for_update(&false_lock, ...\n>> \t...\n>> \tif (write_cache(fd, active_cache, active_nr) || close(fd))\n>> \t\tdie(\"unable to write temporary index file\");\n>>\n>> and the magic here is that *it*closes*the*fd*.\n>\n> While I think the ones that are immediately followed by\n> commit_locked_index() can drop the close(fd) safely, I am not\n> sure about Kristian's changes to the other ones that we\n> currently close(fd) but do not commit nor rollback immediately.\n> These indices are now shown to the hook with open fd to it if\n> you choose not to close them. Is that okay for Windows guys? I\n> somehow had an impression that the other process may have\n> trouble accessing a file that is still open elsewhere for\n> writing.\n>\n> So I think the approach along the lines of your \"hack\" to close\n> and tell lockfile API not to double-close is more appropriate.\n> We would perhaps want \"close_lock_file(struct lock_file *)\" that\n> calls close(lk->fd) and does lk->fd = -1 without rename/unlink,\n> and replace these close() with that.\n>\n> I am sick today, feeling feverish, and not thinking straight,\n> so I may be talking total nonsense...\n\nI'll aplly and push out Kristian's one that apparently got\nTested-by from Brandon for tonight.\n"},{"id":"65465","messageId":"Pine.LNX.4.44.0801152006260.944-100000@demand","threadId":"11596","inReplyTo":"7vmyr6bluy.fsf@gitster.siamese.dyndns.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2008-01-16T02:11:55Z","receivedAt":"2008-01-16T02:11:55Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, 15 Jan 2008, Junio C Hamano wrote:\n\n>> Junio C Hamano writes:\n>>\n>> While I think the ones that are immediately followed by\n>> commit_locked_index() can drop the close(fd) safely, I am not\n>> sure about Kristian's changes to the other ones that we\n>> currently close(fd) but do not commit nor rollback immediately.\n>> These indices are now shown to the hook with open fd to it if\n>> you choose not to close them. Is that okay for Windows guys? I\n>> somehow had an impression that the other process may have\n>> trouble accessing a file that is still open elsewhere for\n>> writing.\n>>\n>> So I think the approach along the lines of your \"hack\" to close\n>> and tell lockfile API not to double-close is more appropriate.\n>> We would perhaps want \"close_lock_file(struct lock_file *)\" that\n>> calls close(lk->fd) and does lk->fd = -1 without rename/unlink,\n>> and replace these close() with that.\n>>\n>> I am sick today, feeling feverish, and not thinking straight,\n>> so I may be talking total nonsense...\n\n> I'll aplly and push out Kristian's one that apparently got\n> Tested-by from Brandon for tonight.\n\nI've got a followup patch coming that will remove the rest of\nthe redundant close()'s and I'll look into your suggestion for\nclose_lock_file() above.\n\nCurrently everything passes the test suite, I just need to do\nsome manual testing.\n\n-brandon\n"},{"id":"65505","messageId":"478DB7F0.2020108@viscovery.net","threadId":"11596","inReplyTo":"7vzlv6d6sa.fsf@gitster.siamese.dyndns.org","subject":"Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-16T07:53:20Z","receivedAt":"2008-01-16T07:53:20Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> While I think the ones that are immediately followed by\n> commit_locked_index() can drop the close(fd) safely, I am not\n> sure about Kristian's changes to the other ones that we\n> currently close(fd) but do not commit nor rollback immediately.\n> These indices are now shown to the hook with open fd to it if\n> you choose not to close them. Is that okay for Windows guys? I\n> somehow had an impression that the other process may have\n> trouble accessing a file that is still open elsewhere for\n> writing.\n\nThe trouble is that on Windows open files cannot be deleted or renamed.\nHence, if an index file remains open, the hooks won't be able to modify\nthem (because of the create-new-file-then-rename-over-old tactics).\n\n> So I think the approach along the lines of your \"hack\" to close\n> and tell lockfile API not to double-close is more appropriate.\n> We would perhaps want \"close_lock_file(struct lock_file *)\" that\n> calls close(lk->fd) and does lk->fd = -1 without rename/unlink,\n> and replace these close() with that.\n\nYes!\n\n-- Hannes\n"},{"id":"65579","messageId":"7vk5m9r3ya.fsf_-_@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"Pine.LNX.4.44.0801152006260.944-100000@demand","subject":"[PATCH 1/2] Document lockfile API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T19:00:13Z","receivedAt":"2008-01-16T19:00:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We have nice set of placeholders, but nobody stepped in to fill\nthe gap in the API documentation, so I am doing it myself.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/api-lockfile.txt | 67 ++++++++++++++++++++++++++---\n 1 files changed, 60 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 73ac102..5b1553e 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -1,12 +1,65 @@\n lockfile API\n ============\n \n-Talk about , things like:\n+The lockfile API serves two purposes:\n \n-* lockfile lifetime -- atexit(3) looks at them, do not put them on the\n- stack;\n-* hold_lock_file_for_update()\n-* commit_lock_file()\n-* rollback_rock_file()\n+* Mutual exclusion. When we write out a new index file, first\n+ we create a new file `$GIT_DIR/index.lock`, write the new\n+ contents into it, and rename it to the final destination\n+ `$GIT_DIR/index`. We try to create the `$GIT_DIR/index.lock`\n+ file with O_EXCL so that we can notice and fail when somebody\n+ else is already trying to update the index file.\n \n-(JC, Dscho, Shawn)\n+* Automatic cruft removal. After we create the \"lock\" file, we\n+ may decide to `die()`, and we would want to make sure that we\n+ remove the file that has not been committed to its final\n+ destination. This is done by remembering the lockfiles we\n+ created in a linked list and cleaning them up from an\n+ `atexit(3)` handler. Outstanding lockfiles are also removed\n+ when the program dies on a signal.\n+\n+\n+The functions\n+-------------\n+\n+hold_lock_file_for_update::\n+\n+\tTake a pointer to `struct lock_file`, the filename of\n+\tthe final destination (e.g. `$GIT_DIR/index`) and a flag\n+\t`die_on_error`. Attempt to create a lockfile for the\n+\tdestination and return the file descriptor for writing\n+\tto the file. If `die_on_error` flag is true, it dies if\n+\ta lock is already taken for the file; otherwise it\n+\treturns a negative integer to the caller on failure.\n+\n+commit_lock_file::\n+\n+\tTake a pointer to the `struct lock_file` initialized\n+\twith an earlier call to `hold_lock_file_for_update()`,\n+\tclose the file descriptor and rename the lockfile to its\n+\tfinal destination.\n+\n+rollback_lock_file::\n+\n+\tTake a pointer to the `struct lock_file` initialized\n+\twith an earlier call to `hold_lock_file_for_update()`,\n+\tclose the file descriptor and remove the lockfile.\n+\n+Because the structure is used in an `atexit(3)` handler, its\n+storage has to stay throughout the life of the program. It\n+cannot be an auto variable allocated on the stack.\n+\n+Call `commit_lock_file()` or `rollback_lock_file()` when you are\n+done writing to the file descriptor. If you do not call either\n+and simply `exit(3)` from the program, an `atexit(3)` handler\n+will close and remove the lockfile.\n+\n+You should not close the file descriptor you obtained from\n+`hold_lock_file_for_update` function yourself. The `struct\n+lock_file` structure still remembers that the file descriptor\n+needs to be closed, and a later call to `commit_lock_file()` or\n+`rollback_lock_file()` will result in duplicate calls to\n+`close(2)`. Worse yet, if you `close(2)`, open another file\n+descriptor for completely different purpose, and then call\n+`commit_lock_file()` or `rollback_lock_file()`, they may close\n+that unrelated file descriptor.\n-- \n1.5.4.rc3.14.g44397\n"},{"id":"65580","messageId":"7vejchr3pf.fsf_-_@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"Pine.LNX.4.44.0801152006260.944-100000@demand","subject":"[PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T19:05:32Z","receivedAt":"2008-01-16T19:05:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The lockfile API is a handy way to obtain a file that is cleaned\nup if you die(). But sometimes you would need this sequence to\nwork:\n\n 1. hold_lock_file_for_update() to get a file descriptor for\n writing;\n\n 2. write the contents out, without being able to decide if the\n results should be committed or rolled back;\n\n 3. do something else that makes the decision --- and this\n \"something else\" needs the lockfile not to have an open file\n descriptor for writing (e.g. Windows do not want a open file\n to be renamed);\n\n 4. call commit_lock_file() or rollback_lock_file() as\n appropriately.\n\nThis adds close_lock_file() you can call between step 2 and 3 in\nthe above sequence.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/api-lockfile.txt | 11 +++++++++--\n cache.h | 2 +-\n lockfile.c | 6 ++++++\n 3 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 5b1553e..def5f2a 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -45,6 +45,11 @@ rollback_lock_file::\n \twith an earlier call to `hold_lock_file_for_update()`,\n \tclose the file descriptor and remove the lockfile.\n \n+close_lock_file::\n+\tTake a pointer to the `struct lock_file` initialized\n+\twith an earlier call to `hold_lock_file_for_update()`,\n+\tand close the file descriptor.\n+\n Because the structure is used in an `atexit(3)` handler, its\n storage has to stay throughout the life of the program. It\n cannot be an auto variable allocated on the stack.\n@@ -54,8 +59,10 @@ done writing to the file descriptor. If you do not call either\n and simply `exit(3)` from the program, an `atexit(3)` handler\n will close and remove the lockfile.\n \n-You should not close the file descriptor you obtained from\n-`hold_lock_file_for_update` function yourself. The `struct\n+If you need to close the file descriptor you obtained from\n+`hold_lock_file_for_update` function yourself, do so by calling\n+`close_lock_file()`. You should never call `close(2)` yourself!\n+Otherwise the `struct\n lock_file` structure still remembers that the file descriptor\n needs to be closed, and a later call to `commit_lock_file()` or\n `rollback_lock_file()` will result in duplicate calls to\ndiff --git a/cache.h b/cache.h\nindex 39331c2..5033b34 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -308,7 +308,7 @@ extern int commit_lock_file(struct lock_file *);\n extern int hold_locked_index(struct lock_file *, int);\n extern int commit_locked_index(struct lock_file *);\n extern void set_alternate_index_output(const char *);\n-\n+extern void close_lock_file(struct lock_file *);\n extern void rollback_lock_file(struct lock_file *);\n extern int delete_ref(const char *, const unsigned char *sha1);\n \ndiff --git a/lockfile.c b/lockfile.c\nindex f45d3ed..e57d850 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -201,3 +201,9 @@ void rollback_lock_file(struct lock_file *lk)\n \t}\n \tlk->filename[0] = 0;\n }\n+\n+void close_lock_file(struct lock_file *lk)\n+{\n+\tclose(lk->fd);\n+\tlk->fd = -1;\n+}\n-- \n1.5.4.rc3.14.g44397\n"},{"id":"65594","messageId":"alpine.LFD.1.00.0801161207220.2806@woody.linux-foundation.org","threadId":"11596","inReplyTo":"7vejchr3pf.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-16T20:08:58Z","receivedAt":"2008-01-16T20:08:58Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 16 Jan 2008, Junio C Hamano wrote:\n> +\n> +void close_lock_file(struct lock_file *lk)\n> +{\n> +\tclose(lk->fd);\n> +\tlk->fd = -1;\n> +}\n\nSince one of the main purposes of closing would be the error testing of \nwrites that haven't made it out yet on filesystems like NFS that do \nopen-close cache serialization, I'd suggest doing this as\n\n\tint close_lock_file(struct lock_file *lk)\n\t{\n\t\tint fd = lk->fd;\n\t\tlk->df = -1;\n\t\treturn close(fd);\n\t} \n\nto give the return code.\n\n\t\tLinus\n"},{"id":"65600","messageId":"7vodblo6c9.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"alpine.LFD.1.00.0801161207220.2806@woody.linux-foundation.org","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T20:36:54Z","receivedAt":"2008-01-16T20:36:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Wed, 16 Jan 2008, Junio C Hamano wrote:\n>> +\n>> +void close_lock_file(struct lock_file *lk)\n>> +{\n>> +\tclose(lk->fd);\n>> +\tlk->fd = -1;\n>> +}\n>\n> Since one of the main purposes of closing would be the error testing of \n> writes that haven't made it out yet on filesystems like NFS that do \n> open-close cache serialization, I'd suggest doing this as\n>\n> \tint close_lock_file(struct lock_file *lk)\n> \t{\n> \t\tint fd = lk->fd;\n> \t\tlk->df = -1;\n> \t\treturn close(fd);\n> \t} \n>\n> to give the return code.\n\nYup! You are as always right.\n"},{"id":"65603","messageId":"Pine.LNX.4.64.0801161443340.31161@torch.nrlssc.navy.mil","threadId":"11596","inReplyTo":"7vodblo6c9.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-16T20:46:23Z","receivedAt":"2008-01-16T20:46:23Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, 16 Jan 2008, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n>\n>> On Wed, 16 Jan 2008, Junio C Hamano wrote:\n>>> +\n>>> +void close_lock_file(struct lock_file *lk)\n>>> +{\n>>> +\tclose(lk->fd);\n>>> +\tlk->fd = -1;\n>>> +}\n>>\n>> Since one of the main purposes of closing would be the error testing of\n>> writes that haven't made it out yet on filesystems like NFS that do\n>> open-close cache serialization, I'd suggest doing this as\n>>\n>> \tint close_lock_file(struct lock_file *lk)\n>> \t{\n>> \t\tint fd = lk->fd;\n>> \t\tlk->df = -1;\n>> \t\treturn close(fd);\n>> \t}\n>>\n>> to give the return code.\n>\n> Yup! You are as always right.\n\nMy patch does this, though I understand it may take some time to review.\n\nI left the lk->fd unmodified when close() failed in case the caller\nwould like to include it in an error message.\n\n-brandon\n"},{"id":"65610","messageId":"7v7ii9o2ld.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"Pine.LNX.4.64.0801161443340.31161@torch.nrlssc.navy.mil","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T21:57:50Z","receivedAt":"2008-01-16T21:57:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> My patch does this, though I understand it may take some time to review.\n\nThis is what I have right now, squashed your change into [2/2] \nI sent earlier, along with a couple of further fixups.\n\nImprovement since my v1 are:\n\n - close_lock_file() returns int, which is the return value from\n close(2);\n\n - remove_lock_file() avoids calling close(2) if\n close_lock_file() was called earlier on the lockfile;\n\n - commit_lock_file() does the same, and notices failure\n returned from close_lock_file(). In addition, it unlinks the\n lockfile and clears lk->filename upon failure;\n\n - commit_locked_index() does the same, and notices failure\n returned from close_lock_file() in alternate_index_output\n codepath. It unlinks the lockfile and clears lk->filename\n upon failure;\n\n - The codepath in commit_locked_index() for writing to\n alternate_index_output clears the alternate_index_output\n variable when it is done.\n\nMost of the above are thanks to the changes to lockfile.c in your\nversion and Linus's suggestion.\n\nI am planning to take your patch (only the parts that fix the\ncallers, because the changes to lockfile.c are all included\nhere) on top of this one.\n\n-- >8 --\nclose_lock_file(): new function in the lockfile API\n\nThe lockfile API is a handy way to obtain a file that is cleaned\nup if you die(). But sometimes you would need this sequence to\nwork:\n\n 1. hold_lock_file_for_update() to get a file descriptor for\n writing;\n\n 2. write the contents out, without being able to decide if the\n results should be committed or rolled back;\n\n 3. do something else that makes the decision --- and this\n \"something else\" needs the lockfile not to have an open file\n descriptor for writing (e.g. Windows do not want a open file\n to be renamed);\n\n 4. call commit_lock_file() or rollback_lock_file() as\n appropriately.\n\nThis adds close_lock_file() you can call between step 2 and 3 in\nthe above sequence.\n\n[jc: updated with Brandon's stricter error checking on return values\n from close() and a suggestion by Linus.]\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/technical/api-lockfile.txt | 15 +++++++++++--\n cache.h | 2 +-\n lockfile.c | 32 ++++++++++++++++++++++++-----\n 3 files changed, 39 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 5b1553e..dd89404 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -37,7 +37,8 @@ commit_lock_file::\n \tTake a pointer to the `struct lock_file` initialized\n \twith an earlier call to `hold_lock_file_for_update()`,\n \tclose the file descriptor and rename the lockfile to its\n-\tfinal destination.\n+\tfinal destination. Returns 0 upon success, a negative\n+\tvalue on failure to close(2) or rename(2).\n \n rollback_lock_file::\n \n@@ -45,6 +46,12 @@ rollback_lock_file::\n \twith an earlier call to `hold_lock_file_for_update()`,\n \tclose the file descriptor and remove the lockfile.\n \n+close_lock_file::\n+\tTake a pointer to the `struct lock_file` initialized\n+\twith an earlier call to `hold_lock_file_for_update()`,\n+\tand close the file descriptor. Returns 0 upon success,\n+\ta negative value on failure to close(2).\n+\n Because the structure is used in an `atexit(3)` handler, its\n storage has to stay throughout the life of the program. It\n cannot be an auto variable allocated on the stack.\n@@ -54,8 +61,10 @@ done writing to the file descriptor. If you do not call either\n and simply `exit(3)` from the program, an `atexit(3)` handler\n will close and remove the lockfile.\n \n-You should not close the file descriptor you obtained from\n-`hold_lock_file_for_update` function yourself. The `struct\n+If you need to close the file descriptor you obtained from\n+`hold_lock_file_for_update` function yourself, do so by calling\n+`close_lock_file()`. You should never call `close(2)` yourself!\n+Otherwise the `struct\n lock_file` structure still remembers that the file descriptor\n needs to be closed, and a later call to `commit_lock_file()` or\n `rollback_lock_file()` will result in duplicate calls to\ndiff --git a/cache.h b/cache.h\nindex 39331c2..24735bd 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -308,7 +308,7 @@ extern int commit_lock_file(struct lock_file *);\n extern int hold_locked_index(struct lock_file *, int);\n extern int commit_locked_index(struct lock_file *);\n extern void set_alternate_index_output(const char *);\n-\n+extern int close_lock_file(struct lock_file *);\n extern void rollback_lock_file(struct lock_file *);\n extern int delete_ref(const char *, const unsigned char *sha1);\n \ndiff --git a/lockfile.c b/lockfile.c\nindex f45d3ed..fcf9285 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -13,7 +13,8 @@ static void remove_lock_file(void)\n \twhile (lock_file_list) {\n \t\tif (lock_file_list->owner == me &&\n \t\t lock_file_list->filename[0]) {\n-\t\t\tclose(lock_file_list->fd);\n+\t\t\tif (lock_file_list->fd >= 0)\n+\t\t\t\tclose(lock_file_list->fd);\n \t\t\tunlink(lock_file_list->filename);\n \t\t}\n \t\tlock_file_list = lock_file_list->next;\n@@ -159,11 +160,23 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on\n \treturn fd;\n }\n \n+int close_lock_file(struct lock_file *lk)\n+{\n+\tint fd = lk->fd;\n+\tlk->fd = -1;\n+\treturn close(fd);\n+}\n+\n int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\n \tint i;\n-\tclose(lk->fd);\n+\n+\tif (lk->fd >= 0 && close_lock_file(lk)) {\n+\t\tunlink(lk->filename);\n+\t\tlk->filename[0] = 0;\n+\t\treturn -1;\n+\t}\n \tstrcpy(result_file, lk->filename);\n \ti = strlen(result_file) - 5; /* .lock */\n \tresult_file[i] = 0;\n@@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name)\n int commit_locked_index(struct lock_file *lk)\n {\n \tif (alternate_index_output) {\n-\t\tint result = rename(lk->filename, alternate_index_output);\n-\t\tlk->filename[0] = 0;\n-\t\treturn result;\n+\t\tconst char *newname = alternate_index_output;\n+\t\talternate_index_output = NULL;\n+\n+\t\tif (lk->fd >= 0 && close_lock_file(lk)) {\n+\t\t\tunlink(lk->filename);\n+\t\t\tlk->filename[0] = 0;\n+\t\t\treturn -1;\n+\t\t}\n+\t\treturn rename(lk->filename, newname);\n \t}\n \telse\n \t\treturn commit_lock_file(lk);\n@@ -196,7 +215,8 @@ int commit_locked_index(struct lock_file *lk)\n void rollback_lock_file(struct lock_file *lk)\n {\n \tif (lk->filename[0]) {\n-\t\tclose(lk->fd);\n+\t\tif (lk->fd >= 0)\n+\t\t\tclose(lk->fd);\n \t\tunlink(lk->filename);\n \t}\n \tlk->filename[0] = 0;\n-- \n1.5.4.rc3.14.g44397\n"},{"id":"65616","messageId":"478E893F.4070100@nrlssc.navy.mil","threadId":"11596","inReplyTo":"7v7ii9o2ld.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-16T22:46:23Z","receivedAt":"2008-01-16T22:46:23Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> My patch does this, though I understand it may take some time to review.\n> \n> This is what I have right now, squashed your change into [2/2] \n> I sent earlier, along with a couple of further fixups.\n> \n\nMainly, I prefer to not modify the data structures when a failure occurs.\n\nGenerally when commit_lock_file fails, the caller die()'s and then\nremove_lock_file will delete the temporary file. If the caller were to\nnot call die, I think it should call rollback_lock_file() similarly to\nhow unlock_ref() is called everywhere in refs.c.\n\nSo I think the semantics should be:\n\n\tlock_fd = hold_lock_file(&lock);\n\t\n\tif (close_lock_file(&lock)) {\n\t\trollback_lock_file(&lock);\n\t\treturn error;\n\t}\n\tif (commit_lock_file(&lock)) {\n\t\trollback_lock_file(&lock);\n\t\treturn error;\n\t}\n\n\n> diff --git a/lockfile.c b/lockfile.c\n> index f45d3ed..fcf9285 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -13,7 +13,8 @@ static void remove_lock_file(void)\n> \twhile (lock_file_list) {\n> \t\tif (lock_file_list->owner == me &&\n> \t\t lock_file_list->filename[0]) {\n> -\t\t\tclose(lock_file_list->fd);\n> +\t\t\tif (lock_file_list->fd >= 0)\n> +\t\t\t\tclose(lock_file_list->fd);\n> \t\t\tunlink(lock_file_list->filename);\n> \t\t}\n> \t\tlock_file_list = lock_file_list->next;\n> @@ -159,11 +160,23 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on\n> \treturn fd;\n> }\n> \n> +int close_lock_file(struct lock_file *lk)\n> +{\n> +\tint fd = lk->fd;\n> +\tlk->fd = -1;\n> +\treturn close(fd);\n> +}\n\nminor nit on this that I mentioned in another email.\n\n> +\n> int commit_lock_file(struct lock_file *lk)\n> {\n> \tchar result_file[PATH_MAX];\n> \tint i;\n> -\tclose(lk->fd);\n> +\n> +\tif (lk->fd >= 0 && close_lock_file(lk)) {\n> +\t\tunlink(lk->filename);\n> +\t\tlk->filename[0] = 0;\n> +\t\treturn -1;\n> +\t}\n\n\nI would rather have the caller call rollback_lock_file, or\nfall back to remove_lock_file rather than do this unlinking\nand modifying lk->filename here in commit_lock_file.\n\n\n> \tstrcpy(result_file, lk->filename);\n> \ti = strlen(result_file) - 5; /* .lock */\n> \tresult_file[i] = 0;\n> @@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name)\n> int commit_locked_index(struct lock_file *lk)\n> {\n> \tif (alternate_index_output) {\n> -\t\tint result = rename(lk->filename, alternate_index_output);\n> -\t\tlk->filename[0] = 0;\n> -\t\treturn result;\n> +\t\tconst char *newname = alternate_index_output;\n> +\t\talternate_index_output = NULL;\n> +\n> +\t\tif (lk->fd >= 0 && close_lock_file(lk)) {\n> +\t\t\tunlink(lk->filename);\n> +\t\t\tlk->filename[0] = 0;\n> +\t\t\treturn -1;\n> +\t\t}\n\nditto here.\n\n> +\t\treturn rename(lk->filename, newname);\n\n\nIf rename succeeds, we'll try to do an unnecessary unlink atexit.\n\n\nHaving said those things, I will defer to your more experienced judgment.\n\n-brandon\n"},{"id":"65619","messageId":"7vy7apmlci.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"478E893F.4070100@nrlssc.navy.mil","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T22:55:41Z","receivedAt":"2008-01-16T22:55:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Mainly, I prefer to not modify the data structures when a failure occurs.\n\nOk. Is the rest of your patch that fixes callers Ok with that\nsemantics? If so, I'd agree that is probably cleaner. I'll\nscrap the one we are discussing, resurrecting only the api\ndocumentation part, and replace it with the lockfile.c changes\nfrom your patch, along with the fixes to callers.\n \n"},{"id":"65624","messageId":"478E8E6B.80702@nrlssc.navy.mil","threadId":"11596","inReplyTo":"7vy7apmlci.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-16T23:08:27Z","receivedAt":"2008-01-16T23:08:27Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> Mainly, I prefer to not modify the data structures when a failure occurs.\n> \n> Ok. Is the rest of your patch that fixes callers Ok with that\n> semantics?\n\nyes.\n\n> If so, I'd agree that is probably cleaner. I'll\n> scrap the one we are discussing, resurrecting only the api\n> documentation part, and replace it with the lockfile.c changes\n> from your patch, along with the fixes to callers.\n\nMost of that patch is straight forward, just removing close().\n\nI think you should consider how to handle fdopen on the lock\ndescriptor and the fact that start_command closes the lock\nfile descriptor in create_bundle().\n\nAfter we fdopen, we should always fclose() and never close().\nThis isn't enforced.\n\nI merely assigned the file descriptor to -1 when it was safe\n(i.e. after fclose), and added a comment. We could add another\nfunction which did this automatically, but maybe that is too\nmuch effort, especially in the bundle case.\n\n-brandon\n"},{"id":"65629","messageId":"478E9068.1040602@nrlssc.navy.mil","threadId":"11596","inReplyTo":"478E8E6B.80702@nrlssc.navy.mil","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-16T23:16:56Z","receivedAt":"2008-01-16T23:16:56Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Brandon Casey wrote:\n> I think you should consider how to handle fdopen on the lock\n> descriptor\n\nThis happens in\nbuiltin-pack-refs.c:pack_refs\nfast-import.c:dump_marks\n\n-brandon\n"},{"id":"65630","messageId":"7vtzldmk8p.fsf@gitster.siamese.dyndns.org","threadId":"11596","inReplyTo":"Pine.LNX.4.64.0801161443340.31161@torch.nrlssc.navy.mil","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T23:19:34Z","receivedAt":"2008-01-16T23:19:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> My patch does this, though I understand it may take some time to review.\n>\n> I left the lk->fd unmodified when close() failed in case the caller\n> would like to include it in an error message.\n\nBut that would bring us back to the same double-close issue,\nwouldn't it?\n\n\tif (close_lock_file(lock))\n\t\tdie(\"Oops, failed to close fd %d\", lock->fd);\n\nis not enough. You need to do:\n\n\tif (close_lock_file(lock)) {\n \tint fd = lock->fd;\n lock->fd = -1;\n\t\tdie(\"Oops, failed to close fd %d\", fd);\n\t}\n\nto avoid atexit handler closing the lock->fd.\n\nWorse yet, a careless caller may do:\n\n\tclose_lock_file(lock);\n\n\t... do something that opens a new fd, perhaps for\n ... mmaping a packfile in\n\n\trollback_lock_file(lock);\n\n\t... Oops, we cannot mmap the packfile.\n"},{"id":"65631","messageId":"Pine.LNX.4.64.0801161725010.31161@torch.nrlssc.navy.mil","threadId":"11596","inReplyTo":"7vtzldmk8p.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-01-16T23:28:44Z","receivedAt":"2008-01-16T23:28:44Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, 16 Jan 2008, Junio C Hamano wrote:\n\n> Brandon Casey writes:\n>\n>> My patch does this, though I understand it may take some time to review.\n>>\n>> I left the lk->fd unmodified when close() failed in case the caller\n>> would like to include it in an error message.\n>\n> But that would bring us back to the same double-close issue,\n> wouldn't it?\n\nYes it would. Although I knew it would happen, I was disregarding\n_that_ double close case for some reason.\n\nYou're right, your's and Linus's version is better.\n\n-brandon\n"}]}