[PATCH weston 2/2] main: don't leak option strings

[Pekka Paalanen]

On Mon, 08 Sep 2014 11:00:37 -0700
Bill Spitzak <spitzak at gmail.com> wrote:

> On 09/07/2014 11:28 PM, Pekka Paalanen wrote:
> 
> >> I think it is safe to assume the contents of argv will not get overwritten.
> >>
> >> It is common to copy argv itself and modify that, but not to modify the
> >> strings it points at.
> >
> > This would be feasible only if we can guarantee that no-one will ever
> > need to store a dynamically allocated string to any of the assigned
> > variables. If something produces a dynamically allocated string, then
> > you would need to track if you can free() the string or not.
> 
> Or you can just leak that dynamically allocated value, which is what I 
> would do.

I wouldn't.

> Trying to shut up valgrind on exit is an exercise in futility and adding 
> a free() to try to shut it up often requires lots of unwanted code 
> changes as this demonstrates.

Yes, there are at least two schools on that. Some say, that a one-time
allocation cannot be a leak. That is true, but only as long as it stays
a one-time allocation. Guaranteeing that is the hard part.

I prefer plugging all reports that can be plugged without corner-case
bugs, so that actual leaks do not get hidden between these. It is very
frustrating to analyze Valgrind reports just to create a suppressions
file so that I might see only the real leaks.

If you leave something unfreed, you have to audit all changes around
that code in the future to make sure they do not turn the harmless leak
into a harmful leak.

I also believe, that not thinking about proper tear-down allows your
architecture to evolve to a stage, where you simply cannot do a proper
tear-down anymore, and some further development then turns that into
real leak.

The question is, which approach one would assume to be more
cost-effective on the long run.

I also favour consistency in coding style. We code all "normal"
functions to not leak, so why not main(), too.

> Any way it is not a big deal and any real system will probably be based 
> on string objects of some sort. It sounds like you should also change 
> the parse_options to copy all the string arguments too.

parse_options() already strdups() all strings it returns, which is one
reason why I would not go removing the strdups.


- pq