[PATCH] [PERF] do not manually count string lengths

[PATCH] [PERF] do not manually count string lengths

[Vincent Legoll]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

Hello,

here is a small patch to the perf tool to use strlen & macros
instead of manually counting string lengths.

I didn't find mention of any maintainer for the tool...

Please advise if I should submit this in another way

-- 
Vincent Legoll

[-- Attachment #2: 0001-Do-not-manually-count-string-lengths.patch --]
[-- Type: text/x-diff, Size: 2852 bytes --]

From 1c8755d5753f913fe4c02a9d0ee7fa51fda0c002 Mon Sep 17 00:00:00 2001
From: Vincent Legoll <vincent.legoll@gmail.com>
Date: Tue, 13 Oct 2009 10:04:54 +0200
Subject: [PATCH] Do not manually count string lengths

Use strlen & macros instead of manually counting string lengths
as this is error prone and may lend to bugs.

Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
---
 tools/perf/perf.c       |   16 ++++++++--------
 tools/perf/util/cache.h |    5 +++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 19fc7fe..624e62d 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -89,8 +89,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 		/*
 		 * Check remaining flags.
 		 */
-		if (!prefixcmp(cmd, "--exec-path")) {
-			cmd += 11;
+		if (!prefixcmp(cmd, CMD_EXEC_PATH)) {
+			cmd += strlen(CMD_EXEC_PATH);
 			if (*cmd == '=')
 				perf_set_argv_exec_path(cmd + 1);
 			else {
@@ -117,8 +117,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 			(*argv)++;
 			(*argc)--;
 			handled++;
-		} else if (!prefixcmp(cmd, "--perf-dir=")) {
-			setenv(PERF_DIR_ENVIRONMENT, cmd + 10, 1);
+		} else if (!prefixcmp(cmd, CMD_PERF_DIR)) {
+			setenv(PERF_DIR_ENVIRONMENT, cmd + strlen(CMD_PERF_DIR), 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--work-tree")) {
@@ -131,8 +131,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-		} else if (!prefixcmp(cmd, "--work-tree=")) {
-			setenv(PERF_WORK_TREE_ENVIRONMENT, cmd + 12, 1);
+		} else if (!prefixcmp(cmd, CMD_WORK_TREE)) {
+			setenv(PERF_WORK_TREE_ENVIRONMENT, cmd + strlen(CMD_WORK_TREE), 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--debugfs-dir")) {
@@ -146,8 +146,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-		} else if (!prefixcmp(cmd, "--debugfs-dir=")) {
-			strncpy(debugfs_mntpt, cmd + 14, MAXPATHLEN);
+		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
+			strncpy(debugfs_mntpt, cmd + strlen(CMD_DEBUGFS_DIR), MAXPATHLEN);
 			debugfs_mntpt[MAXPATHLEN - 1] = '\0';
 			if (envchanged)
 				*envchanged = 1;
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index 6f8ea9d..c6deebc 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -5,6 +5,11 @@
 #include "strbuf.h"
 #include "../perf.h"
 
+#define CMD_EXEC_PATH "--exec-path"
+#define CMD_PERF_DIR "--perf-dir="
+#define CMD_WORK_TREE "--work-tree="
+#define CMD_DEBUGFS_DIR "--debugfs-dir="
+
 #define PERF_DIR_ENVIRONMENT "PERF_DIR"
 #define PERF_WORK_TREE_ENVIRONMENT "PERF_WORK_TREE"
 #define DEFAULT_PERF_DIR_ENVIRONMENT ".perf"
-- 
1.6.0.4

Re: [PATCH] [PERF] do not manually count string lengths

[Ingo Molnar]

* Vincent Legoll <vincent.legoll@gmail.com> wrote:

> Hello,
> 
> here is a small patch to the perf tool to use strlen & macros
> instead of manually counting string lengths.
> 
> I didn't find mention of any maintainer for the tool...

It's maintained by:

 PERFORMANCE EVENTS SUBSYSTEM
 M:      Peter Zijlstra <a.p.zijlstra@chello.nl>
 M:      Paul Mackerras <paulus@samba.org>
 M:      Ingo Molnar <mingo@elte.hu>
 S:      Supported

We should add 'F:' file patterns to that entry i guess ...

Something like:

F:	kernel/perf_event.c
F:	include/linux/perf_event.h
F:	arch/*/*/kernel/perf_event.c
F:	arch/*/include/asm/perf_event.h
F:	tools/perf/

Would do the trick. Mind sending a patch for that too?

> Please advise if I should submit this in another way

Submitting it to lkml was fine enough - please Cc: it to us in the 
future to make sure we dont miss it.

Applied, thanks for the patch!

	Ingo

[tip:perf/core] perf tools: Do not manually count string lengths

[tip-bot for Vincent Legoll]

Commit-ID:  cfed95a693e1ea5d08b9c9019bc30e448437ee2f
Gitweb:     http://git.kernel.org/tip/cfed95a693e1ea5d08b9c9019bc30e448437ee2f
Author:     Vincent Legoll <vincent.legoll@gmail.com>
AuthorDate: Tue, 13 Oct 2009 10:18:16 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 13 Oct 2009 11:55:31 +0200

perf tools: Do not manually count string lengths

Use strlen & macros instead of manually counting string lengths as
this is error prone and may lend to bugs.

Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
Cc: Linus Torvalds <torvalds@osdl.org>
LKML-Reference: <4727185d0910130118m5387058dndb02ac9b384af9f0@mail.gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 tools/perf/perf.c       |   16 ++++++++--------
 tools/perf/util/cache.h |    5 +++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 19fc7fe..624e62d 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -89,8 +89,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 		/*
 		 * Check remaining flags.
 		 */
-		if (!prefixcmp(cmd, "--exec-path")) {
-			cmd += 11;
+		if (!prefixcmp(cmd, CMD_EXEC_PATH)) {
+			cmd += strlen(CMD_EXEC_PATH);
 			if (*cmd == '=')
 				perf_set_argv_exec_path(cmd + 1);
 			else {
@@ -117,8 +117,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 			(*argv)++;
 			(*argc)--;
 			handled++;
-		} else if (!prefixcmp(cmd, "--perf-dir=")) {
-			setenv(PERF_DIR_ENVIRONMENT, cmd + 10, 1);
+		} else if (!prefixcmp(cmd, CMD_PERF_DIR)) {
+			setenv(PERF_DIR_ENVIRONMENT, cmd + strlen(CMD_PERF_DIR), 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--work-tree")) {
@@ -131,8 +131,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-		} else if (!prefixcmp(cmd, "--work-tree=")) {
-			setenv(PERF_WORK_TREE_ENVIRONMENT, cmd + 12, 1);
+		} else if (!prefixcmp(cmd, CMD_WORK_TREE)) {
+			setenv(PERF_WORK_TREE_ENVIRONMENT, cmd + strlen(CMD_WORK_TREE), 1);
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--debugfs-dir")) {
@@ -146,8 +146,8 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				*envchanged = 1;
 			(*argv)++;
 			(*argc)--;
-		} else if (!prefixcmp(cmd, "--debugfs-dir=")) {
-			strncpy(debugfs_mntpt, cmd + 14, MAXPATHLEN);
+		} else if (!prefixcmp(cmd, CMD_DEBUGFS_DIR)) {
+			strncpy(debugfs_mntpt, cmd + strlen(CMD_DEBUGFS_DIR), MAXPATHLEN);
 			debugfs_mntpt[MAXPATHLEN - 1] = '\0';
 			if (envchanged)
 				*envchanged = 1;
diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
index f26172c..918eb37 100644
--- a/tools/perf/util/cache.h
+++ b/tools/perf/util/cache.h
@@ -5,6 +5,11 @@
 #include "strbuf.h"
 #include "../perf.h"
 
+#define CMD_EXEC_PATH "--exec-path"
+#define CMD_PERF_DIR "--perf-dir="
+#define CMD_WORK_TREE "--work-tree="
+#define CMD_DEBUGFS_DIR "--debugfs-dir="
+
 #define PERF_DIR_ENVIRONMENT "PERF_DIR"
 #define PERF_WORK_TREE_ENVIRONMENT "PERF_WORK_TREE"
 #define DEFAULT_PERF_DIR_ENVIRONMENT ".perf"

Re: [PATCH] [PERF] do not manually count string lengths

[Vincent Legoll]

On Tue, Oct 13, 2009 at 12:01 PM, Ingo Molnar <mingo@elte.hu> wrote:
> We should add 'F:' file patterns to that entry i guess ...
>
> Something like:
>
> F:      kernel/perf_event.c
> F:      include/linux/perf_event.h
> F:      arch/*/*/kernel/perf_event.c
> F:      arch/*/include/asm/perf_event.h
> F:      tools/perf/
>
> Would do the trick. Mind sending a patch for that too?

Yep, I'll do, but a quick find in the tree may reveal other
suspects that could be in the same league:

$ find . -name .git -prune -false -o -name drivers -prune \
-false -o -name '*perf*' | grep -v perfmon | grep -v gperf

arch/*/kernel/perf_event.c

arch/frv/lib/perf_event.c
arch/x86/kernel/cpu/perf_event.c

or maybe not:

arch/powerpc/kernel/perf_callchain.c
arch/parisc/kernel/perf_asm.S
arch/parisc/kernel/perf_images.h
arch/parisc/kernel/perf.c
arch/parisc/include/asm/perf.h

Was your list really comprehensive ?

-- 
Vincent Legoll

Re: [PATCH] [PERF] do not manually count string lengths

[Ingo Molnar]

* Vincent Legoll <vincent.legoll@gmail.com> wrote:

> On Tue, Oct 13, 2009 at 12:01 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > We should add 'F:' file patterns to that entry i guess ...
> >
> > Something like:
> >
> > F:      kernel/perf_event.c
> > F:      include/linux/perf_event.h
> > F:      arch/*/*/kernel/perf_event.c
> > F:      arch/*/include/asm/perf_event.h
> > F:      tools/perf/
> >
> > Would do the trick. Mind sending a patch for that too?
> 
> Yep, I'll do, but a quick find in the tree may reveal other
> suspects that could be in the same league:
> 
> $ find . -name .git -prune -false -o -name drivers -prune \
> -false -o -name '*perf*' | grep -v perfmon | grep -v gperf
> 
> arch/*/kernel/perf_event.c
> 
> arch/frv/lib/perf_event.c
> arch/x86/kernel/cpu/perf_event.c
> 
> or maybe not:
> 
> arch/powerpc/kernel/perf_callchain.c
> arch/parisc/kernel/perf_asm.S
> arch/parisc/kernel/perf_images.h
> arch/parisc/kernel/perf.c
> arch/parisc/include/asm/perf.h
> 
> Was your list really comprehensive ?

ah, yes - perf_callchain.c too indeed. The pattern does not have to be 
100% full coverage.

	Ingo

Re: [PATCH] [PERF] do not manually count string lengths

[Vincent Legoll]

[-- Attachment #1: Type: text/plain, Size: 251 bytes --]

On Tue, Oct 13, 2009 at 2:27 PM, Ingo Molnar <mingo@elte.hu> wrote:
>> Was your list really comprehensive ?
>
> ah, yes - perf_callchain.c too indeed. The pattern does not have to be
> 100% full coverage.

How looks the attached ?

-- 
Vincent Legoll

[-- Attachment #2: 0001-Update-performance-events-subsystem-maintainers-entr.patch --]
[-- Type: text/x-diff, Size: 971 bytes --]

From 74ef7cf99d1c5415771a4c7c98e72e247339ebe6 Mon Sep 17 00:00:00 2001
From: Vincent Legoll <vincent.legoll@gmail.com>
Date: Tue, 13 Oct 2009 14:41:46 +0200
Subject: [PATCH] Update performance events subsystem maintainers entry

Add file patterns that match relevant files for this
subsystem.

Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
---
 MAINTAINERS |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 69e31aa..cf69091 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4071,6 +4071,13 @@ M:	Peter Zijlstra <a.p.zijlstra@chello.nl>
 M:	Paul Mackerras <paulus@samba.org>
 M:	Ingo Molnar <mingo@elte.hu>
 S:	Supported
+F:	kernel/perf_event.c
+F:	include/linux/perf_event.h
+F:	arch/*/*/kernel/perf_event.c
+F:	arch/*/include/asm/perf_event.h
+F:	arch/*/lib/perf_event.c
+F:	arch/*/kernel/perf_callchain.c
+F:	tools/perf/
 
 PERSONALITY HANDLING
 M:	Christoph Hellwig <hch@infradead.org>
-- 
1.6.0.4

Re: [PATCH] [PERF] do not manually count string lengths

[Pekka Enberg]

Hi Vincent,

On Tue, Oct 13, 2009 at 2:27 PM, Ingo Molnar <mingo@elte.hu> wrote:
>>> Was your list really comprehensive ?
>>
>> ah, yes - perf_callchain.c too indeed. The pattern does not have to be
>> 100% full coverage.

On Tue, Oct 13, 2009 at 3:48 PM, Vincent Legoll
<vincent.legoll@gmail.com> wrote:
> How looks the attached ?

Can you please send patches as plain-text in the future? Attachments
are pain to review on some email clients (such as mine).

                        Pekka

[tip:perf/urgent] perf events: Update MAINTAINERS entry file patterns

[tip-bot for Vincent Legoll]

Commit-ID:  a003236c32706f3c1f74d4e3b98c58cf0d9a9d8f
Gitweb:     http://git.kernel.org/tip/a003236c32706f3c1f74d4e3b98c58cf0d9a9d8f
Author:     Vincent Legoll <vincent.legoll@gmail.com>
AuthorDate: Tue, 13 Oct 2009 14:48:14 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 13 Oct 2009 15:28:52 +0200

perf events: Update MAINTAINERS entry file patterns

Add file patterns that match relevant files for this subsystem.

Signed-off-by: Vincent Legoll <vincent.legoll@gmail.com>
Cc: Linus Torvalds <torvalds@osdl.org>
Cc: paulus@samba.org
Cc: a.p.zijlstra@chello.nl
LKML-Reference: <4727185d0910130548p325f0185vf4e23b5491c730a0@mail.gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 MAINTAINERS |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 69e31aa..cf69091 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4071,6 +4071,13 @@ M:	Peter Zijlstra <a.p.zijlstra@chello.nl>
 M:	Paul Mackerras <paulus@samba.org>
 M:	Ingo Molnar <mingo@elte.hu>
 S:	Supported
+F:	kernel/perf_event.c
+F:	include/linux/perf_event.h
+F:	arch/*/*/kernel/perf_event.c
+F:	arch/*/include/asm/perf_event.h
+F:	arch/*/lib/perf_event.c
+F:	arch/*/kernel/perf_callchain.c
+F:	tools/perf/
 
 PERSONALITY HANDLING
 M:	Christoph Hellwig <hch@infradead.org>