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

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

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

Hi,

This small patchset is just some random cosmetic cleanups I did when
starting to read perf-probe(1) stuffs.

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(-)

-- 
1.7.3.2

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

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

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 <fbuihuu@gmail.com>
---
 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

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

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 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

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

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

It also removes some superflous parentheses.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 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

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

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

	$ perf-probe -L sched.c

is currently allowed but not documented.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 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

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

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

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 <fbuihuu@gmail.com>
---
 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

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

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

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 <fbuihuu@gmail.com>
---
 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

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

[Masami Hiramatsu]

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> 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 <masami.hiramatsu.pt@hitachi.com>

> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  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: masami.hiramatsu.pt@hitachi.com

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

[Masami Hiramatsu]

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>

Please explain what this patch will do...

> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  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: masami.hiramatsu.pt@hitachi.com

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

[Masami Hiramatsu]

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> It also removes some superflous parentheses.

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


> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  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: masami.hiramatsu.pt@hitachi.com

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

[Masami Hiramatsu]

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> 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 <masami.hiramatsu.pt@hitachi.com>

> 
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  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: masami.hiramatsu.pt@hitachi.com

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

[Masami Hiramatsu]

(2010/12/20 23:18), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> 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 <fbuihuu@gmail.com>

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

> ---
>  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: masami.hiramatsu.pt@hitachi.com

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

[Masami Hiramatsu]

(2010/12/20 23:17), Franck Bui-Huu wrote:
> From: Franck Bui-Huu <fbuihuu@gmail.com>
> 
> 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: masami.hiramatsu.pt@hitachi.com

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

[Franck Bui-Huu]

Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> 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

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

[Arnaldo Carvalho de Melo]

Em Tue, Dec 21, 2010 at 07:01:06PM +0100, Franck Bui-Huu escreveu:
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> 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

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

[tip-bot for Franck Bui-Huu]

Commit-ID:  62c15fc49bd1b35d79b34ea96f132ab435e2215a
Gitweb:     http://git.kernel.org/tip/62c15fc49bd1b35d79b34ea96f132ab435e2215a
Author:     Franck Bui-Huu <fbuihuu@gmail.com>
AuthorDate: Mon, 20 Dec 2010 15:18:00 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <masami.hiramatsu.pt@hitachi.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
LKML-Reference: <1292854685-8230-2-git-send-email-fbuihuu@gmail.com>
Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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) {

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

[tip-bot for Franck Bui-Huu]

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

perf probe: Rewrite show_one_line() to make it simpler

Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
LKML-Reference: <1292854685-8230-3-git-send-email-fbuihuu@gmail.com>
Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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:

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

[tip-bot for Franck Bui-Huu]

Commit-ID:  44b81e929b0c00e703a31a3d634b668bb27eb1c8
Gitweb:     http://git.kernel.org/tip/44b81e929b0c00e703a31a3d634b668bb27eb1c8
Author:     Franck Bui-Huu <fbuihuu@gmail.com>
AuthorDate: Mon, 20 Dec 2010 15:18:02 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <masami.hiramatsu.pt@hitachi.com>
LKML-Reference: <1292854685-8230-4-git-send-email-fbuihuu@gmail.com>
Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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;

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

[tip-bot for Franck Bui-Huu]

Commit-ID:  9d95b580a8d64ef4d1660a21a9de0658fe29f041
Gitweb:     http://git.kernel.org/tip/9d95b580a8d64ef4d1660a21a9de0658fe29f041
Author:     Franck Bui-Huu <fbuihuu@gmail.com>
AuthorDate: Mon, 20 Dec 2010 15:18:03 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <masami.hiramatsu.pt@hitachi.com>
LKML-Reference: <1292854685-8230-5-git-send-email-fbuihuu@gmail.com>
Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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);

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

[tip-bot for Franck Bui-Huu]

Commit-ID:  fde52dbd7f71934aba4e150f3d1d51e826a08850
Gitweb:     http://git.kernel.org/tip/fde52dbd7f71934aba4e150f3d1d51e826a08850
Author:     Franck Bui-Huu <fbuihuu@gmail.com>
AuthorDate: Mon, 20 Dec 2010 15:18:04 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <masami.hiramatsu.pt@hitachi.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
LKML-Reference: <1292854685-8230-6-git-send-email-fbuihuu@gmail.com>
Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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:

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

[tip-bot for Franck Bui-Huu]

Commit-ID:  21dd9ae5a4e9f717f3957ec934dd3158129436b8
Gitweb:     http://git.kernel.org/tip/21dd9ae5a4e9f717f3957ec934dd3158129436b8
Author:     Franck Bui-Huu <fbuihuu@gmail.com>
AuthorDate: Mon, 20 Dec 2010 15:18:05 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <masami.hiramatsu.pt@hitachi.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
LKML-Reference: <1292854685-8230-7-git-send-email-fbuihuu@gmail.com>
Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 */

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

[Franck Bui-Huu]

Arnaldo Carvalho de Melo <acme@ghostprotocols.net> 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

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

[Franck Bui-Huu]

tip-bot for Franck Bui-Huu <fbuihuu@gmail.com> 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

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

[Arnaldo Carvalho de Melo]

Em Wed, Dec 22, 2010 at 02:14:43PM +0100, Franck Bui-Huu escreveu:
> tip-bot for Franck Bui-Huu <fbuihuu@gmail.com> 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

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

[Franck Bui-Huu]

Arnaldo Carvalho de Melo <acme@infradead.org> writes:

> Em Wed, Dec 22, 2010 at 02:14:43PM +0100, Franck Bui-Huu escreveu:
>> tip-bot for Franck Bui-Huu <fbuihuu@gmail.com> 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 <fbuihuu@gmail.com>

This was introduced by commit fde52dbd7f71934aba4e150f3d1d51e826a08850.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 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

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

[Masami Hiramatsu]

(2010/12/22 22:11), Franck Bui-Huu wrote:
> Arnaldo Carvalho de Melo <acme@ghostprotocols.net> 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: masami.hiramatsu.pt@hitachi.com

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

[tip-bot for Franck Bui-Huu]

Commit-ID:  32b2b6ec57a3adb3ab7215fbf36ec61c15de06ee
Gitweb:     http://git.kernel.org/tip/32b2b6ec57a3adb3ab7215fbf36ec61c15de06ee
Author:     Franck Bui-Huu <vagabon.xyz@gmail.com>
AuthorDate: Wed, 22 Dec 2010 17:37:13 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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 <hpa@zytor.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <m3y67hsr0m.fsf@gmail.com>
Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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;

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

[Masami Hiramatsu]

(2010/12/25 17:58), tip-bot for Franck Bui-Huu wrote:
> Commit-ID:  32b2b6ec57a3adb3ab7215fbf36ec61c15de06ee
> Gitweb:     http://git.kernel.org/tip/32b2b6ec57a3adb3ab7215fbf36ec61c15de06ee
> Author:     Franck Bui-Huu <vagabon.xyz@gmail.com>
> AuthorDate: Wed, 22 Dec 2010 17:37:13 +0100
> Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
> 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.
> 

It's a good fix for me.

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> LKML-Reference: <m3y67hsr0m.fsf@gmail.com>
> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  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;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


-- 
Masami HIRAMATSU
2nd Dept. Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com