From: Jim Meyering Date: Mon, 15 Jun 2009 11:49:09 GMT Subject: Re: [PATCH] use xstrdup, not strdup in ll-merge.c Message-ID: <877hzdlnt6.fsf@meyering.net> In-Reply-To: <81b0412b0906150439r755a7498ldddbca911b2fd36e@mail.gmail.com> Alex Riesen wrote: > 2009/6/15 Jim Meyering : >> Alex Riesen wrote: >>> 2009/6/15 Jim Meyering : >>>> Alex Riesen wrote: >>>>> 2009/6/14 Jim Meyering : >>>>>> @@ -231,7 +231,7 @@ static int read_merge_config(const char *var, const char *value, void *cb) >>>>>> >>>>>>        if (!strcmp(var, "merge.default")) { >>>>>>                if (value) >>>>>> -                       default_ll_merge = strdup(value); >>>>>> +                       default_ll_merge = xstrdup(value); >>>>> >>>>> read_merge_config has a failure mode (where it returns -1), why not use it? >>>> >>>> I didn't even consider it, because it would be inconsistent with >>>> the other heap-allocation functions used there (xcalloc, xmemdupz). >>>> >>>> However, now that I do, it looks like that would mean adding four times >>>> the same code (including conditionals and code to generate a diagnostic via >>>> a call to error -- or a goto). Why bother, when all of that is already >>>> encapsulated in xmalloc? >>> >>> So that a useful error message can be given in the _caller_ (it knows >>> more about context)? >> >> So you want to tell the user that we failed >> to strdup the "merge.default" value? >> Or the "driver" value? > > "merge: recursive: error loading configuration (last seen: > merge.default): Out of memory\n" > >> Of more general interest, when xstrdup fails, it might be useful to >> include in the diagnostic how long the would-be-dup'd string was.  I.e., >> rather than saying >> >>    die("Out of memory, strdup failed"); >> say >>    die("Out of memory, failed to strdup a %lu-byte string", >>        (unsigned long int) strlen(str)); > > Yes. Still lacks higher level information, though. > >>> Otherwise the error message ("Out of memory, strdup failed") does not >>> have anything about the place nor situation in it. As the situations >>> when a modern system really runs out of memory are very rare, >>> mostly such reports just point at some inconsistency elsewhere >> >> Exactly.  This is why I think it's not worthwhile to invest in >> a more precise diagnostic, here. > > I disagree. It is already hard to find starting point for debugging if > the failed code is just a layer: the config of ll-merge is called not only > from the merge drivers, but also indirectly from the programs which > call the merge itself. Now, go figure where has it failed... If you're convinced of the value of such a change, go for it. Though it sounds like you're saying you'd prefer a stack trace.