On 09/26, Sylvain 'ythier' Hitier wrote:
>
> retval = sched_fork(clone_flags, p);
> if (retval)
> // // mustn't perf_event_free_task()
> goto bad_fork_cleanup_policy;
Agreed, this is wrong. Good catch.
but, unless I missed something,
> retval = perf_event_init_task(p);
> if (retval)
> // // mustn't perf_event_free_task()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
this is not right and thus the patch is not right too.
Suppose that perf_event_init_task() -> perf_event_init_context(ctxn => 0)
succeeds and then perf_event_init_context(ctxn => 1) fails, we need
perf_event_free_task() to cleanup ->perf_event_ctxp[0].
So if perf_event_init_task() fails, we still need "goto bad_fork_cleanup_perf".
No?
Or, probably better, we need to change perf_event_init_context() to call
perf_event_free_task() on failure.
Or. We can simply move memset(child->perf_event_ctxp, 0, ...) from
perf_event_init_context() up. This reminds that we really need to cleanup
copy_process(), in particular I think it asks for the new copy_xxx() helper
which should do misc simple initializations which can't fail.
What do you think?
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
