* [PATCH] perf_event: fix error handling path @ 2010-12-04 3:19 bookjovi 2010-12-05 12:29 ` Thiago Farina 0 siblings, 1 reply; 7+ messages in thread From: bookjovi @ 2010-12-04 3:19 UTC (permalink / raw) To: a.p.zijlstra, paulus, mingo, acme, linux-kernel; +Cc: Jovi Zhang 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(); exit: mutex_unlock(&callchain_mutex); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path 2010-12-04 3:19 [PATCH] perf_event: fix error handling path bookjovi @ 2010-12-05 12:29 ` Thiago Farina 2010-12-06 1:59 ` jovi zhang 0 siblings, 1 reply; 7+ messages in thread From: Thiago Farina @ 2010-12-05 12:29 UTC (permalink / raw) To: bookjovi; +Cc: a.p.zijlstra, paulus, mingo, acme, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path 2010-12-05 12:29 ` Thiago Farina @ 2010-12-06 1:59 ` jovi zhang 2010-12-08 1:51 ` jovi zhang 0 siblings, 1 reply; 7+ messages in thread From: jovi zhang @ 2010-12-06 1:59 UTC (permalink / raw) To: Thiago Farina; +Cc: a.p.zijlstra, paulus, mingo, acme, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path 2010-12-06 1:59 ` jovi zhang @ 2010-12-08 1:51 ` jovi zhang 2010-12-09 17:06 ` Corey Ashford 0 siblings, 1 reply; 7+ messages in thread From: jovi zhang @ 2010-12-08 1:51 UTC (permalink / raw) To: Thiago Farina; +Cc: a.p.zijlstra, paulus, mingo, acme, linux-kernel 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path 2010-12-08 1:51 ` jovi zhang @ 2010-12-09 17:06 ` Corey Ashford 2010-12-09 17:17 ` Peter Zijlstra 0 siblings, 1 reply; 7+ messages in thread From: Corey Ashford @ 2010-12-09 17:06 UTC (permalink / raw) To: jovi zhang; +Cc: Thiago Farina, a.p.zijlstra, paulus, mingo, acme, linux-kernel 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. - Corey ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path 2010-12-09 17:06 ` Corey Ashford @ 2010-12-09 17:17 ` Peter Zijlstra 2010-12-10 2:27 ` jovi zhang 0 siblings, 1 reply; 7+ messages in thread From: Peter Zijlstra @ 2010-12-09 17:17 UTC (permalink / raw) To: Corey Ashford Cc: jovi zhang, Thiago Farina, paulus, mingo, acme, linux-kernel 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf_event: fix error handling path 2010-12-09 17:17 ` Peter Zijlstra @ 2010-12-10 2:27 ` jovi zhang 0 siblings, 0 replies; 7+ messages in thread From: jovi zhang @ 2010-12-10 2:27 UTC (permalink / raw) To: Peter Zijlstra Cc: Corey Ashford, Thiago Farina, paulus, mingo, acme, linux-kernel On Fri, Dec 10, 2010 at 1:17 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote: > 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(); >> Thanks for you suggestion. I also thought it, but I think it should release the buffers in alloc_callchain_buffers if there have some error(like many other part of kernel code). It's not a good way to trace return error code outside of alloc_callchain_buffer, if there have many return error code in future, maybe need to write code like this: if(err && err != -ENOMEM && err != -EXXX ) ... It's very ugly. is right? >> >> 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? > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-12-10 2:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-04 3:19 [PATCH] perf_event: fix error handling path bookjovi 2010-12-05 12:29 ` Thiago Farina 2010-12-06 1:59 ` jovi zhang 2010-12-08 1:51 ` jovi zhang 2010-12-09 17:06 ` Corey Ashford 2010-12-09 17:17 ` Peter Zijlstra 2010-12-10 2:27 ` jovi zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox