Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_func succeeded
On Thu, 2013-05-09 at 14:44 +0900, Masami Hiramatsu wrote: > Return 0 instead of the number of activated ftrace if > event_enable_func succeeded and return an error code if failed, > beacuse 0 is success code at caller (ftrace_regex_write). > > Without this fix, writing enable_event trigger on set_ftrace_filter > always doesn't work, since event_enable_func returns 1 to > ftrace_regex_write, it consumes 1 byte and pass input string > without the first character again. This makes event_enable_func > fail and disables event entry. > Ah, this actually fixes two bugs! :-) A typo will be considered success, but it also sends back to the user that it only wrote one byte! -- Steve -- 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/
Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_func succeeded
On Thu, 2013-05-09 at 10:31 -0400, Steven Rostedt wrote:
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 53582e9..44ac836 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -2061,8 +2061,11 @@ event_enable_func(struct ftrace_hash *hash,
> > if (ret < 0)
> > goto out_put;
> > ret = register_ftrace_function_probe(glob, ops, data);
> > - if (!ret)
> > + if (!ret) {
> > + ret = -ENOENT;
> > goto out_disable;
> > + } else
> > + ret = 0;
>
> I think you meant:
>
> if (ret < 0)
> goto out_disable;
> ret = 0;
>
> Otherwise, I don't see how you fixed anything, as you still return error
> if ret is something other than zero.
>
> Or am I missing something?
Yeah, this needs a comment. register_ftrace_function_probe() returns
the number of functions enabled, but if that is zero (or less), then it
should fail. We still need to check for less than ret.
I'll fix this one up.
Thanks!
-- Steve
--
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/
Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_func succeeded
On Thu, 2013-05-09 at 14:44 +0900, Masami Hiramatsu wrote:
> Return 0 instead of the number of activated ftrace if
> event_enable_func succeeded and return an error code if failed,
> beacuse 0 is success code at caller (ftrace_regex_write).
>
> Without this fix, writing enable_event trigger on set_ftrace_filter
> always doesn't work, since event_enable_func returns 1 to
> ftrace_regex_write, it consumes 1 byte and pass input string
> without the first character again. This makes event_enable_func
> fail and disables event entry.
>
> Signed-off-by: Masami Hiramatsu
> Cc: Steven Rostedt
> Cc: Frederic Weisbecker
> Cc: Ingo Molnar
> Cc: Tom Zanussi
> ---
> kernel/trace/trace_events.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 53582e9..44ac836 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2061,8 +2061,11 @@ event_enable_func(struct ftrace_hash *hash,
> if (ret < 0)
> goto out_put;
> ret = register_ftrace_function_probe(glob, ops, data);
> - if (!ret)
> + if (!ret) {
> + ret = -ENOENT;
> goto out_disable;
> + } else
> + ret = 0;
I think you meant:
if (ret < 0)
goto out_disable;
ret = 0;
Otherwise, I don't see how you fixed anything, as you still return error
if ret is something other than zero.
Or am I missing something?
-- Steve
> out:
> mutex_unlock(_mutex);
> return ret;
--
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/
Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_func succeeded
On Thu, 2013-05-09 at 14:44 +0900, Masami Hiramatsu wrote:
Return 0 instead of the number of activated ftrace if
event_enable_func succeeded and return an error code if failed,
beacuse 0 is success code at caller (ftrace_regex_write).
Without this fix, writing enable_event trigger on set_ftrace_filter
always doesn't work, since event_enable_func returns 1 to
ftrace_regex_write, it consumes 1 byte and pass input string
without the first character again. This makes event_enable_func
fail and disables event entry.
Signed-off-by: Masami Hiramatsu masami.hiramatsu...@hitachi.com
Cc: Steven Rostedt rost...@goodmis.org
Cc: Frederic Weisbecker fweis...@gmail.com
Cc: Ingo Molnar mi...@redhat.com
Cc: Tom Zanussi tom.zanu...@intel.com
---
kernel/trace/trace_events.c |5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 53582e9..44ac836 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2061,8 +2061,11 @@ event_enable_func(struct ftrace_hash *hash,
if (ret 0)
goto out_put;
ret = register_ftrace_function_probe(glob, ops, data);
- if (!ret)
+ if (!ret) {
+ ret = -ENOENT;
goto out_disable;
+ } else
+ ret = 0;
I think you meant:
if (ret 0)
goto out_disable;
ret = 0;
Otherwise, I don't see how you fixed anything, as you still return error
if ret is something other than zero.
Or am I missing something?
-- Steve
out:
mutex_unlock(event_mutex);
return ret;
--
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/
Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_func succeeded
On Thu, 2013-05-09 at 10:31 -0400, Steven Rostedt wrote:
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 53582e9..44ac836 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2061,8 +2061,11 @@ event_enable_func(struct ftrace_hash *hash,
if (ret 0)
goto out_put;
ret = register_ftrace_function_probe(glob, ops, data);
- if (!ret)
+ if (!ret) {
+ ret = -ENOENT;
goto out_disable;
+ } else
+ ret = 0;
I think you meant:
if (ret 0)
goto out_disable;
ret = 0;
Otherwise, I don't see how you fixed anything, as you still return error
if ret is something other than zero.
Or am I missing something?
Yeah, this needs a comment. register_ftrace_function_probe() returns
the number of functions enabled, but if that is zero (or less), then it
should fail. We still need to check for less than ret.
I'll fix this one up.
Thanks!
-- Steve
--
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/
Re: [PATCH 01/11] [BUGFIX] tracing: Return 0 if event_enable_func succeeded
On Thu, 2013-05-09 at 14:44 +0900, Masami Hiramatsu wrote: Return 0 instead of the number of activated ftrace if event_enable_func succeeded and return an error code if failed, beacuse 0 is success code at caller (ftrace_regex_write). Without this fix, writing enable_event trigger on set_ftrace_filter always doesn't work, since event_enable_func returns 1 to ftrace_regex_write, it consumes 1 byte and pass input string without the first character again. This makes event_enable_func fail and disables event entry. Ah, this actually fixes two bugs! :-) A typo will be considered success, but it also sends back to the user that it only wrote one byte! -- Steve -- 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/
