LinuxLists.cc - [PATCH 0/6] Minor cleanups & fixes for perf-probe(1)

[permalink] [raw]

Subject: [PATCH 1/6] perf-tools: make perf-probe -L display the absolute path of the dumped file

From: Franck Bui-Huu <[email protected]>

The actual file used by 'perf probe -L sched.c' is reported in the
ouput of the command.

But it's simply displayed as it has been given to the command (simply
sched.c) which is too ambiguous to be really usefull since several
sched.c files can be found into the same project and we also don't
know which search path has been used.

Signed-off-by: Franck Bui-Huu <[email protected]>
---
tools/perf/util/probe-event.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 558545e..6fa0403 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -381,7 +381,7 @@ int show_line_range(struct line_range *lr, const char *module)
fprintf(stdout, "<%s:%d>\n", lr->function,
lr->start - lr->offset);
else
- fprintf(stdout, "<%s:%d>\n", lr->file, lr->start);
+ fprintf(stdout, "<%s:%d>\n", lr->path, lr->start);

fp = fopen(lr->path, "r");
if (fp == NULL) {
--
1.7.3.2

2010-12-20 14:18:19

[permalink] [raw]

Subject: [PATCH 2/6] perf-probe: rewrite show_one_line() to make it simpler

From: Franck Bui-Huu <[email protected]>

Signed-off-by: Franck Bui-Huu <[email protected]>
---
tools/perf/util/probe-event.c | 29 +++++++++++------------------
1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 6fa0403..5cc8f6b 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -300,28 +300,21 @@ static int get_real_path(const char *raw_path, const char *comp_dir,
static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
{
char buf[LINEBUF_SIZE];
- const char *color = PERF_COLOR_BLUE;
+ const char *color = show_num ? "" : PERF_COLOR_BLUE;
+ const char *prefix = NULL;

- if (fgets(buf, LINEBUF_SIZE, fp) == NULL)
- goto error;
- if (!skip) {
- if (show_num)
- fprintf(stdout, "%7d %s", l, buf);
- else
- color_fprintf(stdout, color, " %s", buf);
- }
-
- while (strlen(buf) == LINEBUF_SIZE - 1 &&
- buf[LINEBUF_SIZE - 2] != '\n') {
+ do {
if (fgets(buf, LINEBUF_SIZE, fp) == NULL)
goto error;
- if (!skip) {
- if (show_num)
- fprintf(stdout, "%s", buf);
- else
- color_fprintf(stdout, color, "%s", buf);
+ if (skip)
+ continue;
+ if (!prefix) {
+ prefix = show_num ? "%7d " : " ";
+ fprintf(stdout, prefix, l);
}
- }
+ color_fprintf(stdout, color, "%s", buf);
+
+ } while (strchr(buf, '\n') == NULL);

return 0;
error:
--
1.7.3.2

2010-12-20 14:18:27

[permalink] [raw]

Subject: [PATCH 4/6] perf-probe: Fix line range description since a single file is allowed

From: Franck Bui-Huu <[email protected]>

$ perf-probe -L sched.c

is currently allowed but not documented.

Signed-off-by: Franck Bui-Huu <[email protected]>
---
tools/perf/Documentation/perf-probe.txt | 2 +-
tools/perf/util/probe-event.c | 13 ++++++++-----
2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/Documentation/perf-probe.txt b/tools/perf/Documentation/perf-probe.txt
index 62de1b7..562501f 100644
--- a/tools/perf/Documentation/perf-probe.txt
+++ b/tools/perf/Documentation/perf-probe.txt
@@ -117,7 +117,7 @@ LINE SYNTAX
-----------
Line range is descripted by following syntax.

- "FUNC[:RLN[+NUM|-RLN2]]|SRC:ALN[+NUM|-ALN2]"
+ "FUNC[:RLN[+NUM|-RLN2]]|SRC[:ALN[+NUM|-ALN2]]"

FUNC specifies the function name of showing lines. 'RLN' is the start line
number from function entry line, and 'RLN2' is the end line number. As same as
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 3c92b92..8e5f5ff 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -525,15 +525,18 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
}
#endif

+/*
+ * Stuff 'lr' according to the line range described by 'arg'.
+ * The line range syntax is described by:
+ *
+ * SRC[:SLN[+NUM|-ELN]]
+ * FNC[:SLN[+NUM|-ELN]]
+ */
int parse_line_range_desc(const char *arg, struct line_range *lr)
{
const char *ptr;
char *tmp;
- /*
- * <Syntax>
- * SRC:SLN[+NUM|-ELN]
- * FUNC[:SLN[+NUM|-ELN]]
- */
+
ptr = strchr(arg, ':');
if (ptr) {
lr->start = (int)strtoul(ptr + 1, &tmp, 0);
--
1.7.3.2

2010-12-20 14:18:32

[permalink] [raw]

Subject: [PATCH 6/6] perf-probe: handle gracefully some stupid and buggy line syntaxes

From: Franck Bui-Huu <[email protected]>

Currently perf-probe doesn't handle those incorrect syntaxes

$ perf probe -L sched.c:++13
$ perf probe -L sched.c:-+13
$ perf probe -L sched.c:10000000000000000000000000000+13

This patches rewrites parse_line_range_desc() to handle them.

As a bonus side, it reports more usefull error messages instead of:
"Tailing with invalid character...".

Signed-off-by: Franck Bui-Huu <[email protected]>
---
tools/perf/util/probe-event.c | 92 ++++++++++++++++++++++++++--------------
1 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 1e81936..469ad35 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -539,6 +539,19 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
}
#endif

+static int parse_line_num(char **ptr, int *val, const char *what)
+{
+ const char *start = *ptr;
+
+ errno = 0;
+ *val = strtol(*ptr, ptr, 0);
+ if (errno || *ptr == start) {
+ semantic_error("'%s' is not a valid number.\n", what);
+ return -EINVAL;
+ }
+ return 0;
+}
+
/*
* Stuff 'lr' according to the line range described by 'arg'.
* The line range syntax is described by:
@@ -548,50 +561,65 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
*/
int parse_line_range_desc(const char *arg, struct line_range *lr)
{
- const char *ptr;
- char *tmp;
+ char *range, *name = strdup(arg);
+ int err;
+
+ if (!name)
+ return -ENOMEM;
+
+ lr->start = 0;
+ lr->end = INT_MAX;
+
+ range = strchr(name, ':');
+ if (range) {
+ *range++ = '\0';
+
+ err = parse_line_num(&range, &lr->start, "start line");
+ if (err)
+ goto err;
+
+ if (*range == '+' || *range == '-') {
+ const char c = *range++;
+
+ err = parse_line_num(&range, &lr->end, "end line");
+ if (err)
+ goto err;
+
+ if (c == '+') {
+ lr->end += lr->start;
+ /*
+ * Adjust the number of lines here.
+ * If the number of lines == 1, the
+ * the end of line should be equal to
+ * the start of line.
+ */
+ lr->end--;
+ }
+ }

- ptr = strchr(arg, ':');
- if (ptr) {
- lr->start = (int)strtoul(ptr + 1, &tmp, 0);
- if (*tmp == '+') {
- lr->end = lr->start + (int)strtoul(tmp + 1, &tmp, 0);
- lr->end--; /*
- * Adjust the number of lines here.
- * If the number of lines == 1, the
- * the end of line should be equal to
- * the start of line.
- */
- } else if (*tmp == '-')
- lr->end = (int)strtoul(tmp + 1, &tmp, 0);
- else
- lr->end = INT_MAX;
pr_debug("Line range is %d to %d\n", lr->start, lr->end);
+
+ err = -EINVAL;
if (lr->start > lr->end) {
semantic_error("Start line must be smaller"
" than end line.\n");
- return -EINVAL;
+ goto err;
}
- if (*tmp != '\0') {
- semantic_error("Tailing with invalid character '%d'.\n",
- *tmp);
- return -EINVAL;
+ if (*range != '\0') {
+ semantic_error("Tailing with invalid str '%s'.\n", range);
+ goto err;
}
- tmp = strndup(arg, (ptr - arg));
- } else {
- tmp = strdup(arg);
- lr->end = INT_MAX;
}

- if (tmp == NULL)
- return -ENOMEM;
-
- if (strchr(tmp, '.'))
- lr->file = tmp;
+ if (strchr(name, '.'))
+ lr->file = name;
else
- lr->function = tmp;
+ lr->function = name;

return 0;
+err:
+ free(name);
+ return err;
}

/* Check the name is good for event/group */
--
1.7.3.2

2010-12-20 14:18:22

[permalink] [raw]

Subject: [PATCH 3/6] perf-probe: clean up redundant tests in show_line_range()

From: Franck Bui-Huu <[email protected]>

It also removes some superflous parentheses.

Signed-off-by: Franck Bui-Huu <[email protected]>
---
tools/perf/util/probe-event.c | 26 +++++++++++++++-----------
1 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 5cc8f6b..3c92b92 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -383,26 +383,30 @@ int show_line_range(struct line_range *lr, const char *module)
return -errno;
}
/* Skip to starting line number */
- while (l < lr->start && ret >= 0)
+ while (l < lr->start) {
ret = show_one_line(fp, l++, true, false);
- if (ret < 0)
- goto end;
+ if (ret < 0)
+ goto end;
+ }

list_for_each_entry(ln, &lr->line_list, list) {
- while (ln->line > l && ret >= 0)
- ret = show_one_line(fp, (l++) - lr->offset,
- false, false);
- if (ret >= 0)
- ret = show_one_line(fp, (l++) - lr->offset,
- false, true);
+ for (; ln->line > l; l++) {
+ ret = show_one_line(fp, l - lr->offset, false, false);
+ if (ret < 0)
+ goto end;
+ }
+ ret = show_one_line(fp, l++ - lr->offset, false, true);
if (ret < 0)
goto end;
}

if (lr->end == INT_MAX)
lr->end = l + NR_ADDITIONAL_LINES;
- while (l <= lr->end && !feof(fp) && ret >= 0)
- ret = show_one_line(fp, (l++) - lr->offset, false, false);
+ while (l <= lr->end && !feof(fp)) {
+ ret = show_one_line(fp, l++ - lr->offset, false, false);
+ if (ret < 0)
+ break;
+ }
end:
fclose(fp);
return ret;
--
1.7.3.2

2010-12-20 14:18:30

[permalink] [raw]

Subject: [PATCH 5/6] perf-probe: don't always consider EOF as an error when listing source code

From: Franck Bui-Huu <[email protected]>

When listing a whole file or a function which is located at the end,
perf-probe -L output wrongly: "Source file is shorter than expected.".

This is because show_one_line() always consider EOF as an error.

This patch fixes this by not considering EOF as an error when dumping
the trailing lines. Otherwise it's still an error and perf-probe still
outputs its warning.

Signed-off-by: Franck Bui-Huu <[email protected]>
---
tools/perf/util/probe-event.c | 38 ++++++++++++++++++++++++++------------
1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 8e5f5ff..1e81936 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -297,7 +297,7 @@ static int get_real_path(const char *raw_path, const char *comp_dir,
#define LINEBUF_SIZE 256
#define NR_ADDITIONAL_LINES 2

-static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
+static int __show_one_line(FILE *fp, int l, bool skip, bool show_num)
{
char buf[LINEBUF_SIZE];
const char *color = show_num ? "" : PERF_COLOR_BLUE;
@@ -316,16 +316,30 @@ static int show_one_line(FILE *fp, int l, bool skip, bool show_num)

} while (strchr(buf, '\n') == NULL);

- return 0;
+ return 1;
error:
- if (feof(fp))
+ if (ferror(fp)) {
pr_warning("Source file is shorter than expected.\n");
- else
- pr_warning("File read error: %s\n", strerror(errno));
+ return -1;
+ }
+ return 0;
+}

- return -1;
+static int _show_one_line(FILE *fp, int l, bool skip, bool show_num)
+{
+ int rv = __show_one_line(fp, l, skip, show_num);
+ if (rv == 0) {
+ pr_warning("Source file is shorter than expected.\n");
+ rv = -1;
+ }
+ return rv;
}

+#define show_one_line_with_num(f,l) _show_one_line(f,l,false,true)
+#define show_one_line(f,l) _show_one_line(f,l,false,false)
+#define skip_one_line(f,l) _show_one_line(f,l,true,false)
+#define show_one_line_or_eof(f,l) __show_one_line(f,l,false,false)
+
/*
* Show line-range always requires debuginfo to find source file and
* line number.
@@ -384,27 +398,27 @@ int show_line_range(struct line_range *lr, const char *module)
}
/* Skip to starting line number */
while (l < lr->start) {
- ret = show_one_line(fp, l++, true, false);
+ ret = skip_one_line(fp, l++);
if (ret < 0)
goto end;
}

list_for_each_entry(ln, &lr->line_list, list) {
for (; ln->line > l; l++) {
- ret = show_one_line(fp, l - lr->offset, false, false);
+ ret = show_one_line(fp, l - lr->offset);
if (ret < 0)
goto end;
}
- ret = show_one_line(fp, l++ - lr->offset, false, true);
+ ret = show_one_line_with_num(fp, l++ - lr->offset);
if (ret < 0)
goto end;
}

if (lr->end == INT_MAX)
lr->end = l + NR_ADDITIONAL_LINES;
- while (l <= lr->end && !feof(fp)) {
- ret = show_one_line(fp, l++ - lr->offset, false, false);
- if (ret < 0)
+ while (l <= lr->end) {
+ ret = show_one_line_or_eof(fp, l++ - lr->offset);
+ if (ret <= 0)
break;
}
end:
--
1.7.3.2

2010-12-21 05:30:10

[permalink] [raw]

Subject: Re: [PATCH 1/6] perf-tools: make perf-probe -L display the absolute path of the dumped file

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <[email protected]>
>
> The actual file used by 'perf probe -L sched.c' is reported in the
> ouput of the command.
>
> But it's simply displayed as it has been given to the command (simply
> sched.c) which is too ambiguous to be really usefull since several
> sched.c files can be found into the same project and we also don't
> know which search path has been used.

It's enough reasonable for me.

Acked-by: Masami Hiramatsu <[email protected]>

>
> Signed-off-by: Franck Bui-Huu <[email protected]>
> ---
> tools/perf/util/probe-event.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 558545e..6fa0403 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -381,7 +381,7 @@ int show_line_range(struct line_range *lr, const char *module)
> fprintf(stdout, "<%s:%d>\n", lr->function,
> lr->start - lr->offset);
> else
> - fprintf(stdout, "<%s:%d>\n", lr->file, lr->start);
> + fprintf(stdout, "<%s:%d>\n", lr->path, lr->start);
>
> fp = fopen(lr->path, "r");
> if (fp == NULL) {

--
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: [email protected]

2010-12-21 08:55:36

[permalink] [raw]

Subject: Re: [PATCH 2/6] perf-probe: rewrite show_one_line() to make it simpler

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <[email protected]>

Please explain what this patch will do...

>
> Signed-off-by: Franck Bui-Huu <[email protected]>
> ---
> tools/perf/util/probe-event.c | 29 +++++++++++------------------
> 1 files changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 6fa0403..5cc8f6b 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -300,28 +300,21 @@ static int get_real_path(const char *raw_path, const char *comp_dir,
> static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
> {
> char buf[LINEBUF_SIZE];
> - const char *color = PERF_COLOR_BLUE;
> + const char *color = show_num ? "" : PERF_COLOR_BLUE;
> + const char *prefix = NULL;
>
> - if (fgets(buf, LINEBUF_SIZE, fp) == NULL)
> - goto error;
> - if (!skip) {
> - if (show_num)
> - fprintf(stdout, "%7d %s", l, buf);
> - else
> - color_fprintf(stdout, color, " %s", buf);
> - }
> -
> - while (strlen(buf) == LINEBUF_SIZE - 1 &&
> - buf[LINEBUF_SIZE - 2] != '\n') {
> + do {
> if (fgets(buf, LINEBUF_SIZE, fp) == NULL)
> goto error;
> - if (!skip) {
> - if (show_num)
> - fprintf(stdout, "%s", buf);
> - else
> - color_fprintf(stdout, color, "%s", buf);
> + if (skip)
> + continue;
> + if (!prefix) {
> + prefix = show_num ? "%7d " : " ";
> + fprintf(stdout, prefix, l);

Could you use color_fprintf() here too?
I know currently it's meaningless, but from the view of maintaining,
it's better to use same function.

Thank you,

> }
> - }
> + color_fprintf(stdout, color, "%s", buf);
> +
> + } while (strchr(buf, '\n') == NULL);
>
> return 0;
> error:

--
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: [email protected]

2010-12-21 09:47:54

[permalink] [raw]

Subject: Re: [PATCH 3/6] perf-probe: clean up redundant tests in show_line_range()

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <[email protected]>
>
> It also removes some superflous parentheses.

Oh, don't remove your superfluous comments too! ;-)

>
> Signed-off-by: Franck Bui-Huu <[email protected]>
> ---
> tools/perf/util/probe-event.c | 26 +++++++++++++++-----------
> 1 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 5cc8f6b..3c92b92 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -383,26 +383,30 @@ int show_line_range(struct line_range *lr, const char *module)
> return -errno;
> }
> /* Skip to starting line number */
> - while (l < lr->start && ret >= 0)
> + while (l < lr->start) {
> ret = show_one_line(fp, l++, true, false);
> - if (ret < 0)
> - goto end;
> + if (ret < 0)
> + goto end;
> + }
>
> list_for_each_entry(ln, &lr->line_list, list) {
> - while (ln->line > l && ret >= 0)
> - ret = show_one_line(fp, (l++) - lr->offset,
> - false, false);
> - if (ret >= 0)
> - ret = show_one_line(fp, (l++) - lr->offset,
> - false, true);
> + for (; ln->line > l; l++) {
> + ret = show_one_line(fp, l - lr->offset, false, false);
> + if (ret < 0)
> + goto end;
> + }
> + ret = show_one_line(fp, l++ - lr->offset, false, true);
> if (ret < 0)
> goto end;
> }
>
> if (lr->end == INT_MAX)
> lr->end = l + NR_ADDITIONAL_LINES;
> - while (l <= lr->end && !feof(fp) && ret >= 0)
> - ret = show_one_line(fp, (l++) - lr->offset, false, false);
> + while (l <= lr->end && !feof(fp)) {
> + ret = show_one_line(fp, l++ - lr->offset, false, false);
> + if (ret < 0)
> + break;
> + }
> end:
> fclose(fp);
> return ret;

--
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: [email protected]

2010-12-21 11:54:57

[permalink] [raw]

Subject: Re: [PATCH 5/6] perf-probe: don't always consider EOF as an error when listing source code

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <[email protected]>
>
> When listing a whole file or a function which is located at the end,
> perf-probe -L output wrongly: "Source file is shorter than expected.".
>
> This is because show_one_line() always consider EOF as an error.

Right, that was a wrong behavior.

>
> This patch fixes this by not considering EOF as an error when dumping
> the trailing lines. Otherwise it's still an error and perf-probe still
> outputs its warning.

Thank you for fixing the bug! :)

Acked-by: Masami Hiramatsu <[email protected]>

>
> Signed-off-by: Franck Bui-Huu <[email protected]>
> ---
> tools/perf/util/probe-event.c | 38 ++++++++++++++++++++++++++------------
> 1 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 8e5f5ff..1e81936 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -297,7 +297,7 @@ static int get_real_path(const char *raw_path, const char *comp_dir,
> #define LINEBUF_SIZE 256
> #define NR_ADDITIONAL_LINES 2
>
> -static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
> +static int __show_one_line(FILE *fp, int l, bool skip, bool show_num)
> {
> char buf[LINEBUF_SIZE];
> const char *color = show_num ? "" : PERF_COLOR_BLUE;
> @@ -316,16 +316,30 @@ static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
>
> } while (strchr(buf, '\n') == NULL);
>
> - return 0;
> + return 1;
> error:
> - if (feof(fp))
> + if (ferror(fp)) {
> pr_warning("Source file is shorter than expected.\n");
> - else
> - pr_warning("File read error: %s\n", strerror(errno));
> + return -1;
> + }
> + return 0;
> +}
>
> - return -1;
> +static int _show_one_line(FILE *fp, int l, bool skip, bool show_num)
> +{
> + int rv = __show_one_line(fp, l, skip, show_num);
> + if (rv == 0) {
> + pr_warning("Source file is shorter than expected.\n");
> + rv = -1;
> + }
> + return rv;
> }
>
> +#define show_one_line_with_num(f,l) _show_one_line(f,l,false,true)
> +#define show_one_line(f,l) _show_one_line(f,l,false,false)
> +#define skip_one_line(f,l) _show_one_line(f,l,true,false)
> +#define show_one_line_or_eof(f,l) __show_one_line(f,l,false,false)
> +
> /*
> * Show line-range always requires debuginfo to find source file and
> * line number.
> @@ -384,27 +398,27 @@ int show_line_range(struct line_range *lr, const char *module)
> }
> /* Skip to starting line number */
> while (l < lr->start) {
> - ret = show_one_line(fp, l++, true, false);
> + ret = skip_one_line(fp, l++);
> if (ret < 0)
> goto end;
> }
>
> list_for_each_entry(ln, &lr->line_list, list) {
> for (; ln->line > l; l++) {
> - ret = show_one_line(fp, l - lr->offset, false, false);
> + ret = show_one_line(fp, l - lr->offset);
> if (ret < 0)
> goto end;
> }
> - ret = show_one_line(fp, l++ - lr->offset, false, true);
> + ret = show_one_line_with_num(fp, l++ - lr->offset);
> if (ret < 0)
> goto end;
> }
>
> if (lr->end == INT_MAX)
> lr->end = l + NR_ADDITIONAL_LINES;
> - while (l <= lr->end && !feof(fp)) {
> - ret = show_one_line(fp, l++ - lr->offset, false, false);
> - if (ret < 0)
> + while (l <= lr->end) {
> + ret = show_one_line_or_eof(fp, l++ - lr->offset);
> + if (ret <= 0)
> break;
> }
> end:

--
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: [email protected]

2010-12-21 13:11:06

[permalink] [raw]

Subject: Re: [PATCH 6/6] perf-probe: handle gracefully some stupid and buggy line syntaxes

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <[email protected]>
>
> Currently perf-probe doesn't handle those incorrect syntaxes
>
> $ perf probe -L sched.c:++13
> $ perf probe -L sched.c:-+13
> $ perf probe -L sched.c:10000000000000000000000000000+13
>
> This patches rewrites parse_line_range_desc() to handle them.

OK, without this patch, perf probe accepted that as "0+13".
With this, perf probe rejects it as an invalid input.

Thank you!

>
> As a bonus side, it reports more usefull error messages instead of:
> "Tailing with invalid character...".
>
> Signed-off-by: Franck Bui-Huu <[email protected]>

Acked-by: Masami Hiramatsu <[email protected]>

> ---
> tools/perf/util/probe-event.c | 92 ++++++++++++++++++++++++++--------------
> 1 files changed, 60 insertions(+), 32 deletions(-)
>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 1e81936..469ad35 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -539,6 +539,19 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
> }
> #endif
>
> +static int parse_line_num(char **ptr, int *val, const char *what)
> +{
> + const char *start = *ptr;
> +
> + errno = 0;
> + *val = strtol(*ptr, ptr, 0);
> + if (errno || *ptr == start) {
> + semantic_error("'%s' is not a valid number.\n", what);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> /*
> * Stuff 'lr' according to the line range described by 'arg'.
> * The line range syntax is described by:
> @@ -548,50 +561,65 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
> */
> int parse_line_range_desc(const char *arg, struct line_range *lr)
> {
> - const char *ptr;
> - char *tmp;
> + char *range, *name = strdup(arg);
> + int err;
> +
> + if (!name)
> + return -ENOMEM;
> +
> + lr->start = 0;
> + lr->end = INT_MAX;
> +
> + range = strchr(name, ':');
> + if (range) {
> + *range++ = '\0';
> +
> + err = parse_line_num(&range, &lr->start, "start line");
> + if (err)
> + goto err;
> +
> + if (*range == '+' || *range == '-') {
> + const char c = *range++;
> +
> + err = parse_line_num(&range, &lr->end, "end line");
> + if (err)
> + goto err;
> +
> + if (c == '+') {
> + lr->end += lr->start;
> + /*
> + * Adjust the number of lines here.
> + * If the number of lines == 1, the
> + * the end of line should be equal to
> + * the start of line.
> + */
> + lr->end--;
> + }
> + }
>
> - ptr = strchr(arg, ':');
> - if (ptr) {
> - lr->start = (int)strtoul(ptr + 1, &tmp, 0);
> - if (*tmp == '+') {
> - lr->end = lr->start + (int)strtoul(tmp + 1, &tmp, 0);
> - lr->end--; /*
> - * Adjust the number of lines here.
> - * If the number of lines == 1, the
> - * the end of line should be equal to
> - * the start of line.
> - */
> - } else if (*tmp == '-')
> - lr->end = (int)strtoul(tmp + 1, &tmp, 0);
> - else
> - lr->end = INT_MAX;
> pr_debug("Line range is %d to %d\n", lr->start, lr->end);
> +
> + err = -EINVAL;
> if (lr->start > lr->end) {
> semantic_error("Start line must be smaller"
> " than end line.\n");
> - return -EINVAL;
> + goto err;
> }
> - if (*tmp != '\0') {
> - semantic_error("Tailing with invalid character '%d'.\n",
> - *tmp);
> - return -EINVAL;
> + if (*range != '\0') {
> + semantic_error("Tailing with invalid str '%s'.\n", range);
> + goto err;
> }
> - tmp = strndup(arg, (ptr - arg));
> - } else {
> - tmp = strdup(arg);
> - lr->end = INT_MAX;
> }
>
> - if (tmp == NULL)
> - return -ENOMEM;
> -
> - if (strchr(tmp, '.'))
> - lr->file = tmp;
> + if (strchr(name, '.'))
> + lr->file = name;
> else
> - lr->function = tmp;
> + lr->function = name;
>
> return 0;
> +err:
> + free(name);
> + return err;
> }
>
> /* Check the name is good for event/group */

--
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: [email protected]

2010-12-21 13:11:57

[permalink] [raw]

Subject: Re: [PATCH 0/6] Minor cleanups & fixes for perf-probe(1)

(2010/12/20 23:17), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <[email protected]>
>
> Hi,
>
> This small patchset is just some random cosmetic cleanups I did when
> starting to read perf-probe(1) stuffs.

Hi Franck,

Basically the contains of this series are good to me.
(I've sent my comments)
however, it's less comments on each patch. Please explain
what you did in each patch! (not only on the subject line:-))

Thank you,

>
> The 3 last patches of this serie are minor fixes though.
>
> ---
>
> Franck Bui-Huu (6):
> perf-tools: make perf-probe -L display the absolute path of the dumped file
> perf-probe: rewrite show_one_line() to make it simpler
> perf-probe: clean up redundant tests in show_line_range()
> perf-probe: Fix line range description since a single file is allowed
> perf-probe: don't always consider EOF as an error when listing source code
> perf-probe: handle gracefully some stupid and buggy line syntaxes
>
> tools/perf/Documentation/perf-probe.txt | 2 +-
> tools/perf/util/probe-event.c | 190 +++++++++++++++++++------------
> 2 files changed, 117 insertions(+), 75 deletions(-)
>

--
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: [email protected]

2010-12-21 18:01:23

[permalink] [raw]

Subject: Re: [PATCH 0/6] Minor cleanups & fixes for perf-probe(1)

Masami Hiramatsu <[email protected]> writes:

> (2010/12/20 23:17), Franck Bui-Huu wrote:
>>
>> This small patchset is just some random cosmetic cleanups I did when
>> starting to read perf-probe(1) stuffs.
>
> Basically the contains of this series are good to me.
> (I've sent my comments)
> however, it's less comments on each patch. Please explain
> what you did in each patch! (not only on the subject line:-))

Ok. I thought those changes in this serie was obvious enough.

> Thank you,

You're welcome.

--
Franck

2010-12-21 21:10:21

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [PATCH 0/6] Minor cleanups & fixes for perf-probe(1)

Em Tue, Dec 21, 2010 at 07:01:06PM +0100, Franck Bui-Huu escreveu:
> Masami Hiramatsu <[email protected]> writes:
> > (2010/12/20 23:17), Franck Bui-Huu wrote:
> >> This small patchset is just some random cosmetic cleanups I did when
> >> starting to read perf-probe(1) stuffs.

> > Basically the contains of this series are good to me. (I've sent my
> > comments) however, it's less comments on each patch. Please explain
> > what you did in each patch! (not only on the subject line:-))

> Ok. I thought those changes in this serie was obvious enough.
>
> > Thank you,
>
> You're welcome.

I did just that s/fprintf/color_fprintf/g change Masami asked and
stashed everything in my perf/core branch.

Thanks,

- Arnaldo

2010-12-22 11:31:00

[permalink] [raw]

Subject: [tip:perf/core] perf probe: Make -L display the absolute path of the dumped file

Commit-ID: 62c15fc49bd1b35d79b34ea96f132ab435e2215a
Gitweb: http://git.kernel.org/tip/62c15fc49bd1b35d79b34ea96f132ab435e2215a
Author: Franck Bui-Huu <[email protected]>
AuthorDate: Mon, 20 Dec 2010 15:18:00 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 21 Dec 2010 16:20:11 -0200

perf probe: Make -L display the absolute path of the dumped file

The actual file used by 'perf probe -L sched.c' is reported in the ouput
of the command.

But it's simply displayed as it has been given to the command (simply
sched.c) which is too ambiguous to be really usefull since several
sched.c files can be found into the same project and we also don't know
which search path has been used.

Acked-by: Masami Hiramatsu <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Franck Bui-Huu <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/probe-event.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 089d78e..0163fc0 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -371,7 +371,7 @@ int show_line_range(struct line_range *lr, const char *module)
fprintf(stdout, "<%s:%d>\n", lr->function,
lr->start - lr->offset);
else
- fprintf(stdout, "<%s:%d>\n", lr->file, lr->start);
+ fprintf(stdout, "<%s:%d>\n", lr->path, lr->start);

fp = fopen(lr->path, "r");
if (fp == NULL) {

2010-12-22 11:31:11

[permalink] [raw]

Subject: [tip:perf/core] perf probe: Rewrite show_one_line() to make it simpler

Commit-ID: befe341468f4e61ecaf337a0237f2aab76817437
Gitweb: http://git.kernel.org/tip/befe341468f4e61ecaf337a0237f2aab76817437
Author: Franck Bui-Huu <[email protected]>
AuthorDate: Mon, 20 Dec 2010 15:18:01 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 21 Dec 2010 16:20:11 -0200

perf probe: Rewrite show_one_line() to make it simpler

Cc: Masami Hiramatsu <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Franck Bui-Huu <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/probe-event.c | 29 +++++++++++------------------
1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 0163fc0..327604c 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -290,28 +290,21 @@ static int get_real_path(const char *raw_path, const char *comp_dir,
static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
{
char buf[LINEBUF_SIZE];
- const char *color = PERF_COLOR_BLUE;
+ const char *color = show_num ? "" : PERF_COLOR_BLUE;
+ const char *prefix = NULL;

- if (fgets(buf, LINEBUF_SIZE, fp) == NULL)
- goto error;
- if (!skip) {
- if (show_num)
- fprintf(stdout, "%7d %s", l, buf);
- else
- color_fprintf(stdout, color, " %s", buf);
- }
-
- while (strlen(buf) == LINEBUF_SIZE - 1 &&
- buf[LINEBUF_SIZE - 2] != '\n') {
+ do {
if (fgets(buf, LINEBUF_SIZE, fp) == NULL)
goto error;
- if (!skip) {
- if (show_num)
- fprintf(stdout, "%s", buf);
- else
- color_fprintf(stdout, color, "%s", buf);
+ if (skip)
+ continue;
+ if (!prefix) {
+ prefix = show_num ? "%7d " : " ";
+ color_fprintf(stdout, color, prefix, l);
}
- }
+ color_fprintf(stdout, color, "%s", buf);
+
+ } while (strchr(buf, '\n') == NULL);

return 0;
error:

2010-12-22 11:31:26

[permalink] [raw]

Subject: [tip:perf/core] perf probe: Clean up redundant tests in show_line_range()

Commit-ID: 44b81e929b0c00e703a31a3d634b668bb27eb1c8
Gitweb: http://git.kernel.org/tip/44b81e929b0c00e703a31a3d634b668bb27eb1c8
Author: Franck Bui-Huu <[email protected]>
AuthorDate: Mon, 20 Dec 2010 15:18:02 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 21 Dec 2010 16:20:12 -0200

perf probe: Clean up redundant tests in show_line_range()

It also removes some superflous parentheses.

Cc: Masami Hiramatsu <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Franck Bui-Huu <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/probe-event.c | 26 +++++++++++++++-----------
1 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 327604c..b812f14 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -373,26 +373,30 @@ int show_line_range(struct line_range *lr, const char *module)
return -errno;
}
/* Skip to starting line number */
- while (l < lr->start && ret >= 0)
+ while (l < lr->start) {
ret = show_one_line(fp, l++, true, false);
- if (ret < 0)
- goto end;
+ if (ret < 0)
+ goto end;
+ }

list_for_each_entry(ln, &lr->line_list, list) {
- while (ln->line > l && ret >= 0)
- ret = show_one_line(fp, (l++) - lr->offset,
- false, false);
- if (ret >= 0)
- ret = show_one_line(fp, (l++) - lr->offset,
- false, true);
+ for (; ln->line > l; l++) {
+ ret = show_one_line(fp, l - lr->offset, false, false);
+ if (ret < 0)
+ goto end;
+ }
+ ret = show_one_line(fp, l++ - lr->offset, false, true);
if (ret < 0)
goto end;
}

if (lr->end == INT_MAX)
lr->end = l + NR_ADDITIONAL_LINES;
- while (l <= lr->end && !feof(fp) && ret >= 0)
- ret = show_one_line(fp, (l++) - lr->offset, false, false);
+ while (l <= lr->end && !feof(fp)) {
+ ret = show_one_line(fp, l++ - lr->offset, false, false);
+ if (ret < 0)
+ break;
+ }
end:
fclose(fp);
return ret;

2010-12-22 11:31:53

[permalink] [raw]

Subject: [tip:perf/core] perf probe: Fix line range description since a single file is allowed

Commit-ID: 9d95b580a8d64ef4d1660a21a9de0658fe29f041
Gitweb: http://git.kernel.org/tip/9d95b580a8d64ef4d1660a21a9de0658fe29f041
Author: Franck Bui-Huu <[email protected]>
AuthorDate: Mon, 20 Dec 2010 15:18:03 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 21 Dec 2010 16:20:12 -0200

perf probe: Fix line range description since a single file is allowed

$ perf-probe -L sched.c

is currently allowed but not documented.

Cc: Masami Hiramatsu <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Franck Bui-Huu <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/Documentation/perf-probe.txt | 2 +-
tools/perf/util/probe-event.c | 13 ++++++++-----
2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/perf/Documentation/perf-probe.txt b/tools/perf/Documentation/perf-probe.txt
index 4e23232..86b797a 100644
--- a/tools/perf/Documentation/perf-probe.txt
+++ b/tools/perf/Documentation/perf-probe.txt
@@ -117,7 +117,7 @@ LINE SYNTAX
-----------
Line range is described by following syntax.

- "FUNC[:RLN[+NUM|-RLN2]]|SRC:ALN[+NUM|-ALN2]"
+ "FUNC[:RLN[+NUM|-RLN2]]|SRC[:ALN[+NUM|-ALN2]]"

FUNC specifies the function name of showing lines. 'RLN' is the start line
number from function entry line, and 'RLN2' is the end line number. As same as
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index b812f14..3ba9c53 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -515,15 +515,18 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
}
#endif

+/*
+ * Stuff 'lr' according to the line range described by 'arg'.
+ * The line range syntax is described by:
+ *
+ * SRC[:SLN[+NUM|-ELN]]
+ * FNC[:SLN[+NUM|-ELN]]
+ */
int parse_line_range_desc(const char *arg, struct line_range *lr)
{
const char *ptr;
char *tmp;
- /*
- * <Syntax>
- * SRC:SLN[+NUM|-ELN]
- * FUNC[:SLN[+NUM|-ELN]]
- */
+
ptr = strchr(arg, ':');
if (ptr) {
lr->start = (int)strtoul(ptr + 1, &tmp, 0);

2010-12-22 11:32:15

[permalink] [raw]

Subject: [tip:perf/core] perf probe: Don't always consider EOF as an error when listing source code

Commit-ID: fde52dbd7f71934aba4e150f3d1d51e826a08850
Gitweb: http://git.kernel.org/tip/fde52dbd7f71934aba4e150f3d1d51e826a08850
Author: Franck Bui-Huu <[email protected]>
AuthorDate: Mon, 20 Dec 2010 15:18:04 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 21 Dec 2010 16:20:12 -0200

perf probe: Don't always consider EOF as an error when listing source code

When listing a whole file or a function which is located at the end,
perf-probe -L output wrongly: "Source file is shorter than expected.".

This is because show_one_line() always consider EOF as an error.

This patch fixes this by not considering EOF as an error when dumping
the trailing lines. Otherwise it's still an error and perf-probe still
outputs its warning.

Acked-by: Masami Hiramatsu <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Franck Bui-Huu <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/probe-event.c | 38 ++++++++++++++++++++++++++------------
1 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 3ba9c53..80cc0bc 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -287,7 +287,7 @@ static int get_real_path(const char *raw_path, const char *comp_dir,
#define LINEBUF_SIZE 256
#define NR_ADDITIONAL_LINES 2

-static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
+static int __show_one_line(FILE *fp, int l, bool skip, bool show_num)
{
char buf[LINEBUF_SIZE];
const char *color = show_num ? "" : PERF_COLOR_BLUE;
@@ -306,16 +306,30 @@ static int show_one_line(FILE *fp, int l, bool skip, bool show_num)

} while (strchr(buf, '\n') == NULL);

- return 0;
+ return 1;
error:
- if (feof(fp))
+ if (ferror(fp)) {
pr_warning("Source file is shorter than expected.\n");
- else
- pr_warning("File read error: %s\n", strerror(errno));
+ return -1;
+ }
+ return 0;
+}

- return -1;
+static int _show_one_line(FILE *fp, int l, bool skip, bool show_num)
+{
+ int rv = __show_one_line(fp, l, skip, show_num);
+ if (rv == 0) {
+ pr_warning("Source file is shorter than expected.\n");
+ rv = -1;
+ }
+ return rv;
}

+#define show_one_line_with_num(f,l) _show_one_line(f,l,false,true)
+#define show_one_line(f,l) _show_one_line(f,l,false,false)
+#define skip_one_line(f,l) _show_one_line(f,l,true,false)
+#define show_one_line_or_eof(f,l) __show_one_line(f,l,false,false)
+
/*
* Show line-range always requires debuginfo to find source file and
* line number.
@@ -374,27 +388,27 @@ int show_line_range(struct line_range *lr, const char *module)
}
/* Skip to starting line number */
while (l < lr->start) {
- ret = show_one_line(fp, l++, true, false);
+ ret = skip_one_line(fp, l++);
if (ret < 0)
goto end;
}

list_for_each_entry(ln, &lr->line_list, list) {
for (; ln->line > l; l++) {
- ret = show_one_line(fp, l - lr->offset, false, false);
+ ret = show_one_line(fp, l - lr->offset);
if (ret < 0)
goto end;
}
- ret = show_one_line(fp, l++ - lr->offset, false, true);
+ ret = show_one_line_with_num(fp, l++ - lr->offset);
if (ret < 0)
goto end;
}

if (lr->end == INT_MAX)
lr->end = l + NR_ADDITIONAL_LINES;
- while (l <= lr->end && !feof(fp)) {
- ret = show_one_line(fp, l++ - lr->offset, false, false);
- if (ret < 0)
+ while (l <= lr->end) {
+ ret = show_one_line_or_eof(fp, l++ - lr->offset);
+ if (ret <= 0)
break;
}
end:

2010-12-22 11:32:36

[permalink] [raw]

Subject: [tip:perf/core] perf probe: Handle gracefully some stupid and buggy line syntaxes

Commit-ID: 21dd9ae5a4e9f717f3957ec934dd3158129436b8
Gitweb: http://git.kernel.org/tip/21dd9ae5a4e9f717f3957ec934dd3158129436b8
Author: Franck Bui-Huu <[email protected]>
AuthorDate: Mon, 20 Dec 2010 15:18:05 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 21 Dec 2010 17:20:13 -0200

perf probe: Handle gracefully some stupid and buggy line syntaxes

Currently perf probe doesn't handle those incorrect syntaxes:

$ perf probe -L sched.c:++13
$ perf probe -L sched.c:-+13
$ perf probe -L sched.c:10000000000000000000000000000+13

This patches rewrites parse_line_range_desc() to handle them.

As a bonus, it reports more useful error messages instead of: "Tailing
with invalid character...".

Acked-by: Masami Hiramatsu <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Franck Bui-Huu <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/probe-event.c | 92 ++++++++++++++++++++++++++--------------
1 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 80cc0bc..099336e 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -529,6 +529,19 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
}
#endif

+static int parse_line_num(char **ptr, int *val, const char *what)
+{
+ const char *start = *ptr;
+
+ errno = 0;
+ *val = strtol(*ptr, ptr, 0);
+ if (errno || *ptr == start) {
+ semantic_error("'%s' is not a valid number.\n", what);
+ return -EINVAL;
+ }
+ return 0;
+}
+
/*
* Stuff 'lr' according to the line range described by 'arg'.
* The line range syntax is described by:
@@ -538,50 +551,65 @@ int show_available_vars(struct perf_probe_event *pevs __unused,
*/
int parse_line_range_desc(const char *arg, struct line_range *lr)
{
- const char *ptr;
- char *tmp;
+ char *range, *name = strdup(arg);
+ int err;
+
+ if (!name)
+ return -ENOMEM;
+
+ lr->start = 0;
+ lr->end = INT_MAX;
+
+ range = strchr(name, ':');
+ if (range) {
+ *range++ = '\0';
+
+ err = parse_line_num(&range, &lr->start, "start line");
+ if (err)
+ goto err;
+
+ if (*range == '+' || *range == '-') {
+ const char c = *range++;
+
+ err = parse_line_num(&range, &lr->end, "end line");
+ if (err)
+ goto err;
+
+ if (c == '+') {
+ lr->end += lr->start;
+ /*
+ * Adjust the number of lines here.
+ * If the number of lines == 1, the
+ * the end of line should be equal to
+ * the start of line.
+ */
+ lr->end--;
+ }
+ }

- ptr = strchr(arg, ':');
- if (ptr) {
- lr->start = (int)strtoul(ptr + 1, &tmp, 0);
- if (*tmp == '+') {
- lr->end = lr->start + (int)strtoul(tmp + 1, &tmp, 0);
- lr->end--; /*
- * Adjust the number of lines here.
- * If the number of lines == 1, the
- * the end of line should be equal to
- * the start of line.
- */
- } else if (*tmp == '-')
- lr->end = (int)strtoul(tmp + 1, &tmp, 0);
- else
- lr->end = INT_MAX;
pr_debug("Line range is %d to %d\n", lr->start, lr->end);
+
+ err = -EINVAL;
if (lr->start > lr->end) {
semantic_error("Start line must be smaller"
" than end line.\n");
- return -EINVAL;
+ goto err;
}
- if (*tmp != '\0') {
- semantic_error("Tailing with invalid character '%d'.\n",
- *tmp);
- return -EINVAL;
+ if (*range != '\0') {
+ semantic_error("Tailing with invalid str '%s'.\n", range);
+ goto err;
}
- tmp = strndup(arg, (ptr - arg));
- } else {
- tmp = strdup(arg);
- lr->end = INT_MAX;
}

- if (tmp == NULL)
- return -ENOMEM;
-
- if (strchr(tmp, '.'))
- lr->file = tmp;
+ if (strchr(name, '.'))
+ lr->file = name;
else
- lr->function = tmp;
+ lr->function = name;

return 0;
+err:
+ free(name);
+ return err;
}

/* Check the name is good for event/group */

2010-12-22 13:11:45

[permalink] [raw]

Subject: Re: [PATCH 0/6] Minor cleanups & fixes for perf-probe(1)

Arnaldo Carvalho de Melo <[email protected]> writes:

[...]

>
> I did just that s/fprintf/color_fprintf/g change Masami asked and
> stashed everything in my perf/core branch.

Thanks.

Should I still send some details about what patches do ?

--
Franck

2010-12-22 13:14:50

[permalink] [raw]

Subject: Re: [tip:perf/core] perf probe: Don't always consider EOF as an error when listing source code

tip-bot for Franck Bui-Huu <[email protected]> writes:

[...]

> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 3ba9c53..80cc0bc 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -287,7 +287,7 @@ static int get_real_path(const char *raw_path, const char *comp_dir,
> #define LINEBUF_SIZE 256
> #define NR_ADDITIONAL_LINES 2
>
> -static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
> +static int __show_one_line(FILE *fp, int l, bool skip, bool show_num)
> {
> char buf[LINEBUF_SIZE];
> const char *color = show_num ? "" : PERF_COLOR_BLUE;
> @@ -306,16 +306,30 @@ static int show_one_line(FILE *fp, int l, bool skip, bool show_num)
>
> } while (strchr(buf, '\n') == NULL);
>
> - return 0;
> + return 1;
> error:
> - if (feof(fp))
> + if (ferror(fp)) {
> pr_warning("Source file is shorter than expected.\n");
> - else
> - pr_warning("File read error: %s\n", strerror(errno));
> + return -1;

Argh, something wrong here.

The warning left here, is the wrong one, I should have left the other
one.

Sorry for the mistake.

Should I send a patch to fix that ?
--
Franck

2010-12-22 14:43:46

by Arnaldo Carvalho de Melo

[permalink] [raw]

Subject: Re: [tip:perf/core] perf probe: Don't always consider EOF as an error when listing source code

Em Wed, Dec 22, 2010 at 02:14:43PM +0100, Franck Bui-Huu escreveu:
> tip-bot for Franck Bui-Huu <[email protected]> writes:
> > error:
> > - if (feof(fp))
> > + if (ferror(fp)) {
> > pr_warning("Source file is shorter than expected.\n");
> > - else
> > - pr_warning("File read error: %s\n", strerror(errno));
> > + return -1;
>
> Argh, something wrong here.
>
> The warning left here, is the wrong one, I should have left the other
> one.
>
> Sorry for the mistake.
>
> Should I send a patch to fix that ?

Please.

- Arnaldo

2010-12-22 16:37:22

[permalink] [raw]

Subject: Re: [tip:perf/core] perf probe: Don't always consider EOF as an error when listing source code

Arnaldo Carvalho de Melo <[email protected]> writes:

> Em Wed, Dec 22, 2010 at 02:14:43PM +0100, Franck Bui-Huu escreveu:
>> tip-bot for Franck Bui-Huu <[email protected]> writes:
>> > error:
>> > - if (feof(fp))
>
>> > + if (ferror(fp)) {
>> > pr_warning("Source file is shorter than expected.\n");
>> > - else
>> > - pr_warning("File read error: %s\n", strerror(errno));
>> > + return -1;
>>
>> Argh, something wrong here.
>>
>> The warning left here, is the wrong one, I should have left the other
>> one.
>>
>> Sorry for the mistake.
>>
>> Should I send a patch to fix that ?
>
> Please.
>

[PATCH] perf-probe: Fix wrong warning in __show_one_line() if read(1) errors happen

From: Franck Bui-Huu <[email protected]>

This was introduced by commit fde52dbd7f71934aba4e150f3d1d51e826a08850.

Signed-off-by: Franck Bui-Huu <[email protected]>
---
tools/perf/util/probe-event.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 469ad35..10ad1ad 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -319,7 +319,7 @@ static int __show_one_line(FILE *fp, int l, bool skip, bool show_num)
return 1;
error:
if (ferror(fp)) {
- pr_warning("Source file is shorter than expected.\n");
+ pr_warning("File read error: %s\n", strerror(errno));
return -1;
}
return 0;
--
1.7.3.2

--
Franck

2010-12-24 00:36:23

[permalink] [raw]

Subject: Re: [PATCH 0/6] Minor cleanups & fixes for perf-probe(1)

(2010/12/22 22:11), Franck Bui-Huu wrote:
> Arnaldo Carvalho de Melo <[email protected]> writes:
>
> [...]
>
>>
>> I did just that s/fprintf/color_fprintf/g change Masami asked and
>> stashed everything in my perf/core branch.
>
> Thanks.
>
> Should I still send some details about what patches do ?
>

No, it seems that Arnaldo has already fixed it in his tree.

Thank you,

--
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: [email protected]

2010-12-25 08:59:05

[permalink] [raw]

Subject: [tip:perf/core] perf probe: Fix wrong warning in __show_one_line() if read(1) errors happen

Commit-ID: 32b2b6ec57a3adb3ab7215fbf36ec61c15de06ee
Gitweb: http://git.kernel.org/tip/32b2b6ec57a3adb3ab7215fbf36ec61c15de06ee
Author: Franck Bui-Huu <[email protected]>
AuthorDate: Wed, 22 Dec 2010 17:37:13 +0100
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 22 Dec 2010 20:32:08 -0200

perf probe: Fix wrong warning in __show_one_line() if read(1) errors happen

This was introduced by commit fde52dbd7f71934aba4e150f3d1d51e826a08850.

Cc: H. Peter Anvin <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Franck Bui-Huu <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/probe-event.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 099336e..4bde988 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -309,7 +309,7 @@ static int __show_one_line(FILE *fp, int l, bool skip, bool show_num)
return 1;
error:
if (ferror(fp)) {
- pr_warning("Source file is shorter than expected.\n");
+ pr_warning("File read error: %s\n", strerror(errno));
return -1;
}
return 0;

2010-12-26 14:11:26