ftrace: Rename set_tracer_flags()'s local variable trace_flags

PATCH Superseded Zhaolei 3 participants 10 messages Aug 6, 2009 – Aug 8, 2009 Tracing

Share & tools

Zhaolei

author

2009-08-06 03:23 UTC #0 lore raw

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

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 <[email protected]>

---
 kernel/trace/trace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

kernel/trace/trace.c +7 -7

index e30e6b1..464b7bb 100644
@@ -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;
 }

Signature · 1 line

1.5.5.3

Zhaolei

author

2009-08-06 03:24 UTC #1 lore raw

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

"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 <[email protected]>

---
 kernel/trace/trace.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

kernel/trace/trace.c +2 -3

index 464b7bb..2fa2bac 100644
@@ -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;
 	}

Signature · 1 line

1.5.5.3

Frederic Weisbecker

Acked-by on v1 2/2

2009-08-06 04:53 UTC #2 lore raw

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

Quoted 19 lines · 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 <[email protected]>
---
 kernel/trace/trace.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
kernel/trace/trace.c +0 -1
index 464b7bb..2fa2bac 100644
@@ -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);

kernel/trace/trace.c trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt, · line 3903

Quoted 10 lines

kernel/trace/trace.c +2 -2

+		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 <[email protected]>

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...

Li Zefan

2009-08-06 05:06 UTC #3 lore raw

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

In reply to Zhaolei

Quoted 5 lines · 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().

Nice catch.

Quoted 14 lines

Signed-off-by Zhao Lei <[email protected]>

---
 kernel/trace/trace.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

kernel/trace/trace.c +0 -1

index 464b7bb..2fa2bac 100644
@@ -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);

kernel/trace/trace.c trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt, · line 3903

Quoted 10 lines

kernel/trace/trace.c +2 -2

+		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);

Zhaolei

author

2009-08-07 09:33 UTC #4 lore raw

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

Quoted 36 lines · Li Zefan

Quoted 5 lines · 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().
Nice catch.
Quoted 14 lines
Signed-off-by Zhao Lei <[email protected]>
---
 kernel/trace/trace.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
kernel/trace/trace.c +0 -1
index 464b7bb..2fa2bac 100644
@@ -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);
kernel/trace/trace.c trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt, · line 3903
Quoted 10 lines
kernel/trace/trace.c +2 -2
+		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:

Quoted 3 lines

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

Hello, Li

Thanks.
V2 will send.

Thanks
Zhaolei

Zhaolei

author

2009-08-07 10:50 UTC #5 lore raw

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

In reply to Li Zefan

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 <[email protected]>

---
 kernel/trace/trace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

kernel/trace/trace.c +7 -7

index 6344573..6c5f67f 100644
@@ -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;
 }

Signature · 1 line

1.5.5.3

Frederic Weisbecker

Applied

2009-08-08 00:38 UTC #6 lore raw

Quoted 10 lines · 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 <[email protected]>

Queued for .32, thanks!

Quoted 53 lines

---
 kernel/trace/trace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

kernel/trace/trace.c +7 -7

index 6344573..6c5f67f 100644
@@ -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;
 }

Signature · 1 line

1.5.5.3

Zhaolei

author

2009-08-07 10:52 UTC #7 lore raw

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

In reply to Li Zefan

"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 <[email protected]>'s suggest.

Signed-off-by Zhao Lei <[email protected]>

---
 kernel/trace/trace.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

kernel/trace/trace.c +2 ~~-10~~

index 6c5f67f..06a2dfa 100644
@@ -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;

Signature · 1 line

1.5.5.3

Frederic Weisbecker

Applied

2009-08-08 00:39 UTC #8 lore raw

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

Quoted 9 lines · 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 <[email protected]>'s suggest.
Signed-off-by Zhao Lei <[email protected]>

Queued for .31, thanks!

Quoted 32 lines

---
 kernel/trace/trace.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

kernel/trace/trace.c +2 ~~-10~~

index 6c5f67f..06a2dfa 100644
@@ -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;

Signature · 1 line

1.5.5.3

Frederic Weisbecker

Acked-by on v1 1/2

2009-08-06 03:50 UTC #9 lore raw

In reply to Zhaolei

Quoted 7 lines · 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 <[email protected]>

Acked-by Frederic Weisbecker <[email protected]>

Quoted 53 lines

---
 kernel/trace/trace.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

kernel/trace/trace.c +7 -7

index e30e6b1..464b7bb 100644
@@ -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;
 }

Signature · 1 line

1.5.5.3