[PATCH 1/2] ftrace: Rename set_tracer_flags()'s local variable trace_flags

[PATCH 1/2] ftrace: Rename set_tracer_flags()'s local variable trace_flags

[Zhaolei]

set_tracer_flags() have a local variable named trace_flags which have
same name with global one.
Actually, using tracer_flags should be better by its meaning.

[ Impact: cleanup ]

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 kernel/trace/trace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e30e6b1..464b7bb 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2291,23 +2291,23 @@ tracing_trace_options_read(struct file *filp, char __user *ubuf,
 /* Try to assign a tracer specific option */
 static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
 {
-	struct tracer_flags *trace_flags = trace->flags;
+	struct tracer_flags *tracer_flags = trace->flags;
 	struct tracer_opt *opts = NULL;
 	int ret = 0, i = 0;
 	int len;
 
-	for (i = 0; trace_flags->opts[i].name; i++) {
-		opts = &trace_flags->opts[i];
+	for (i = 0; tracer_flags->opts[i].name; i++) {
+		opts = &tracer_flags->opts[i];
 		len = strlen(opts->name);
 
 		if (strncmp(cmp, opts->name, len) == 0) {
-			ret = trace->set_flag(trace_flags->val,
+			ret = trace->set_flag(tracer_flags->val,
 				opts->bit, !neg);
 			break;
 		}
 	}
 	/* Not found */
-	if (!trace_flags->opts[i].name)
+	if (!tracer_flags->opts[i].name)
 		return -EINVAL;
 
 	/* Refused to handle */
@@ -2315,9 +2315,9 @@ static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
 		return ret;
 
 	if (neg)
-		trace_flags->val &= ~opts->bit;
+		tracer_flags->val &= ~opts->bit;
 	else
-		trace_flags->val |= opts->bit;
+		tracer_flags->val |= opts->bit;
 
 	return 0;
 }
-- 
1.5.5.3

[PATCH 2/2] ftrace: Unify effect of writing to trace_options and option/*

[Zhaolei]

"echo noglobal-clock > trace_options" can be used to change trace
clock but "echo 0 > options/global-clock" can't.

We can fix it by using set_tracer_flags() in trace_options_core_write().

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 kernel/trace/trace.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 464b7bb..2fa2bac 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3893,12 +3893,11 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt,
 
 	switch (val) {
 	case 0:
-		trace_flags &= ~(1 << index);
+		set_tracer_flags(1 << index, 0);
 		break;
 	case 1:
-		trace_flags |= 1 << index;
+		set_tracer_flags(1 << index, 1);
 		break;
-
 	default:
 		return -EINVAL;
 	}
-- 
1.5.5.3

Re: [PATCH 1/2] ftrace: Rename set_tracer_flags()'s local variable trace_flags

[Frederic Weisbecker]

On Thu, Aug 06, 2009 at 11:26:51AM +0800, Zhaolei wrote:
> set_tracer_flags() have a local variable named trace_flags which have
> same name with global one.
> Actually, using tracer_flags should be better by its meaning.
> 
> [ Impact: cleanup ]
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>


> ---
>  kernel/trace/trace.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index e30e6b1..464b7bb 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2291,23 +2291,23 @@ tracing_trace_options_read(struct file *filp, char __user *ubuf,
>  /* Try to assign a tracer specific option */
>  static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
>  {
> -	struct tracer_flags *trace_flags = trace->flags;
> +	struct tracer_flags *tracer_flags = trace->flags;
>  	struct tracer_opt *opts = NULL;
>  	int ret = 0, i = 0;
>  	int len;
>  
> -	for (i = 0; trace_flags->opts[i].name; i++) {
> -		opts = &trace_flags->opts[i];
> +	for (i = 0; tracer_flags->opts[i].name; i++) {
> +		opts = &tracer_flags->opts[i];
>  		len = strlen(opts->name);
>  
>  		if (strncmp(cmp, opts->name, len) == 0) {
> -			ret = trace->set_flag(trace_flags->val,
> +			ret = trace->set_flag(tracer_flags->val,
>  				opts->bit, !neg);
>  			break;
>  		}
>  	}
>  	/* Not found */
> -	if (!trace_flags->opts[i].name)
> +	if (!tracer_flags->opts[i].name)
>  		return -EINVAL;
>  
>  	/* Refused to handle */
> @@ -2315,9 +2315,9 @@ static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
>  		return ret;
>  
>  	if (neg)
> -		trace_flags->val &= ~opts->bit;
> +		tracer_flags->val &= ~opts->bit;
>  	else
> -		trace_flags->val |= opts->bit;
> +		tracer_flags->val |= opts->bit;
>  
>  	return 0;
>  }
> -- 
> 1.5.5.3
> 
> 

Re: [PATCH 2/2] ftrace: Unify effect of writing to trace_options and option/*

[Frederic Weisbecker]

On Thu, Aug 06, 2009 at 11:27:56AM +0800, Zhaolei wrote:
> "echo noglobal-clock > trace_options" can be used to change trace
> clock but "echo 0 > options/global-clock" can't.
> 
> We can fix it by using set_tracer_flags() in trace_options_core_write().
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  kernel/trace/trace.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 464b7bb..2fa2bac 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3893,12 +3893,11 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  
>  	switch (val) {
>  	case 0:
> -		trace_flags &= ~(1 << index);
> +		set_tracer_flags(1 << index, 0);
>  		break;
>  	case 1:
> -		trace_flags |= 1 << index;
> +		set_tracer_flags(1 << index, 1);
>  		break;
> -
>  	default:
>  		return -EINVAL;
>  	}


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

It applies on .31 BTW.

That makes me think: shouldn't it be better to keep
tracing/options for global options and options/* for
local tracer options? That breaks the ABI but...

Re: [PATCH 2/2] ftrace: Unify effect of writing to trace_options and option/*

[Li Zefan]

Zhaolei wrote:
> "echo noglobal-clock > trace_options" can be used to change trace
> clock but "echo 0 > options/global-clock" can't.
> 
> We can fix it by using set_tracer_flags() in trace_options_core_write().
> 

Nice catch.

> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
> ---
>  kernel/trace/trace.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 464b7bb..2fa2bac 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3893,12 +3893,11 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  
>  	switch (val) {
>  	case 0:
> -		trace_flags &= ~(1 << index);
> +		set_tracer_flags(1 << index, 0);
>  		break;
>  	case 1:
> -		trace_flags |= 1 << index;
> +		set_tracer_flags(1 << index, 1);
>  		break;
> -
>  	default:
>  		return -EINVAL;
>  	}

The whole switch statement can be simplied:

	if (val != 0 || val != 1)
		return -EINVAL;
	set_tracer_flags(1 << index, val);

Re: Re: [PATCH 2/2] ftrace: Unify effect of writing to trace_options and option/*

[Zhaolei]

Li Zefan wrote:
> Zhaolei wrote:
>> "echo noglobal-clock > trace_options" can be used to change trace
>> clock but "echo 0 > options/global-clock" can't.
>>
>> We can fix it by using set_tracer_flags() in trace_options_core_write().
>>
> 
> Nice catch.
> 
>> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
>> ---
>>  kernel/trace/trace.c |    5 ++---
>>  1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>> index 464b7bb..2fa2bac 100644
>> --- a/kernel/trace/trace.c
>> +++ b/kernel/trace/trace.c
>> @@ -3893,12 +3893,11 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt,
>>  
>>  	switch (val) {
>>  	case 0:
>> -		trace_flags &= ~(1 << index);
>> +		set_tracer_flags(1 << index, 0);
>>  		break;
>>  	case 1:
>> -		trace_flags |= 1 << index;
>> +		set_tracer_flags(1 << index, 1);
>>  		break;
>> -
>>  	default:
>>  		return -EINVAL;
>>  	}
> 
> The whole switch statement can be simplied:
> 
> 	if (val != 0 || val != 1)
> 		return -EINVAL;
> 	set_tracer_flags(1 << index, val);

Hello, Li

Thanks.
V2 will send.

Thanks
Zhaolei

[PATCH v2 1/2] ftrace: Rename set_tracer_flags()'s local variable trace_flags

[Zhaolei]

set_tracer_flags() have a local variable named trace_flags which have
same name with global one.
Actually, using tracer_flags should be better by its meaning.

Changelog:
v1->v2: Simplied another patch in this patchset, no change in this patch.

[ Impact: cleanup ]

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 kernel/trace/trace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6344573..6c5f67f 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2117,23 +2117,23 @@ tracing_trace_options_read(struct file *filp, char __user *ubuf,
 /* Try to assign a tracer specific option */
 static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
 {
-	struct tracer_flags *trace_flags = trace->flags;
+	struct tracer_flags *tracer_flags = trace->flags;
 	struct tracer_opt *opts = NULL;
 	int ret = 0, i = 0;
 	int len;
 
-	for (i = 0; trace_flags->opts[i].name; i++) {
-		opts = &trace_flags->opts[i];
+	for (i = 0; tracer_flags->opts[i].name; i++) {
+		opts = &tracer_flags->opts[i];
 		len = strlen(opts->name);
 
 		if (strncmp(cmp, opts->name, len) == 0) {
-			ret = trace->set_flag(trace_flags->val,
+			ret = trace->set_flag(tracer_flags->val,
 				opts->bit, !neg);
 			break;
 		}
 	}
 	/* Not found */
-	if (!trace_flags->opts[i].name)
+	if (!tracer_flags->opts[i].name)
 		return -EINVAL;
 
 	/* Refused to handle */
@@ -2141,9 +2141,9 @@ static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
 		return ret;
 
 	if (neg)
-		trace_flags->val &= ~opts->bit;
+		tracer_flags->val &= ~opts->bit;
 	else
-		trace_flags->val |= opts->bit;
+		tracer_flags->val |= opts->bit;
 
 	return 0;
 }
-- 
1.5.5.3

[PATCH v2 2/2] ftrace: Unify effect of writing to trace_options and option/*

[Zhaolei]

"echo noglobal-clock > trace_options" can be used to change trace
clock but "echo 0 > options/global-clock" can't.

We can fix it by using set_tracer_flags() in trace_options_core_write().

Changelog:
v1->v2: Simplied switch() by Li Zefan <lizf@cn.fujitsu.com>'s suggest.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 kernel/trace/trace.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6c5f67f..06a2dfa 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -3718,17 +3718,9 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt,
 	if (ret < 0)
 		return ret;
 
-	switch (val) {
-	case 0:
-		trace_flags &= ~(1 << index);
-		break;
-	case 1:
-		trace_flags |= 1 << index;
-		break;
-
-	default:
+	if (val != 0 && val != 1)
 		return -EINVAL;
-	}
+	set_tracer_flags(1 << index, val);
 
 	*ppos += cnt;
 
-- 
1.5.5.3

Re: [PATCH v2 1/2] ftrace: Rename set_tracer_flags()'s local variable trace_flags

[Frederic Weisbecker]

On Fri, Aug 07, 2009 at 06:53:21PM +0800, Zhaolei wrote:
> set_tracer_flags() have a local variable named trace_flags which have
> same name with global one.
> Actually, using tracer_flags should be better by its meaning.
> 
> Changelog:
> v1->v2: Simplied another patch in this patchset, no change in this patch.
> 
> [ Impact: cleanup ]
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>


Queued for .32, thanks!


> ---
>  kernel/trace/trace.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6344573..6c5f67f 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -2117,23 +2117,23 @@ tracing_trace_options_read(struct file *filp, char __user *ubuf,
>  /* Try to assign a tracer specific option */
>  static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
>  {
> -	struct tracer_flags *trace_flags = trace->flags;
> +	struct tracer_flags *tracer_flags = trace->flags;
>  	struct tracer_opt *opts = NULL;
>  	int ret = 0, i = 0;
>  	int len;
>  
> -	for (i = 0; trace_flags->opts[i].name; i++) {
> -		opts = &trace_flags->opts[i];
> +	for (i = 0; tracer_flags->opts[i].name; i++) {
> +		opts = &tracer_flags->opts[i];
>  		len = strlen(opts->name);
>  
>  		if (strncmp(cmp, opts->name, len) == 0) {
> -			ret = trace->set_flag(trace_flags->val,
> +			ret = trace->set_flag(tracer_flags->val,
>  				opts->bit, !neg);
>  			break;
>  		}
>  	}
>  	/* Not found */
> -	if (!trace_flags->opts[i].name)
> +	if (!tracer_flags->opts[i].name)
>  		return -EINVAL;
>  
>  	/* Refused to handle */
> @@ -2141,9 +2141,9 @@ static int set_tracer_option(struct tracer *trace, char *cmp, int neg)
>  		return ret;
>  
>  	if (neg)
> -		trace_flags->val &= ~opts->bit;
> +		tracer_flags->val &= ~opts->bit;
>  	else
> -		trace_flags->val |= opts->bit;
> +		tracer_flags->val |= opts->bit;
>  
>  	return 0;
>  }
> -- 
> 1.5.5.3
> 
> 

Re: [PATCH v2 2/2] ftrace: Unify effect of writing to trace_options and option/*

[Frederic Weisbecker]

On Fri, Aug 07, 2009 at 06:55:48PM +0800, Zhaolei wrote:
> "echo noglobal-clock > trace_options" can be used to change trace
> clock but "echo 0 > options/global-clock" can't.
> 
> We can fix it by using set_tracer_flags() in trace_options_core_write().
> 
> Changelog:
> v1->v2: Simplied switch() by Li Zefan <lizf@cn.fujitsu.com>'s suggest.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>


Queued for .31, thanks!

> ---
>  kernel/trace/trace.c |   12 ++----------
>  1 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6c5f67f..06a2dfa 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3718,17 +3718,9 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  	if (ret < 0)
>  		return ret;
>  
> -	switch (val) {
> -	case 0:
> -		trace_flags &= ~(1 << index);
> -		break;
> -	case 1:
> -		trace_flags |= 1 << index;
> -		break;
> -
> -	default:
> +	if (val != 0 && val != 1)
>  		return -EINVAL;
> -	}
> +	set_tracer_flags(1 << index, val);
>  
>  	*ppos += cnt;
>  
> -- 
> 1.5.5.3
> 
>