perf-record fix and UI improvement

perf-record fix and UI improvement

[Pierre Habouzit]

While toying with perf, I've noticed that perf record can easily enter
a busy loop when doing something as silly as:

    $ perf record -A ls

I've searched why and here are the patches:

 [PATCH 1/2] perf util: do_read should fail on EOF instead of busy-looping.

   Yeah, do_read here really wants to read a known size, not being able
   to should die(), not busy-lopp ;)
   That was the cause for the bug.


 [PATCH 2/2] perf-record: improve -A UI for empty or non-existent perf.data

   Though with 1/2 `git record -A ls` would then fail miserably with
   some kind of "cannot read" error, which sucks. So this patch
   understands -A as a "append or create if file is empty or inexistant"

   This fact may deserve to be documented properly, if so just tell me
   I'll send an updated patch for Documentation/


I'm kind of new to the kernel world, so I hope I sent the patches to the
proper persons.
-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

[PATCH 1/2] perf util: do_read should fail on EOF instead of busy-looping.

[Pierre Habouzit]

From: Pierre Habouzit <madcoder@debian.org>

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 tools/perf/util/header.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 450384b..531f2b9 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -185,6 +185,8 @@ static void do_read(int fd, void *buf, size_t size)
 
 		if (ret < 0)
 			die("failed to read");
+		if (ret == 0)
+			die("failed to read: missing data");
 
 		size -= ret;
 		buf += ret;
-- 
1.6.4.rc2.183.g9085a

[PATCH 2/2] perf-record: improve -A UI for empty or non-existent perf.data

[Pierre Habouzit]

From: Pierre Habouzit <madcoder@debian.org>

1. Ignore the -A argument if there is no perf.data file
2. Treat an empty file like a non existent file.

Else, perf will try to read the perf.data header, and fail with an error.

Treating an empty file like a non-existent file makes sense, since an
interupted (as in SIGKILLed) perf could leave such files around, and you
don't want to annoy the user with errors for files with no data in it.

Signed-off-by: Pierre Habouzit <pierre.habouzit@intersec.com>
---
 tools/perf/builtin-record.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 6da0992..afbfb9d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -524,10 +524,14 @@ static int __cmd_record(int argc, const char **argv)
 	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 
-	if (!stat(output_name, &st) && !force && !append_file) {
-		fprintf(stderr, "Error, output file %s exists, use -A to append or -f to overwrite.\n",
-				output_name);
-		exit(-1);
+	if (!stat(output_name, &st) && st.st_size) {
+		if (!force && !append_file) {
+			fprintf(stderr, "Error, output file %s exists, use -A to append or -f to overwrite.\n",
+					output_name);
+			exit(-1);
+		}
+	} else {
+		append_file = 0;
 	}
 
 	flags = O_CREAT|O_RDWR;
-- 
1.6.4.rc2.183.g9085a

[tip:perfcounters/urgent] perf record: Fix the -A UI for empty or non-existent perf.data

[tip-bot for Pierre Habouzit]

Commit-ID:  7e6649a1a2dd0774160dc4f8d5db762ac3678ba0
Gitweb:     http://git.kernel.org/tip/7e6649a1a2dd0774160dc4f8d5db762ac3678ba0
Author:     Pierre Habouzit <pierre.habouzit@intersec.com>
AuthorDate: Fri, 7 Aug 2009 14:16:01 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 7 Aug 2009 17:08:17 +0200

perf record: Fix the -A UI for empty or non-existent perf.data

1. Ignore the -A argument if there is no perf.data file
2. Treat an empty file like a non existent file.

Else, perf will try to read the perf.data header, and fail with
an error.

Treating an empty file like a non-existent file makes sense,
since an interupted (as in SIGKILLed) perf could leave such
files around, and you don't want to annoy the user with errors
for files with no data in it.

Signed-off-by: Pierre Habouzit <pierre.habouzit@intersec.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
LKML-Reference: <1249647361-11582-3-git-send-email-pierre.habouzit@intersec.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 tools/perf/builtin-record.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 6da0992..afbfb9d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -524,10 +524,14 @@ static int __cmd_record(int argc, const char **argv)
 	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 
-	if (!stat(output_name, &st) && !force && !append_file) {
-		fprintf(stderr, "Error, output file %s exists, use -A to append or -f to overwrite.\n",
-				output_name);
-		exit(-1);
+	if (!stat(output_name, &st) && st.st_size) {
+		if (!force && !append_file) {
+			fprintf(stderr, "Error, output file %s exists, use -A to append or -f to overwrite.\n",
+					output_name);
+			exit(-1);
+		}
+	} else {
+		append_file = 0;
 	}
 
 	flags = O_CREAT|O_RDWR;

[tip:perfcounters/urgent] perf util: Fix do_read() to fail on EOF instead of busy-looping

[tip-bot for Pierre Habouzit]

Commit-ID:  47efff9a9b958f1c459a4c669168be0de474a9e3
Gitweb:     http://git.kernel.org/tip/47efff9a9b958f1c459a4c669168be0de474a9e3
Author:     Pierre Habouzit <pierre.habouzit@intersec.com>
AuthorDate: Fri, 7 Aug 2009 14:16:00 +0200
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 7 Aug 2009 17:08:17 +0200

perf util: Fix do_read() to fail on EOF instead of busy-looping

While toying with perf, I've noticed that perf record can
easily enter a busy loop when doing something as silly as:

    $ perf record -A ls

Yeah, do_read here really wants to read a known size, not being
able to should die(), not busy-loop ;)

That was the cause for the bug.

Signed-off-by: Pierre Habouzit <pierre.habouzit@intersec.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
LKML-Reference: <1249647361-11582-2-git-send-email-pierre.habouzit@intersec.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 tools/perf/util/header.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 95a44bc..b92a457 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -185,6 +185,8 @@ static void do_read(int fd, void *buf, size_t size)
 
 		if (ret < 0)
 			die("failed to read");
+		if (ret == 0)
+			die("failed to read: missing data");
 
 		size -= ret;
 		buf += ret;

Re: perf-record fix and UI improvement

[Pierre Habouzit]

And of course I borked the patches, please fix the patches using a
From/Signed-off-by: Pierre Habouzit <pierre.habouzit@intersec.com>

TIA
-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

Re: perf-record fix and UI improvement

[Peter Zijlstra]

On Fri, 2009-08-07 at 14:15 +0200, Pierre Habouzit wrote:
> While toying with perf, I've noticed that perf record can easily enter
> a busy loop when doing something as silly as:
> 
>     $ perf record -A ls
> 
> I've searched why and here are the patches:
> 
>  [PATCH 1/2] perf util: do_read should fail on EOF instead of busy-looping.
> 
>    Yeah, do_read here really wants to read a known size, not being able
>    to should die(), not busy-lopp ;)
>    That was the cause for the bug.
> 
> 
>  [PATCH 2/2] perf-record: improve -A UI for empty or non-existent perf.data
> 
>    Though with 1/2 `git record -A ls` would then fail miserably with
>    some kind of "cannot read" error, which sucks. So this patch
>    understands -A as a "append or create if file is empty or inexistant"
> 
>    This fact may deserve to be documented properly, if so just tell me
>    I'll send an updated patch for Documentation/
> 
> 
> I'm kind of new to the kernel world, so I hope I sent the patches to the
> proper persons.

