Re: [PATCH] use xstrdup, not strdup in ll-merge.c

[Alex Riesen <raa.lkml@gmail.com>]

2009/6/15 Jim Meyering <jim@meyering.net>:
> Alex Riesen wrote:
>> 2009/6/15 Jim Meyering <jim@meyering.net>:
>>> Alex Riesen wrote:
>>>> 2009/6/14 Jim Meyering <jim@meyering.net>:
>>>>> @@ -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...

>> (like bloody stupid memory management in system support libraries
>> on an OS-not-to-be-named-again or the usual corruption of heap
>> control structures).
>>
>> Besides, xstrdup does more then just allocation: it tries to free global
>> list of cached pack chunks. This does not play very well with the efforts
>> to make a library out of the modern Git code.
>
> Ahh... librarification.  This is a slightly different topic.
> I see existing uses of xcalloc and xmemdupz, not to
> mention "error" calls, and conclude that this function is
> not library caliber code, so there's no need to invest.

Well, error() does not finish the programs, and the rest (in just my opinion)
suffer the same problems except where it is used in cmd_-functions
(IOW, in top-level caller).

> If you want a version of this function that is more library-friendly,
> then that will be more work.  However, I think librarification should
> be addressed separately from this simple patch to avoid a potential NULL
> dereference (and *no* diagnostic).

I do not explicitely _want_ librarification in this particular context.
I'm just pointing out that you just made another (maybe small) obstacle for it.