perf tools: Fix the code to strip command name

PATCH In review Jiri Olsa 5 participants 10 messages Apr 20, 2017 – Apr 24, 2017 perf events

Share & tools

Jiri Olsa

author reviewer

2017-04-20 09:24 UTC #0 lore raw

PATCH 1/1 perf tools: Fix the code to strip command name

Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.

Keeping the ltrim, but moving back the left to right '\n' search instead of the rtrim.

Fixes bdd97ca63faa ("perf tools: Refactor the code to strip command name with {l,r}trim()")

Signed-off-by Jiri Olsa <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Taeung Song <[email protected]>

Cc Jin Yao <[email protected]>

Link http://lkml.kernel.org/n/[email protected] open here

---
 tools/perf/util/event.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

tools/perf/util/event.c +8 -1

index cf457ef534da..1a9164a816d9 100644
@@ -138,8 +138,15 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
 	ppids = strstr(bf, "PPid:");
 
 	if (name) {
+		char *nl;
+
 		name += 5;  /* strlen("Name:") */
-		name = rtrim(ltrim(name));
+		name = ltrim(name);
+
+		nl = strchr(name, '\n');
+		if (nl)
+			*nl = '\0';
+
 		size = strlen(name);
 		if (size >= len)
 			size = len - 1;

Signature · 1 line

2.9.3

Taeung Song

2017-04-20 10:17 UTC #1 lore raw

Hi Jiri,

Quoted 4 lines · Jiri Olsa

Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.

Sorry for my commit that have failings.

Could I know the command name in the above case ?
The command name can have two '\n' ?

Thanks,
Taeung

Quoted 38 lines

Keeping the ltrim, but moving back the left to right '\n' search instead of the rtrim.
Fixes bdd97ca63faa ("perf tools: Refactor the code to strip command name with {l,r}trim()")
Signed-off-by Jiri Olsa <[email protected]>
Cc David Ahern <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Taeung Song <[email protected]>
Cc Jin Yao <[email protected]>
Link http://lkml.kernel.org/n/[email protected] open here
---
 tools/perf/util/event.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
tools/perf/util/event.c +8 -1
index cf457ef534da..1a9164a816d9 100644
@@ -138,8 +138,15 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
 	ppids = strstr(bf, "PPid:");
 
 	if (name) {
+		char *nl;
+
 		name += 5;  /* strlen("Name:") */
-		name = rtrim(ltrim(name));
+		name = ltrim(name);
+
+		nl = strchr(name, '\n');
+		if (nl)
+			*nl = '\0';
+
 		size = strlen(name);
 		if (size >= len)
 			size = len - 1;

Jiri Olsa

2017-04-20 10:35 UTC #2 lore raw

Quoted 8 lines · Taeung Song

Hi Jiri,
Quoted 4 lines · Jiri Olsa
Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.

Quoted 4 lines

Sorry for my commit that have failings.
Could I know the command name in the above case ?
The command name can have two '\n' ?

it's the next line in the status file.. parts of the Umask string and 1 newline

Name:   systemd
Umask:  0000
State:  S (sleeping)

...

I've already posted it in here:
http://marc.info/?l=linuxppc-embedded&m=149200723316270&w=2

jirka

Taeung Song

2017-04-20 10:42 UTC #3 lore raw

Quoted 20 lines · Jiri Olsa

Quoted 8 lines · Taeung Song
Hi Jiri,
Quoted 4 lines · Jiri Olsa
Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.
Quoted 4 lines
Sorry for my commit that have failings.
Could I know the command name in the above case ?
The command name can have two '\n' ?
it's the next line in the status file.. parts of the Umask string and 1 newline
Name:   systemd
Umask:  0000
State:  S (sleeping)

Quoted 7 lines

...
I've already posted it in here:
http://marc.info/?l=linuxppc-embedded&m=149200723316270&w=2
jirka

I understood it.
Sorry for my mistake..

Thanks,
Taeung

Jiri Olsa

2017-04-20 10:49 UTC #4 lore raw

Quoted 25 lines · Taeung Song

Quoted 22 lines · Jiri Olsa
Quoted 8 lines · Taeung Song
Hi Jiri,
Quoted 4 lines · Jiri Olsa
Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.
Quoted 4 lines
Sorry for my commit that have failings.
Could I know the command name in the above case ?
The command name can have two '\n' ?
it's the next line in the status file.. parts of the Umask string and 1 newline
Name:   systemd
Umask:  0000
State:  S (sleeping)
...

Quoted 8 lines

Quoted 5 lines
I've already posted it in here:
http://marc.info/?l=linuxppc-embedded&m=149200723316270&w=2
jirka
I understood it.
Sorry for my mistake..

no worries.. we do plenty of those ;-)

jirka

Jiri Olsa

2017-04-24 11:37 UTC #5 lore raw

In reply to Jiri Olsa

Arnaldo,
could you please take this one?

thanks,
jirka

Quoted 44 lines · Jiri Olsa

Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.
Keeping the ltrim, but moving back the left to right '\n' search instead of the rtrim.
Fixes bdd97ca63faa ("perf tools: Refactor the code to strip command name with {l,r}trim()")
Signed-off-by Jiri Olsa <[email protected]>
Cc David Ahern <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Taeung Song <[email protected]>
Cc Jin Yao <[email protected]>
Link http://lkml.kernel.org/n/[email protected] open here
---
 tools/perf/util/event.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
tools/perf/util/event.c +8 -1
index cf457ef534da..1a9164a816d9 100644
@@ -138,8 +138,15 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
 	ppids = strstr(bf, "PPid:");
 
 	if (name) {
+		char *nl;
+
 		name += 5;  /* strlen("Name:") */
-		name = rtrim(ltrim(name));
+		name = ltrim(name);
+
+		nl = strchr(name, '\n');
+		if (nl)
+			*nl = '\0';
+
 		size = strlen(name);
 		if (size >= len)
 			size = len - 1;
Signature · 1 line
2.9.3

Arnaldo Carvalho de Melo

maintainer

2017-04-24 15:45 UTC #6 lore raw

In reply to Jiri Olsa

Em Thu, Apr 20, 2017 at 11:24:30AM +0200, Jiri Olsa escreveu:

Quoted 7 lines

Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.
Keeping the ltrim, but moving back the left to right '\n' search instead of the rtrim.

perf/urgent?

Quoted 1 line

Fixes bdd97ca63faa ("perf tools: Refactor the code to strip command name with {l,r}trim()")

[acme@jouet linux]$ git tag --contains bdd97ca63faa perf-core-for-mingo-4.12-20170411 perf-core-for-mingo-4.12-20170413 perf-core-for-mingo-4.12-20170419 [acme@jouet linux]$

It is just in tip/perf/core, will put in acme/perf/core and push to Ingo in my next pull req.

Thanks,

- Arnaldo

Quoted 34 lines

Signed-off-by Jiri Olsa <[email protected]>
Cc David Ahern <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Taeung Song <[email protected]>
Cc Jin Yao <[email protected]>
Link http://lkml.kernel.org/n/[email protected] open here
---
 tools/perf/util/event.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
tools/perf/util/event.c +8 -1
index cf457ef534da..1a9164a816d9 100644
@@ -138,8 +138,15 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
 	ppids = strstr(bf, "PPid:");
 
 	if (name) {
+		char *nl;
+
 		name += 5;  /* strlen("Name:") */
-		name = rtrim(ltrim(name));
+		name = ltrim(name);
+
+		nl = strchr(name, '\n');
+		if (nl)
+			*nl = '\0';
+
 		size = strlen(name);
 		if (size >= len)
 			size = len - 1;
Signature · 1 line
2.9.3

Jiri Olsa

2017-04-24 16:07 UTC #7 lore raw

Quoted 13 lines · Arnaldo Carvalho de Melo

Em Thu, Apr 20, 2017 at 11:24:30AM +0200, Jiri Olsa escreveu:
Quoted 7 lines
Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.
Keeping the ltrim, but moving back the left to right '\n' search instead of the rtrim.
perf/urgent?
Quoted 1 line
Fixes bdd97ca63faa ("perf tools: Refactor the code to strip command name with {l,r}trim()")

Quoted 8 lines

[acme@jouet linux]$ git tag --contains bdd97ca63faa perf-core-for-mingo-4.12-20170411 perf-core-for-mingo-4.12-20170413 perf-core-for-mingo-4.12-20170419 [acme@jouet linux]$
It is just in tip/perf/core, will put in acme/perf/core and push to Ingo in my next pull req.

sure, I did not check.. just thought it's urgent from time POV ;-)

thanks,
jirka

Arnaldo Carvalho de Melo

maintainer

2017-04-24 16:28 UTC #8 lore raw

Em Mon, Apr 24, 2017 at 06:06:45PM +0200, Jiri Olsa escreveu:

Quoted 20 lines · Arnaldo Carvalho de Melo

Quoted 19 lines
Em Thu, Apr 20, 2017 at 11:24:30AM +0200, Jiri Olsa escreveu:
Quoted 7 lines
Recent commit broke command name strip in perf_event__get_comm_ids function. It replaced left to right search for '\n' with rtrim, which actually does right to left search. It occasionally caught earlier '\n' and kept trash in the command name.
Keeping the ltrim, but moving back the left to right '\n' search instead of the rtrim.
perf/urgent?
Quoted 1 line
Fixes bdd97ca63faa ("perf tools: Refactor the code to strip command name with {l,r}trim()")
[acme@jouet linux]$ git tag --contains bdd97ca63faa perf-core-for-mingo-4.12-20170411 perf-core-for-mingo-4.12-20170413 perf-core-for-mingo-4.12-20170419 [acme@jouet linux]$

Quoted 4 lines

Quoted 2 lines
It is just in tip/perf/core, will put in acme/perf/core and push to Ingo in my next pull req.
sure, I did not check.. just thought it's urgent from time POV ;-)

:-)

I took it too literally then, tried to apply it to perf/urgent, it failed, scratched my head...

Anyway, applied.

- Arnaldo

tip-bot for Jiri Olsa

merge bot

2017-04-24 21:20 UTC #9 lore raw

In reply to Jiri Olsa

perf tools: Fix the code to strip command name

Commit-ID:  9d43f5e8df6804ae271407500af9062e9278167a
Gitweb:     http://git.kernel.org/tip/9d43f5e8df6804ae271407500af9062e9278167a
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 20 Apr 2017 11:24:30 +0200
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 24 Apr 2017 13:43:37 -0300

perf tools: Fix the code to strip command name

Keeping the ltrim, but moving back the left to right '\n' search instead of the rtrim.

Signed-off-by Jiri Olsa <[email protected]>

Acked-by Taeung Song <[email protected]>

Cc David Ahern <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Yao Jin <[email protected]>

Fixes bdd97ca63faa ("perf tools: Refactor the code to strip command name with {l,r}trim()")

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/event.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

tools/perf/util/event.c +8 -1

index 2e829ac..142835c 100644
@@ -141,8 +141,15 @@ static int perf_event__get_comm_ids(pid_t pid, char *comm, size_t len,
 	ppids = strstr(bf, "PPid:");
 
 	if (name) {
+		char *nl;
+
 		name += 5;  /* strlen("Name:") */
-		name = rtrim(ltrim(name));
+		name = ltrim(name);
+
+		nl = strchr(name, '\n');
+		if (nl)
+			*nl = '\0';
+
 		size = strlen(name);
 		if (size >= len)
 			size = len - 1;