Re: [PATCH] perf_event: fix error handling path

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Thu, 2010-12-09 at 09:06 -0800, Corey Ashford wrote:
> On 12/07/2010 05:51 PM, jovi zhang wrote:
> > On Mon, Dec 6, 2010 at 9:59 AM, jovi zhang<bookjovi@gmail.com>  wrote:
> >> On Sun, Dec 5, 2010 at 8:29 PM, Thiago Farina<tfransosi@gmail.com>  wrote:
> >>>
> >>> On Sat, Dec 4, 2010 at 1:19 AM,<bookjovi@gmail.com>  wrote:
> >>>> fix error handling path
> >>>>
> >>>> Signed-off-by: Jovi Zhang<bookjovi@gmail.com>
> >>>>   kernel/perf_event.c |    2 --
> >>>>   1 files changed, 0 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> >>>> index cb6c0d2..62f9e9d 100644
> >>>> --- a/kernel/perf_event.c
> >>>> +++ b/kernel/perf_event.c
> >>>> @@ -1918,8 +1918,6 @@ static int get_callchain_buffers(void)
> >>>>         }
> >>>>
> >>>>         err = alloc_callchain_buffers();
> >>>> -       if (err)
> >>>> -               release_callchain_buffers();
> >>>
> >>> Care to explain in the change log message? As I reader, is not clear
> >>> to me what is wrong with this.
> >>
> >> Sorry, the description should be as:
> >> fix error handling path. alloc_callchain_buffers() can return -ENOMEM,
> >> in this time callchain_cpus_entries maybe is NULL, It will oops if
> >> invoke release_callchain_buffers() when callchain_cpus_entries is
> >> NULL.
> >>
> > I hope my understanding is right, is it?
> 
> One possible problem here is what if it returns an error other than 
> -ENOMEM, and the buffers do need to be released?  Maybe you could change 
> the code to
> 
> err = alloc_callchain_buffers();
> if (err != -ENOMEM)
> 	release_callchain_buffers();
> 
> 
> Currently, alloc_callchain_buffers cannot return any error code other 
> than -ENOMEM, but that might change in the future.

The kernel convention is to fully clean up after yourself if you return
an error. So in that sense the patch seems right.

Anyway, anybody care to post a new patch with slightly extended
changelog?