Re: [PATCH 2/2] perf trace: Fix segfault on perf trace -i perf.data

[Arnaldo Carvalho de Melo <acme@ghostprotocols.net>]

Em Tue, Nov 12, 2013 at 09:27:23PM +0000, Namhyung Kim escreveu:
> Hi Arnaldo,
> 
> On Tue, Nov 12, 2013 at 08:57:00AM -0300, Arnaldo Carvalho de Melo wrote:
> > So this becomes the first part of this patch, split from yours and
> > massaged a bit so that by looking at the patch it becomes quickly clear
> > what it is doing, please let me now if I can keep this as-is (with your
> > authorship, etc).
> 
> Looks good to me.

Thanks for checking!
 
> But I just have a nitpick, please see below.
 
> > +{
> > +	evsel->priv = malloc(sizeof(struct syscall_tp));
> > +	if (evsel->priv != NULL) {
> > +		if (perf_evsel__init_sc_tp_uint_field(evsel, id))
> > +			goto out_delete;
> > +
> > +		evsel->handler = handler;
> > +		return 0;
> > +	}
> > +
> > +	return -ENOMEM;
> > +
> > +out_delete:
> > +	free(evsel->priv);
> > +	evsel->priv = NULL;
 
> Is this part needed?  I can see that perf_evsel__delete_priv() can do
> it for you anyway.  Yes I know it's needed for my later change, but I
> think we do it a bit differently.
 
> And again, is perf_evsel__delete_priv() needed?  Isn't the ->priv is
> not used for anything else?  Why not just letting perf_evsel__delete()
> handle this transparently?

Because it may point to something not allocated via malloc, so the one
who allocates it, frees it, furthermore, the one who allocates it, if
fails to complete the greater init sequence of which the allocation is
part of, frees it and leaves it as it was before the transaction
started.
 
- Arnaldo