{"thread":{"id":"7514","subject":"[PATCH 1/2] Added use of xmalloc() on diff-delta.c","startedAt":"2007-04-04T18:50:08Z","lastAt":"2007-04-05T05:14:12Z","messageCount":5,"participants":["Bruno Ribas","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"38644","messageId":"11757126093105-git-send-email-ribas@c3sl.ufpr.br","threadId":"7514","inReplyTo":null,"subject":"[PATCH 1/2] Added use of xmalloc() on diff-delta.c","fromName":"Bruno Ribas","fromEmail":"ribas@c3sl.ufpr.br","sentAt":"2007-04-04T18:50:08Z","receivedAt":"2007-04-04T18:50:08Z","isPatch":true,"sender":{"key":"ribas@c3sl.ufpr.br","avatar":null},"body":"\nSigned-off-by: Bruno Ribas \n---\n diff-delta.c | 9 +++------\n 1 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex 9f998d0..74a8377 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -157,9 +157,8 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)\n \tmemsize = sizeof(*index) +\n \t\t sizeof(*hash) * hsize +\n \t\t sizeof(*entry) * entries;\n-\tmem = malloc(memsize);\n-\tif (!mem)\n-\t\treturn NULL;\n+\tmem = xmalloc(memsize);\n+\n \tindex = mem;\n \tmem = index + 1;\n \thash = mem;\n@@ -258,9 +257,7 @@ create_delta(const struct delta_index *index,\n \toutsize = 8192;\n \tif (max_size && outsize >= max_size)\n \t\toutsize = max_size + MAX_OP_SIZE + 1;\n-\tout = malloc(outsize);\n-\tif (!out)\n-\t\treturn NULL;\n+\tout = xmalloc(outsize);\n \n \t/* store reference buffer size */\n \ti = index->src_size;\n-- \n1.5.0.3\n"},{"id":"38645","messageId":"11757126101733-git-send-email-ribas@c3sl.ufpr.br","threadId":"7514","inReplyTo":"11757126093105-git-send-email-ribas@c3sl.ufpr.br","subject":"[PATCH 2/2] Removed NULL check on builtin-pack-objects.c from create_delta_index() as it just checks for Out of Memory","fromName":"Bruno Ribas","fromEmail":"ribas@c3sl.ufpr.br","sentAt":"2007-04-04T18:50:09Z","receivedAt":"2007-04-04T18:50:09Z","isPatch":true,"sender":{"key":"ribas@c3sl.ufpr.br","avatar":null},"body":"\nSigned-off-by: Bruno Ribas \n---\n builtin-pack-objects.c | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex b5f9648..04a4abc 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1254,8 +1254,6 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t}\n \tif (!src->index) {\n \t\tsrc->index = create_delta_index(src->data, src_size);\n-\t\tif (!src->index)\n-\t\t\tdie(\"out of memory\");\n \t}\n \n \tdelta_buf = create_delta(src->index, trg->data, trg_size, &delta_size, max_size);\n-- \n1.5.0.3\n"},{"id":"38646","messageId":"7vejn02bcv.fsf@assigned-by-dhcp.cox.net","threadId":"7514","inReplyTo":"11757126093105-git-send-email-ribas@c3sl.ufpr.br","subject":"Re: [PATCH 1/2] Added use of xmalloc() on diff-delta.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-04T19:22:40Z","receivedAt":"2007-04-04T19:22:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These two functions, create_delta_index() and create_delta(),\nare already nicely libified. They allow the caller to deal with\noom condition. The caller may die(), or it may decide to\ncontinue its operation with reduced functionality without using\ndelta data. A good example of this is found a few lines after\nthe lines the second patch touches. When create_delta() cannot\nfind memory to work with, the entire function returns 0, saying\n\"sorry, cannot deltify these two\", which would cause the object\nstored without deltification.\n\nThese patches take that nice property away, making libification\nmore difficult, which is the downside. Is there an upside?\n\nIf anything, I suspect that the part that calls die() you\ntouched in the second patch could return NULL.\n"},{"id":"38661","messageId":"Pine.LNX.4.64.0704041525220.6730@woody.linux-foundation.org","threadId":"7514","inReplyTo":"7vejn02bcv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/2] Added use of xmalloc() on diff-delta.c","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-04T22:31:21Z","receivedAt":"2007-04-04T22:31:21Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 4 Apr 2007, Junio C Hamano wrote:\n> \n> These patches take that nice property away, making libification\n> more difficult, which is the downside. Is there an upside?\n\nWell, we could just make the libification rule very simple:\n\n - the library does *not* include \"xmalloc()\", and you have to handle \n out-of-memory situations yourself inside the xmalloc() that *you* as a \n libification user provide!.\n\nThen, we just make our xmalloc() be non-inlined (which we should do \n*anyway* - it's long since grown so big that it shouldn't be inlined in \nthe first place), and we make it part of a non-library git object file.\n\nOther libgit uses might end up doing something like\n\n\t\t..\n\t\tif (sigsetjump(buffer, 1)) {\n\t\t\tshow_oom_message();\n\t\t..\n\n\tvoid *xmalloc(size_t size)\n\t{\n\t\tvoid *ret = malloc(size ? size : 1);\n\t\tif (!ret)\n\t\t\tsiglongjmp(buffer);\n\t\treturn ret;\n\t}\n\nor, if they use C++ exception handling, they'd just make their own \nxmalloc() raise an exception, and have the callers catch it.\n\nThe point being that this is what you'd need to do *anyway*, and trying to \nmake all the library routines return NULL or some other error case is just \nworse programming practice than just having a xmalloc() that dies by \ndefault but that can be overridden.\n\n\t\tLinus\n"},{"id":"38671","messageId":"7vy7l7wggr.fsf@assigned-by-dhcp.cox.net","threadId":"7514","inReplyTo":"Pine.LNX.4.64.0704041525220.6730@woody.linux-foundation.org","subject":"Re: [PATCH 1/2] Added use of xmalloc() on diff-delta.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-05T05:14:12Z","receivedAt":"2007-04-05T05:14:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Wed, 4 Apr 2007, Junio C Hamano wrote:\n>> \n>> These patches take that nice property away, making libification\n>> more difficult, which is the downside. Is there an upside?\n>\n> Well, we could just make the libification rule very simple:\n>\n> - the library does *not* include \"xmalloc()\", and you have to handle \n> out-of-memory situations yourself inside the xmalloc() that *you* as a \n> libification user provide!.\n>\n> Then, we just make our xmalloc() be non-inlined (which we should do \n> *anyway* - it's long since grown so big that it shouldn't be inlined in \n> the first place), and we make it part of a non-library git object file.\n\nI agree that is probably a sane thing to do for existing callers\nof xmalloc() -- the callers are not prepared to handle\n(near-)oom case gracefully to begin with.\n\nYour \"libified git decides how xmalloc() copes with (near-)oom\ncondition gracefully\" is much better than \"memory-allocating\nfunction in git declares that oom in xmalloc() is fatal\", which\nis what we currently have.\n\nI however think it is an independent issue, wrt the part Bruno's\npatch touches. In the case of this particular call chain\nbetween create_delta()/create_delta_index() and its caller, I\nthink the code that is there allows nicer arrangement. The\ncaller could instead easily attempt to cope with (near-)oom\ncondition more gracefully. And that was my suggestion about\nreturning 0 when delta_index cannot be built instead of dying.\n\nOf course, this caller is in memory-hungry \"pack-objects\", and\nall of the above is mostly academic, as failure to allocate\nmemory in create_delta_index() would most likely mean you would\nhave trouble allocating memory for other more important data.\n"}]}