{"thread":{"id":"19811","subject":"[PATCH] use xstrdup, not strdup in ll-merge.c","startedAt":"2009-06-14T19:47:54Z","lastAt":"2009-06-15T15:21:11Z","messageCount":10,"participants":["Jim Meyering","Alex Riesen","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"116289","messageId":"87bpoqoavp.fsf@meyering.net","threadId":"19811","inReplyTo":null,"subject":"[PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-06-14T19:47:54Z","receivedAt":"2009-06-14T19:47:54Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"\nOtherwise, a fluky allocation failure would cause merge\nconfiguration settings to be silently ignored.\n\nSigned-off-by: Jim Meyering \n---\n ll-merge.c | 8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 31d6f0a..9168958 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n\n \tif (!strcmp(var, \"merge.default\")) {\n \t\tif (value)\n-\t\t\tdefault_ll_merge = strdup(value);\n+\t\t\tdefault_ll_merge = xstrdup(value);\n \t\treturn 0;\n \t}\n\n@@ -265,7 +265,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"name\", ep)) {\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\n-\t\tfn->description = strdup(value);\n+\t\tfn->description = xstrdup(value);\n \t\treturn 0;\n \t}\n\n@@ -288,14 +288,14 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n \t\t * file named by %A, and signal that it has done with zero exit\n \t\t * status.\n \t\t */\n-\t\tfn->cmdline = strdup(value);\n+\t\tfn->cmdline = xstrdup(value);\n \t\treturn 0;\n \t}\n\n \tif (!strcmp(\"recursive\", ep)) {\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\n-\t\tfn->recursive = strdup(value);\n+\t\tfn->recursive = xstrdup(value);\n \t\treturn 0;\n \t}\n\n--\n1.6.3.2.406.gd6a466\n"},{"id":"116295","messageId":"81b0412b0906141503v14484d9fyea56198910305bfc@mail.gmail.com","threadId":"19811","inReplyTo":"87bpoqoavp.fsf@meyering.net","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-14T22:03:27Z","receivedAt":"2009-06-14T22:03:27Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/6/14 Jim Meyering :\n> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n>\n>        if (!strcmp(var, \"merge.default\")) {\n>                if (value)\n> -                       default_ll_merge = strdup(value);\n> +                       default_ll_merge = xstrdup(value);\n\nread_merge_config has a failure mode (where it returns -1), why not use it?\n"},{"id":"116320","messageId":"87fxe2lybr.fsf@meyering.net","threadId":"19811","inReplyTo":"81b0412b0906141503v14484d9fyea56198910305bfc@mail.gmail.com","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-06-15T08:02:00Z","receivedAt":"2009-06-15T08:02:00Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Alex Riesen wrote:\n> 2009/6/14 Jim Meyering :\n>> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n>>\n>>        if (!strcmp(var, \"merge.default\")) {\n>>                if (value)\n>> -                       default_ll_merge = strdup(value);\n>> +                       default_ll_merge = xstrdup(value);\n>\n> read_merge_config has a failure mode (where it returns -1), why not use it?\n\nI didn't even consider it, because it would be inconsistent with\nthe other heap-allocation functions used there (xcalloc, xmemdupz).\n\nHowever, now that I do, it looks like that would mean adding four times\nthe same code (including conditionals and code to generate a diagnostic via\na call to error -- or a goto). Why bother, when all of that is already\nencapsulated in xmalloc?\nMaybe because you want to be able to continue after an allocation failure?\nIf a small strdup allocation fails, odds are good that the code won't\nbe able to do anything useful, so when not in library code, cleanest is\nsimply to exit.\n\nIn addition, if you insist on using strdup, you'll probably want to\nbe consistent and use calloc and memdupz, too. Adding all of the code\nrequired to recover from those failures and to avoid leaks would be messy.\n"},{"id":"116322","messageId":"81b0412b0906150145j7c717a6ar33bb4f2ebd6095e1@mail.gmail.com","threadId":"19811","inReplyTo":"87fxe2lybr.fsf@meyering.net","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-15T08:45:22Z","receivedAt":"2009-06-15T08:45:22Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/6/15 Jim Meyering :\n> Alex Riesen wrote:\n>> 2009/6/14 Jim Meyering :\n>>> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n>>>\n>>>        if (!strcmp(var, \"merge.default\")) {\n>>>                if (value)\n>>> -                       default_ll_merge = strdup(value);\n>>> +                       default_ll_merge = xstrdup(value);\n>>\n>> read_merge_config has a failure mode (where it returns -1), why not use it?\n>\n> I didn't even consider it, because it would be inconsistent with\n> the other heap-allocation functions used there (xcalloc, xmemdupz).\n>\n> However, now that I do, it looks like that would mean adding four times\n> the same code (including conditionals and code to generate a diagnostic via\n> a call to error -- or a goto). Why bother, when all of that is already\n> encapsulated in xmalloc?\n\nSo that a useful error message can be given in the _caller_ (it knows\nmore about context)?\n\nOtherwise the error message (\"Out of memory, strdup failed\") does not\nhave anything about the place nor situation in it. As the situations\nwhen a modern system really runs out of memory are very rare,\nmostly such reports just point at some inconsistency elsewhere\n(like bloody stupid memory management in system support libraries\non an OS-not-to-be-named-again or the usual corruption of heap\ncontrol structures).\n\nBesides, xstrdup does more then just allocation: it tries to free global\nlist of cached pack chunks. This does not play very well with the efforts\nto make a library out of the modern Git code.\n\n> Maybe because you want to be able to continue after an allocation failure?\n\nNo.\n\n> If a small strdup allocation fails, odds are good that the code won't\n> be able to do anything useful, so when not in library code, cleanest is\n> simply to exit.\n\nDoubt it (because you better describe _why_ you \"simply\" interrupted\nusers workflow, so the said user can do something about it).\n\n> In addition, if you insist on using strdup, you'll probably want to\n> be consistent and use calloc and memdupz, too.  Adding all of the code\n> required to recover from those failures and to avoid leaks would be messy.\n\nI don't insist on that. I should have said in the first message about\nmore elaborate explanation of the error to user. Sorry.\n"},{"id":"116325","messageId":"87vdmxluj8.fsf@meyering.net","threadId":"19811","inReplyTo":"81b0412b0906150145j7c717a6ar33bb4f2ebd6095e1@mail.gmail.com","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-06-15T09:23:55Z","receivedAt":"2009-06-15T09:23:55Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Alex Riesen wrote:\n> 2009/6/15 Jim Meyering :\n>> Alex Riesen wrote:\n>>> 2009/6/14 Jim Meyering :\n>>>> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n>>>>\n>>>>        if (!strcmp(var, \"merge.default\")) {\n>>>>                if (value)\n>>>> -                       default_ll_merge = strdup(value);\n>>>> +                       default_ll_merge = xstrdup(value);\n>>>\n>>> read_merge_config has a failure mode (where it returns -1), why not use it?\n>>\n>> I didn't even consider it, because it would be inconsistent with\n>> the other heap-allocation functions used there (xcalloc, xmemdupz).\n>>\n>> However, now that I do, it looks like that would mean adding four times\n>> the same code (including conditionals and code to generate a diagnostic via\n>> a call to error -- or a goto). Why bother, when all of that is already\n>> encapsulated in xmalloc?\n>\n> So that a useful error message can be given in the _caller_ (it knows\n> more about context)?\n\nSo you want to tell the user that we failed\nto strdup the \"merge.default\" value?\nOr the \"driver\" value?\n\nGiven the apparently-high cost/benefit ratio, I would not bother. I.e.,\nthis failure is so unlikely to trigger, and when it does, knowing for which\nconfig value strdup failed is even less likely to be useful,\nthat minimal diagnostics should be fine.\n\nOf more general interest, when xstrdup fails, it might be useful to\ninclude in the diagnostic how long the would-be-dup'd string was. I.e.,\nrather than saying\n\n die(\"Out of memory, strdup failed\");\nsay\n die(\"Out of memory, failed to strdup a %lu-byte string\",\n (unsigned long int) strlen(str));\n\nThen, you have at least a clue as to whether the failure\nis due to some ridiculously-long string value, or to some\nunrelated-to-config, systemic problem.\n\n> Otherwise the error message (\"Out of memory, strdup failed\") does not\n> have anything about the place nor situation in it. As the situations\n> when a modern system really runs out of memory are very rare,\n> mostly such reports just point at some inconsistency elsewhere\n\nExactly. This is why I think it's not worthwhile to invest in\na more precise diagnostic, here.\n\n> (like bloody stupid memory management in system support libraries\n> on an OS-not-to-be-named-again or the usual corruption of heap\n> control structures).\n>\n> Besides, xstrdup does more then just allocation: it tries to free global\n> list of cached pack chunks. This does not play very well with the efforts\n> to make a library out of the modern Git code.\n\nAhh... librarification. This is a slightly different topic.\nI see existing uses of xcalloc and xmemdupz, not to\nmention \"error\" calls, and conclude that this function is\nnot library caliber code, so there's no need to invest.\n\nIf you want a version of this function that is more library-friendly,\nthen that will be more work. However, I think librarification should\nbe addressed separately from this simple patch to avoid a potential NULL\ndereference (and *no* diagnostic).\n"},{"id":"116327","messageId":"81b0412b0906150439r755a7498ldddbca911b2fd36e@mail.gmail.com","threadId":"19811","inReplyTo":"87vdmxluj8.fsf@meyering.net","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-15T11:39:08Z","receivedAt":"2009-06-15T11:39:08Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/6/15 Jim Meyering :\n> Alex Riesen wrote:\n>> 2009/6/15 Jim Meyering :\n>>> Alex Riesen wrote:\n>>>> 2009/6/14 Jim Meyering :\n>>>>> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n>>>>>\n>>>>>        if (!strcmp(var, \"merge.default\")) {\n>>>>>                if (value)\n>>>>> -                       default_ll_merge = strdup(value);\n>>>>> +                       default_ll_merge = xstrdup(value);\n>>>>\n>>>> read_merge_config has a failure mode (where it returns -1), why not use it?\n>>>\n>>> I didn't even consider it, because it would be inconsistent with\n>>> the other heap-allocation functions used there (xcalloc, xmemdupz).\n>>>\n>>> However, now that I do, it looks like that would mean adding four times\n>>> the same code (including conditionals and code to generate a diagnostic via\n>>> a call to error -- or a goto). Why bother, when all of that is already\n>>> encapsulated in xmalloc?\n>>\n>> So that a useful error message can be given in the _caller_ (it knows\n>> more about context)?\n>\n> So you want to tell the user that we failed\n> to strdup the \"merge.default\" value?\n> Or the \"driver\" value?\n\n\"merge: recursive: error loading configuration (last seen:\nmerge.default): Out of memory\\n\"\n\n> Of more general interest, when xstrdup fails, it might be useful to\n> include in the diagnostic how long the would-be-dup'd string was.  I.e.,\n> rather than saying\n>\n>    die(\"Out of memory, strdup failed\");\n> say\n>    die(\"Out of memory, failed to strdup a %lu-byte string\",\n>        (unsigned long int) strlen(str));\n\nYes. Still lacks higher level information, though.\n\n>> Otherwise the error message (\"Out of memory, strdup failed\") does not\n>> have anything about the place nor situation in it. As the situations\n>> when a modern system really runs out of memory are very rare,\n>> mostly such reports just point at some inconsistency elsewhere\n>\n> Exactly.  This is why I think it's not worthwhile to invest in\n> a more precise diagnostic, here.\n\nI disagree. It is already hard to find starting point for debugging if\nthe failed code is just a layer: the config of ll-merge is called not only\nfrom the merge drivers, but also indirectly from the programs which\ncall the merge itself. Now, go figure where has it failed...\n\n>> (like bloody stupid memory management in system support libraries\n>> on an OS-not-to-be-named-again or the usual corruption of heap\n>> control structures).\n>>\n>> Besides, xstrdup does more then just allocation: it tries to free global\n>> list of cached pack chunks. This does not play very well with the efforts\n>> to make a library out of the modern Git code.\n>\n> Ahh... librarification.  This is a slightly different topic.\n> I see existing uses of xcalloc and xmemdupz, not to\n> mention \"error\" calls, and conclude that this function is\n> not library caliber code, so there's no need to invest.\n\nWell, error() does not finish the programs, and the rest (in just my opinion)\nsuffer the same problems except where it is used in cmd_-functions\n(IOW, in top-level caller).\n\n> If you want a version of this function that is more library-friendly,\n> then that will be more work.  However, I think librarification should\n> be addressed separately from this simple patch to avoid a potential NULL\n> dereference (and *no* diagnostic).\n\nI do not explicitely _want_ librarification in this particular context.\nI'm just pointing out that you just made another (maybe small) obstacle for it.\n"},{"id":"116328","messageId":"877hzdlnt6.fsf@meyering.net","threadId":"19811","inReplyTo":"81b0412b0906150439r755a7498ldddbca911b2fd36e@mail.gmail.com","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-06-15T11:49:09Z","receivedAt":"2009-06-15T11:49:09Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Alex Riesen wrote:\n\n> 2009/6/15 Jim Meyering :\n>> Alex Riesen wrote:\n>>> 2009/6/15 Jim Meyering :\n>>>> Alex Riesen wrote:\n>>>>> 2009/6/14 Jim Meyering :\n>>>>>> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n>>>>>>\n>>>>>>        if (!strcmp(var, \"merge.default\")) {\n>>>>>>                if (value)\n>>>>>> -                       default_ll_merge = strdup(value);\n>>>>>> +                       default_ll_merge = xstrdup(value);\n>>>>>\n>>>>> read_merge_config has a failure mode (where it returns -1), why not use it?\n>>>>\n>>>> I didn't even consider it, because it would be inconsistent with\n>>>> the other heap-allocation functions used there (xcalloc, xmemdupz).\n>>>>\n>>>> However, now that I do, it looks like that would mean adding four times\n>>>> the same code (including conditionals and code to generate a diagnostic via\n>>>> a call to error -- or a goto). Why bother, when all of that is already\n>>>> encapsulated in xmalloc?\n>>>\n>>> So that a useful error message can be given in the _caller_ (it knows\n>>> more about context)?\n>>\n>> So you want to tell the user that we failed\n>> to strdup the \"merge.default\" value?\n>> Or the \"driver\" value?\n>\n> \"merge: recursive: error loading configuration (last seen:\n> merge.default): Out of memory\\n\"\n>\n>> Of more general interest, when xstrdup fails, it might be useful to\n>> include in the diagnostic how long the would-be-dup'd string was.  I.e.,\n>> rather than saying\n>>\n>>    die(\"Out of memory, strdup failed\");\n>> say\n>>    die(\"Out of memory, failed to strdup a %lu-byte string\",\n>>        (unsigned long int) strlen(str));\n>\n> Yes. Still lacks higher level information, though.\n>\n>>> Otherwise the error message (\"Out of memory, strdup failed\") does not\n>>> have anything about the place nor situation in it. As the situations\n>>> when a modern system really runs out of memory are very rare,\n>>> mostly such reports just point at some inconsistency elsewhere\n>>\n>> Exactly.  This is why I think it's not worthwhile to invest in\n>> a more precise diagnostic, here.\n>\n> I disagree. It is already hard to find starting point for debugging if\n> the failed code is just a layer: the config of ll-merge is called not only\n> from the merge drivers, but also indirectly from the programs which\n> call the merge itself. Now, go figure where has it failed...\n\nIf you're convinced of the value of such a change, go for it.\nThough it sounds like you're saying you'd prefer a stack trace.\n"},{"id":"116332","messageId":"81b0412b0906150546p34ff53ceo81ea59785cf9d18a@mail.gmail.com","threadId":"19811","inReplyTo":"877hzdlnt6.fsf@meyering.net","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-15T12:46:04Z","receivedAt":"2009-06-15T12:46:04Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/6/15 Jim Meyering :\n> Alex Riesen wrote:\n>> 2009/6/15 Jim Meyering :\n>>>\n>>> Exactly.  This is why I think it's not worthwhile to invest in\n>>> a more precise diagnostic, here.\n>>\n>> I disagree. It is already hard to find starting point for debugging if\n>> the failed code is just a layer: the config of ll-merge is called not only\n>> from the merge drivers, but also indirectly from the programs which\n>> call the merge itself. Now, go figure where has it failed...\n>\n> If you're convinced of the value of such a change, go for it.\n\nAs much as I'd like to know as much as possible about why something\nfailed, I can't make a failure handling automatically simple (given the tools).\nI do suggest using goto to handle this particular OOM (below), but it looks\nalmost too ugly. Maybe I am just paranoid, and am overdoing this particular\ncase.\n\n> Though it sounds like you're saying you'd prefer a stack trace.\n\nI haven't though of that, but yes, that would be perfect (except for\na small problem where it is impossible to do in a portable way,\nand there is know way to get information about).\n\nThe \"goto oom\" patch:\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 31d6f0a..4977f20 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -230,8 +230,11 @@ static int read_merge_config(const char *var,\nconst char *value, void *cb)\n \tint namelen;\n\n \tif (!strcmp(var, \"merge.default\")) {\n-\t\tif (value)\n+\t\tif (value) {\n \t\t\tdefault_ll_merge = strdup(value);\n+\t\t\tif (!default_ll_merge)\n+\t\t\t\tgoto oom;\n+\t\t}\n \t\treturn 0;\n \t}\n\n@@ -266,6 +269,8 @@ static int read_merge_config(const char *var,\nconst char *value, void *cb)\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\n \t\tfn->description = strdup(value);\n+\t\tif (!fn->description)\n+\t\t\tgoto oom;\n \t\treturn 0;\n \t}\n\n@@ -289,6 +294,8 @@ static int read_merge_config(const char *var,\nconst char *value, void *cb)\n \t\t * status.\n \t\t */\n \t\tfn->cmdline = strdup(value);\n+\t\tif (!fn->cmdline)\n+\t\t\tgoto oom;\n \t\treturn 0;\n \t}\n\n@@ -296,10 +303,15 @@ static int read_merge_config(const char *var,\nconst char *value, void *cb)\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\n \t\tfn->recursive = strdup(value);\n+\t\tif (!fn->recursive)\n+\t\t\tgoto oom;\n \t\treturn 0;\n \t}\n\n \treturn 0;\n+oom:\n+\treturn error(\"line merge: %s%s%s: out of memory\", var,\n+\t\t value ? \" = \": \"\", value ? value: \"\");\n }\n\n static void initialize_ll_merge(void)\n@@ -307,7 +319,8 @@ static void initialize_ll_merge(void)\n \tif (ll_user_merge_tail)\n \t\treturn;\n \tll_user_merge_tail = &ll_user_merge;\n-\tgit_config(read_merge_config, NULL);\n+\tif (git_config(read_merge_config, NULL) < 0)\n+\t\texit(1);\n }\n\n static const struct ll_merge_driver *find_ll_merge_driver(const char\n*merge_attr)\n"},{"id":"116338","messageId":"20090615142648.GV16497@spearce.org","threadId":"19811","inReplyTo":"81b0412b0906150546p34ff53ceo81ea59785cf9d18a@mail.gmail.com","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-06-15T14:26:48Z","receivedAt":"2009-06-15T14:26:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Alex Riesen wrote:\n> 2009/6/15 Jim Meyering :\n> > Alex Riesen wrote:\n> >> 2009/6/15 Jim Meyering :\n> >>>\n> >>> Exactly. ??This is why I think it's not worthwhile to invest in\n> >>> a more precise diagnostic, here.\n\nSorry Alex, but I have to agree with Jim here. Use xstrdup()\nand its friends and move on with life.\n\n> As much as I'd like to know as much as possible about why something\n> failed, I can't make a failure handling automatically simple (given the tools).\n> I do suggest using goto to handle this particular OOM (below), but it looks\n> almost too ugly. Maybe I am just paranoid, and am overdoing this particular\n> case.\n\nYes, this is horrible. Its a huge amount of code for checking a\nstrdup failure that is unlikely to occur out of a configuration file.\n\nReally, how big is that config file? It can't be so large that a\nstrdup() is likely to run out of memory pulling a value from it.\n\nAnd even if it does run out of memory, unless we can reclaim space\nby shedding pack windows, we're doomed, we can't continue executing.\nThe official error is \"out of memory\", most likely due to too low\nof a rlimit on our heap, and the only resolution is to boost the\nrlimit higher, not something we can do in our process if we're\nalready hitting the hard limit.\n \n> The \"goto oom\" patch:\n\nNAK. We don't do this anywhere else in git. Please don't start now.\n \n-- \nShawn.\n"},{"id":"116344","messageId":"81b0412b0906150821x30eb621ctcddc7691192b5270@mail.gmail.com","threadId":"19811","inReplyTo":"20090615142648.GV16497@spearce.org","subject":"Re: [PATCH] use xstrdup, not strdup in ll-merge.c","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2009-06-15T15:21:11Z","receivedAt":"2009-06-15T15:21:11Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2009/6/15 Shawn O. Pearce :\n> Alex Riesen wrote:\n>> As much as I'd like to know as much as possible about why something\n>> failed, I can't make a failure handling automatically simple (given the tools).\n>> I do suggest using goto to handle this particular OOM (below), but it looks\n>> almost too ugly. Maybe I am just paranoid, and am overdoing this particular\n>> case.\n>\n> Yes, this is horrible.  Its a huge amount of code for checking a\n> strdup failure that is unlikely to occur out of a configuration file.\n>\n> Really, how big is that config file?  It can't be so large that a\n> strdup() is likely to run out of memory pulling a value from it.\n\nOh, I don't worry about config files being too big (it is noticeable\nwithout this checks). I'm worried about our bugs triggering the\nconditions where allocation routines return NULL, like heap corruption\nwhich is hard to notice without valgrind.\n\n>> The \"goto oom\" patch:\n>\n> NAK.  We don't do this anywhere else in git.  Please don't start now.\n\nWe'll have to start paying more attention to memory management sometime.\nAlthough, this particular case may be not the best case for it.\n"}]}