Re: [PATCH] perf hists: Fix period symbol_conf.field_sep display, again
Em Thu, Nov 29, 2012 at 02:40:02PM +0100, Jiri Olsa escreveu:
> On Thu, Nov 29, 2012 at 12:53:44PM +0100, Jiri Olsa wrote:
> > On Thu, Nov 29, 2012 at 04:56:04PM +0900, Namhyung Kim wrote:
>
> SNIP
>
> > > > - if (!sep || !first) {
> > > > - ret = scnprintf(hpp->buf, hpp->size, "%s", sep
> > > > ?: " ");
> > > > - advance_hpp(hpp, ret);
> > > > - first = false;
> > > > - }
> > > > + ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: " ");
> > > > + advance_hpp(hpp, ret);
> > >
> > > It will display the separator even before the first column so that the
> > > output can be messed up with this. I can see that there's a bug setting
> > > 'first' to false - the line should be moved out of the block otherwise
> > > it's pointless since we cannot enter the block.
> >
> > hum, I'll retest
>
> you're right, fix is attached
>
> Arnaldo, I know you've already pulled this one.. I can resend v2
> if needed, which is shorter (just that 'first = false' move as
> Namhyyung said)
I just folded it, was the tip of the tree, will go on processing the
others now.
> thanks,
> jirka
>
>
> ---
> Last fix of this place was wrong and caused the separator
> field to be displayed even before period column.
>
> Should be fixed properly this time by introducing 'first' bool
> like on other places doing the same stuff.
>
> Screwed-up-by: Jiri Olsa
> Reported-by: Namhyung Kim
> Cc: Arnaldo Carvalho de Melo
> Cc: Peter Zijlstra
> Cc: Ingo Molnar
> Cc: Paul Mackerras
> Cc: Corey Ashford
> Cc: Frederic Weisbecker
> Signed-off-by: Jiri Olsa
> ---
> tools/perf/ui/hist.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 2447e0c..6e639b5 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -466,13 +466,21 @@ int hist_entry__period_snprintf(struct perf_hpp *hpp,
> struct hist_entry *he,
> struct perf_hpp_fmt *fmt;
> char *start = hpp->buf;
> int ret;
> + bool first = true;
>
> if (symbol_conf.exclude_other && !he->parent)
> return 0;
>
> perf_hpp__for_each_format(fmt) {
> - ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: " ");
> - advance_hpp(hpp, ret);
> + /*
> + * If there's no field_sep, we still need
> + * to display initial ' '.
> + */
> + if (!sep || !first) {
> + ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: " ");
> + advance_hpp(hpp, ret);
> + } else
> + first = false;
>
> if (color && fmt->color)
> ret = fmt->color(hpp, he);
> --
> 1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] perf hists: Fix period symbol_conf.field_sep display, again
Em Thu, Nov 29, 2012 at 02:40:02PM +0100, Jiri Olsa escreveu:
On Thu, Nov 29, 2012 at 12:53:44PM +0100, Jiri Olsa wrote:
On Thu, Nov 29, 2012 at 04:56:04PM +0900, Namhyung Kim wrote:
SNIP
- if (!sep || !first) {
- ret = scnprintf(hpp-buf, hpp-size, %s, sep
?: );
- advance_hpp(hpp, ret);
- first = false;
- }
+ ret = scnprintf(hpp-buf, hpp-size, %s, sep ?: );
+ advance_hpp(hpp, ret);
It will display the separator even before the first column so that the
output can be messed up with this. I can see that there's a bug setting
'first' to false - the line should be moved out of the block otherwise
it's pointless since we cannot enter the block.
hum, I'll retest
you're right, fix is attached
Arnaldo, I know you've already pulled this one.. I can resend v2
if needed, which is shorter (just that 'first = false' move as
Namhyyung said)
I just folded it, was the tip of the tree, will go on processing the
others now.
thanks,
jirka
---
Last fix of this place was wrong and caused the separator
field to be displayed even before period column.
Should be fixed properly this time by introducing 'first' bool
like on other places doing the same stuff.
Screwed-up-by: Jiri Olsa jo...@redhat.com
Reported-by: Namhyung Kim namhy...@kernel.org
Cc: Arnaldo Carvalho de Melo a...@ghostprotocols.net
Cc: Peter Zijlstra a.p.zijls...@chello.nl
Cc: Ingo Molnar mi...@elte.hu
Cc: Paul Mackerras pau...@samba.org
Cc: Corey Ashford cjash...@linux.vnet.ibm.com
Cc: Frederic Weisbecker fweis...@gmail.com
Signed-off-by: Jiri Olsa jo...@redhat.com
---
tools/perf/ui/hist.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 2447e0c..6e639b5 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -466,13 +466,21 @@ int hist_entry__period_snprintf(struct perf_hpp *hpp,
struct hist_entry *he,
struct perf_hpp_fmt *fmt;
char *start = hpp-buf;
int ret;
+ bool first = true;
if (symbol_conf.exclude_other !he-parent)
return 0;
perf_hpp__for_each_format(fmt) {
- ret = scnprintf(hpp-buf, hpp-size, %s, sep ?: );
- advance_hpp(hpp, ret);
+ /*
+ * If there's no field_sep, we still need
+ * to display initial ' '.
+ */
+ if (!sep || !first) {
+ ret = scnprintf(hpp-buf, hpp-size, %s, sep ?: );
+ advance_hpp(hpp, ret);
+ } else
+ first = false;
if (color fmt-color)
ret = fmt-color(hpp, he);
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf hists: Fix period symbol_conf.field_sep display, again
On Thu, Nov 29, 2012 at 12:53:44PM +0100, Jiri Olsa wrote:
> On Thu, Nov 29, 2012 at 04:56:04PM +0900, Namhyung Kim wrote:
SNIP
> > > - if (!sep || !first) {
> > > - ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: " ");
> > > - advance_hpp(hpp, ret);
> > > - first = false;
> > > - }
> > > + ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: " ");
> > > + advance_hpp(hpp, ret);
> >
> > It will display the separator even before the first column so that the
> > output can be messed up with this. I can see that there's a bug setting
> > 'first' to false - the line should be moved out of the block otherwise
> > it's pointless since we cannot enter the block.
>
> hum, I'll retest
you're right, fix is attached
Arnaldo, I know you've already pulled this one.. I can resend v2
if needed, which is shorter (just that 'first = false' move as
Namhyyung said)
thanks,
jirka
---
Last fix of this place was wrong and caused the separator
field to be displayed even before period column.
Should be fixed properly this time by introducing 'first' bool
like on other places doing the same stuff.
Screwed-up-by: Jiri Olsa
Reported-by: Namhyung Kim
Cc: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra
Cc: Ingo Molnar
Cc: Paul Mackerras
Cc: Corey Ashford
Cc: Frederic Weisbecker
Signed-off-by: Jiri Olsa
---
tools/perf/ui/hist.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 2447e0c..6e639b5 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -466,13 +466,21 @@ int hist_entry__period_snprintf(struct perf_hpp *hpp,
struct hist_entry *he,
struct perf_hpp_fmt *fmt;
char *start = hpp->buf;
int ret;
+ bool first = true;
if (symbol_conf.exclude_other && !he->parent)
return 0;
perf_hpp__for_each_format(fmt) {
- ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: " ");
- advance_hpp(hpp, ret);
+ /*
+* If there's no field_sep, we still need
+* to display initial ' '.
+*/
+ if (!sep || !first) {
+ ret = scnprintf(hpp->buf, hpp->size, "%s", sep ?: " ");
+ advance_hpp(hpp, ret);
+ } else
+ first = false;
if (color && fmt->color)
ret = fmt->color(hpp, he);
--
1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] perf hists: Fix period symbol_conf.field_sep display, again
On Thu, Nov 29, 2012 at 12:53:44PM +0100, Jiri Olsa wrote:
On Thu, Nov 29, 2012 at 04:56:04PM +0900, Namhyung Kim wrote:
SNIP
- if (!sep || !first) {
- ret = scnprintf(hpp-buf, hpp-size, %s, sep ?: );
- advance_hpp(hpp, ret);
- first = false;
- }
+ ret = scnprintf(hpp-buf, hpp-size, %s, sep ?: );
+ advance_hpp(hpp, ret);
It will display the separator even before the first column so that the
output can be messed up with this. I can see that there's a bug setting
'first' to false - the line should be moved out of the block otherwise
it's pointless since we cannot enter the block.
hum, I'll retest
you're right, fix is attached
Arnaldo, I know you've already pulled this one.. I can resend v2
if needed, which is shorter (just that 'first = false' move as
Namhyyung said)
thanks,
jirka
---
Last fix of this place was wrong and caused the separator
field to be displayed even before period column.
Should be fixed properly this time by introducing 'first' bool
like on other places doing the same stuff.
Screwed-up-by: Jiri Olsa jo...@redhat.com
Reported-by: Namhyung Kim namhy...@kernel.org
Cc: Arnaldo Carvalho de Melo a...@ghostprotocols.net
Cc: Peter Zijlstra a.p.zijls...@chello.nl
Cc: Ingo Molnar mi...@elte.hu
Cc: Paul Mackerras pau...@samba.org
Cc: Corey Ashford cjash...@linux.vnet.ibm.com
Cc: Frederic Weisbecker fweis...@gmail.com
Signed-off-by: Jiri Olsa jo...@redhat.com
---
tools/perf/ui/hist.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 2447e0c..6e639b5 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -466,13 +466,21 @@ int hist_entry__period_snprintf(struct perf_hpp *hpp,
struct hist_entry *he,
struct perf_hpp_fmt *fmt;
char *start = hpp-buf;
int ret;
+ bool first = true;
if (symbol_conf.exclude_other !he-parent)
return 0;
perf_hpp__for_each_format(fmt) {
- ret = scnprintf(hpp-buf, hpp-size, %s, sep ?: );
- advance_hpp(hpp, ret);
+ /*
+* If there's no field_sep, we still need
+* to display initial ' '.
+*/
+ if (!sep || !first) {
+ ret = scnprintf(hpp-buf, hpp-size, %s, sep ?: );
+ advance_hpp(hpp, ret);
+ } else
+ first = false;
if (color fmt-color)
ret = fmt-color(hpp, he);
--
1.7.11.7
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