You did well for a first time ;-)

The things you can improve for next time are:

 - placing these nice descriptions you made above into the patches
themselves, as esp the first patch has an empty changelog.

 - get your email right :-)

Anyway, I think Ingo already fixed that up for you, so

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Re: perf-record fix and UI improvement

[Pierre Habouzit]

On Fri, Aug 07, 2009 at 03:01:06PM +0200, Peter Zijlstra wrote:
> On Fri, 2009-08-07 at 14:15 +0200, Pierre Habouzit wrote:
> > I'm kind of new to the kernel world, so I hope I sent the patches to the
> > proper persons.
> 
> You did well for a first time ;-)

Well, sending patches is not really a first time, I kind of wrote most
of util/strbuf.[hc] and util/parse-options.[hc] ;)


> The things you can improve for next time are:
> 
>  - placing these nice descriptions you made above into the patches
> themselves, as esp the first patch has an empty changelog.

Right.

>  - get your email right :-)

*blush*

> Anyway, I think Ingo already fixed that up for you, so
> 
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Wonderful, thanks.

-- 
Intersec <http://www.intersec.com>
Pierre Habouzit <pierre.habouzit@intersec.com>
Tél : +33 (0)1 5570 3346
Mob : +33 (0)6 1636 8131
Fax : +33 (0)1 5570 3332
37 Rue Pierre Lhomme
92400 Courbevoie

Re: perf-record fix and UI improvement

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Fri, 2009-08-07 at 14:15 +0200, Pierre Habouzit wrote:
> > While toying with perf, I've noticed that perf record can easily enter
> > a busy loop when doing something as silly as:
> > 
> >     $ perf record -A ls
> > 
> > I've searched why and here are the patches:
> > 
> >  [PATCH 1/2] perf util: do_read should fail on EOF instead of busy-looping.
> > 
> >    Yeah, do_read here really wants to read a known size, not being able
> >    to should die(), not busy-lopp ;)
> >    That was the cause for the bug.
> > 
> > 
> >  [PATCH 2/2] perf-record: improve -A UI for empty or non-existent perf.data
> > 
> >    Though with 1/2 `git record -A ls` would then fail miserably with
> >    some kind of "cannot read" error, which sucks. So this patch
> >    understands -A as a "append or create if file is empty or inexistant"
> > 
> >    This fact may deserve to be documented properly, if so just tell me
> >    I'll send an updated patch for Documentation/
> > 
> > 
> > I'm kind of new to the kernel world, so I hope I sent the patches to the
> > proper persons.
> 
> You did well for a first time ;-)
> 
> The things you can improve for next time are:
> 
>  - placing these nice descriptions you made above into the patches 
> themselves, as esp the first patch has an empty changelog.
> 
>  - get your email right :-)
> 
> Anyway, I think Ingo already fixed that up for you, so
> 
> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Thanks - i've applied it.

	Ingo