{"thread":{"id":"7338","subject":"Be more careful about zlib return values","startedAt":"2007-03-20T18:38:34Z","lastAt":"2007-03-21T15:29:30Z","messageCount":3,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"37635","messageId":"Pine.LNX.4.64.0703201124260.6730@woody.linux-foundation.org","threadId":"7338","inReplyTo":null,"subject":"Be more careful about zlib return values","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-03-20T18:38:34Z","receivedAt":"2007-03-20T18:38:34Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWhen creating a new object, we use \"deflate(stream, Z_FINISH)\" in a loop \nuntil it no longer returns Z_OK, and then we do \"deflateEnd()\" to finish \nup business.\n\nThat should all work, but the fact is, it's not how you're _supposed_ to \nuse the zlib return values properly:\n\n - deflate() should never return Z_OK in the first place, except if we \n need to increase the output buffer size (which we're not doing, and \n should never need to do, since we pre-allocated a buffer that is \n supposed to be able to hold the output in full). So the \"while()\" loop \n was incorrect: Z_OK doesn't actually mean \"ok, continue\", it means \"ok, \n allocate more memory for me and continue\"!\n\n - if we got an error return, we would consider it to be end-of-stream, \n but it could be some internal zlib error. In short, we should check \n for Z_STREAM_END explicitly, since that's the only valid return value \n anyway for the Z_FINISH case.\n\n - we never checked deflateEnd() return codes at all.\n\nNow, admittedly, none of these issues should ever happen, unless there is \nsome internal bug in zlib. So this patch should make zero difference, but \nit seems to be the right thing to do.\n\nWe should probablybe anal and check the return value of \"deflateInit()\" \ntoo!\n\nSigned-off-by: Linus Torvalds \n---\n\nSomebody who has worked more with zlib should probably double-check me, \nbut this is what claims is the right thing to do.\n\n\t\tLinus\n\n---\n sha1_file.c | 13 +++++++++----\n 1 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c445a24..bfcbbea 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1947,7 +1947,7 @@ int hash_sha1_file(void *buf, unsigned long len, const char *type,\n \n int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n {\n-\tint size;\n+\tint size, ret;\n \tunsigned char *compressed;\n \tz_stream stream;\n \tunsigned char sha1[20];\n@@ -2007,9 +2007,14 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n \t/* Then the data itself.. */\n \tstream.next_in = buf;\n \tstream.avail_in = len;\n-\twhile (deflate(&stream, Z_FINISH) == Z_OK)\n-\t\t/* nothing */;\n-\tdeflateEnd(&stream);\n+\tret = deflate(&stream, Z_FINISH);\n+\tif (ret != Z_STREAM_END)\n+\t\tdie(\"unable to deflate new object %s (%d)\", sha1_to_hex(sha1), ret);\n+\n+\tret = deflateEnd(&stream);\n+\tif (ret != Z_OK)\n+\t\tdie(\"deflateEnd on object %s failed (%d)\", sha1_to_hex(sha1), ret);\n+\n \tsize = stream.total_out;\n \n \tif (write_buffer(fd, compressed, size) < 0)\n"},{"id":"37661","messageId":"7vodmn2eya.fsf@assigned-by-dhcp.cox.net","threadId":"7338","inReplyTo":"Pine.LNX.4.64.0703201124260.6730@woody.linux-foundation.org","subject":"Re: Be more careful about zlib return values","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-21T08:11:57Z","receivedAt":"2007-03-21T08:11:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> When creating a new object, we use \"deflate(stream, Z_FINISH)\" in a loop \n> until it no longer returns Z_OK, and then we do \"deflateEnd()\" to finish \n> up business.\n>\n> That should all work, but the fact is, it's not how you're _supposed_ to \n> use the zlib return values properly:\n> ...\n> Somebody who has worked more with zlib should probably double-check me, \n> but this is what claims is the right thing to do.\n>\n> \t\tLinus\n\nI am observing a very curious performance regression with this\npatch. I do not see anything obviously wrong correctness-wise\nnor performance-wise with it.\n\nThis is driving me crazy, as the benchmark is in that repository\nwith a 1.7GB pack:\n\n\t$ git-rev-list HEAD -- org.eclipse.debug.ui/ >/dev/null\n\nand the patch is ONLY ABOUT write_sha1_file() codepath. The function\nshould not even be called!\n\nWithout the patch (3 typical runs):\n\n16.16user 0.16system 0:16.32elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+79008minor)pagefaults 0swaps\n15.84user 0.14system 0:16.02elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+79009minor)pagefaults 0swaps\n16.04user 0.12system 0:16.17elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+79008minor)pagefaults 0swaps\n\nWith the patch:\n\n17.62user 0.20system 0:17.94elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+79009minor)pagefaults 0swaps\n17.79user 0.19system 0:18.03elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+79011minor)pagefaults 0swaps\n17.40user 0.14system 0:17.63elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k\n0inputs+0outputs (0major+79009minor)pagefaults 0swaps\n\nAnother funny thing is that if I replace the body of\nwrite_sha1_file() with just:\n\nint write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n{\n\tsetup_object_header(NULL, NULL, 0);\n\tdie(\"foo\");\n}\n\nI get somewhat faster number.\n\nI guess I probably should write it off as some curiosity coming\nfrom insn cacheline bouncing between hot functions, that is\ncaused by the size of this unused function changing.\n"},{"id":"37675","messageId":"Pine.LNX.4.64.0703210827591.6730@woody.linux-foundation.org","threadId":"7338","inReplyTo":"7vodmn2eya.fsf@assigned-by-dhcp.cox.net","subject":"Re: Be more careful about zlib return values","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-03-21T15:29:30Z","receivedAt":"2007-03-21T15:29:30Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 21 Mar 2007, Junio C Hamano wrote:\n> \n> I am observing a very curious performance regression with this\n> patch. I do not see anything obviously wrong correctness-wise\n> nor performance-wise with it.\n\nOk, the test you are doing won't call the write function at all, so the \nperformance effect you see is almost certainly due to [un]lucky I$ \nplacement. It happens..\n\n\t\tLinus\n"}]}