lkml.org 
[lkml]   [2013]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] tracing: fix referencing after memory freeing and refactors code
From
2013/10/17 Steven Rostedt <rostedt@goodmis.org>:
> On Thu, 17 Oct 2013 22:44:56 -0300
> "Geyslan G. Bem" <geyslan@gmail.com> wrote:
>
>> Restructures function logic conditions testing 'tracing_open_generic'
>> return before the others. It avoids: unnecessary trace_array_get and
>> kzalloc when tracing is disabled; and fix the possible 'dir'
>> assignment after freeing it.
>>
>> Centralizes the exiting, ensuring the 'trace_array_put' on error,
>> in accordance to Coding Style, Chapter 7.
>>
>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> ---
>> kernel/trace/trace_events.c | 22 ++++++++++++----------
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 368a4d5..8232362 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -1108,26 +1108,28 @@ static int system_tr_open(struct inode *inode, struct file *filp)
>> struct trace_array *tr = inode->i_private;
>> int ret;
>>
>> - if (trace_array_get(tr) < 0)
>> - return -ENODEV;
>> + ret = tracing_open_generic(inode, filp);
>> + if (ret)
>> + return ret;
>> +
>> + ret = trace_array_get(tr);
>> + if (ret)
>> + return ret;
>
> Hmm,
>
> I'm thinking of just nuking the tracing_open_generic() here. The only
> thing it does here is the tracing_disabled check. The assignment of
> inode->i_private to filp->private_data is pointless as it just
> reassigns it anyway.
>
> We could add a tracing_is_disabled() function to test instead.

Nice, I can do it.

Questions:

1267 static const struct file_operations ftrace_enable_fops = {
1268 .open = tracing_open_generic,
...
1286 static const struct file_operations ftrace_event_filter_fops = {
1287 .open = tracing_open_generic,
...
1317 static const struct file_operations ftrace_show_header_fops = {
1318 .open = tracing_open_generic,

Are that structures in same case? Their 'open' can be replaced to the
new 'tracing_is_disabled()?

I think that 'subsystem_open()' can be also refactored to use the
about to rise 'tracing_is_disabled()'. Am I right?
1096 ret = tracing_open_generic(inode, filp);

Regards,
-- Geyslan

>
> -- Steve
>
>>
>> /* Make a temporary dir that has no system but points to tr */
>> dir = kzalloc(sizeof(*dir), GFP_KERNEL);
>> if (!dir) {
>> - trace_array_put(tr);
>> - return -ENOMEM;
>> + ret = -ENOMEM;
>> + goto err_dir;
>> }
>>
>> dir->tr = tr;
>>
>> - ret = tracing_open_generic(inode, filp);
>> - if (ret < 0) {
>> - trace_array_put(tr);
>> - kfree(dir);
>> - }
>> -
>> filp->private_data = dir;
>> + return 0;
>>
>> +err_dir:
>> + trace_array_put(tr);
>> return ret;
>> }
>>
>


\
 
 \ /
  Last update: 2013-10-18 12:01    [W:0.087 / U:5.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site