[PATCH 2.6.32] ftrace: fix event format export

[PATCH 2.6.32] ftrace: fix event format export

[Johannes Berg]

For some reason the export of the event print
format to userspace uses '#fmt' which breaks
if the format string is anything but a plain
string, for example if it is built with macros
then the macro names are exported instead of
their contents.

Use
	"\"%s\"", fmt
instead of
	"%s", #fmt
to export the string and not the way it is built.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
This is making the export of a bunch of events (only checked the
wireless ones but those are all affected) unusable, so please apply to
2.6.32.

 include/trace/ftrace.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- wireless-testing.orig/include/trace/ftrace.h	2009-11-13 13:15:21.000000000 +0100
+++ wireless-testing/include/trace/ftrace.h	2009-11-13 13:34:03.000000000 +0100
@@ -159,7 +159,7 @@
 #undef __get_str
 
 #undef TP_printk
-#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
+#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)
 
 #undef TP_fast_assign
 #define TP_fast_assign(args...) args

Re: [PATCH 2.6.32] ftrace: fix event format export

[Andreas Schwab]

Johannes Berg <johannes@sipsolutions.net> writes:

> --- wireless-testing.orig/include/trace/ftrace.h	2009-11-13 13:15:21.000000000 +0100
> +++ wireless-testing/include/trace/ftrace.h	2009-11-13 13:34:03.000000000 +0100
> @@ -159,7 +159,7 @@
>  #undef __get_str
>  
>  #undef TP_printk
> -#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
> +#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)

Would using __stringify(fmt) work?  If there are double quote characters
in fmt your solution would produce output that is ambiguous.

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: [PATCH 2.6.32] ftrace: fix event format export

[Johannes Berg]

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

On Fri, 2009-11-13 at 14:22 +0100, Andreas Schwab wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
> 
> > --- wireless-testing.orig/include/trace/ftrace.h	2009-11-13 13:15:21.000000000 +0100
> > +++ wireless-testing/include/trace/ftrace.h	2009-11-13 13:34:03.000000000 +0100
> > @@ -159,7 +159,7 @@
> >  #undef __get_str
> >  
> >  #undef TP_printk
> > -#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
> > +#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)
> 
> Would using __stringify(fmt) work?  If there are double quote characters
> in fmt your solution would produce output that is ambiguous.

No, for say

#define FOO_FMT		"foo:%d"

and using
	FOO_FMT ", %d"
that would give
	""%s", %d"
instead of
	"%s, %d"

However, isn't it already ambiguous that way? I fail to see why %s, #fmt
would preserve inside " properly.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2.6.32] ftrace: fix event format export

[Johannes Berg]

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

On Fri, 2009-11-13 at 14:29 +0100, Johannes Berg wrote:

> > > -#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
> > > +#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)
> > 
> > Would using __stringify(fmt) work?  If there are double quote characters
> > in fmt your solution would produce output that is ambiguous.

> However, isn't it already ambiguous that way? I fail to see why %s, #fmt
> would preserve inside " properly.

Never mind, I missed one indirection, so yes, it would preserve that. I
guess I'll have to somehow escape the quotes when copying them out to
userspace.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2.6.32] ftrace: fix event format export

[Ingo Molnar]

( extended the Cc:s. Please try to Cc: people you think would be 
  interested in your patches. )

* Johannes Berg <johannes@sipsolutions.net> wrote:

> For some reason the export of the event print
> format to userspace uses '#fmt' which breaks
> if the format string is anything but a plain
> string, for example if it is built with macros
> then the macro names are exported instead of
> their contents.
> 
> Use
> 	"\"%s\"", fmt
> instead of
> 	"%s", #fmt
> to export the string and not the way it is built.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> This is making the export of a bunch of events (only checked the
> wireless ones but those are all affected) unusable, so please apply to
> 2.6.32.
> 
>  include/trace/ftrace.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- wireless-testing.orig/include/trace/ftrace.h	2009-11-13 13:15:21.000000000 +0100
> +++ wireless-testing/include/trace/ftrace.h	2009-11-13 13:34:03.000000000 +0100
> @@ -159,7 +159,7 @@
>  #undef __get_str
>  
>  #undef TP_printk
> -#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
> +#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)
>  
>  #undef TP_fast_assign
>  #define TP_fast_assign(args...) args
> 
> 
> --
> 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/

Re: [PATCH 2.6.32] ftrace: fix event format export

[Steven Rostedt]

On Fri, 2009-11-13 at 23:40 +0100, Ingo Molnar wrote:
> ( extended the Cc:s. Please try to Cc: people you think would be 
>   interested in your patches. )

Thanks Ingo.

> 
> * Johannes Berg <johannes@sipsolutions.net> wrote:
> 
> > For some reason the export of the event print
> > format to userspace uses '#fmt' which breaks
> > if the format string is anything but a plain
> > string, for example if it is built with macros
> > then the macro names are exported instead of
> > their contents.
> > 
> > Use
> > 	"\"%s\"", fmt
> > instead of
> > 	"%s", #fmt
> > to export the string and not the way it is built.

Is there any examples of this currently in the kernel? If so could you
show that in the changlog.

But other than that, the change looks good.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> > 
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > ---

Re: [PATCH 2.6.32] ftrace: fix event format export

[Johannes Berg]

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

On Fri, 2009-11-13 at 18:28 -0500, Steven Rostedt wrote:

> > > Use
> > > 	"\"%s\"", fmt
> > > instead of
> > > 	"%s", #fmt
> > > to export the string and not the way it is built.
> 
> Is there any examples of this currently in the kernel? If so could you
> show that in the changlog.

net/mac80211/driver-trace.h has some that are exported really badly and
both trace-cmd and perf fall over on them.

Incidentally, the kvm_mmu ones are also weird, but I'm not sure they can
be fixed up properly.

Do you think Andreas's objection to the now-missing escaping of " and \
is a problem, or should we either fix the parsers to parse the string
backward from REC stuff, or fix the routine that outputs them to
userspace to quote " and \?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH][GIT PULL][v2.6.32] tracing: Fix event format export

[Steven Rostedt]

Ingo,

Please pull the latest tip/tracing/urgent tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/tracing/urgent


Johannes Berg (1):
      tracing: Fix event format export

----
 include/trace/ftrace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
---------------------------
commit 811cb50baf63461ce0bdb234927046131fc7fa8b
Author: Johannes Berg <johannes@sipsolutions.net>
Date:   Fri Nov 13 23:40:09 2009 +0100

    tracing: Fix event format export
    
    For some reason the export of the event print format to userspace
    uses '#fmt' which breaks if the format string is anything but a plain
    string, for example if it is built with macros then the macro names
    are exported instead of their contents.
    
    Use
    	"\"%s\"", fmt
    instead of
    	"%s", #fmt
    to export the string and not the way it is built.
    
    For example, in net/mac80211/driver-trace.h for the trace event drv_start
    there is:
    
            TP_printk(
                    LOCAL_PR_FMT, LOCAL_PR_ARG
            )
    
    Which use to produce:
    
     print fmt: LOCAL_PR_FMT, REC->wiphy_name
    
    Now produces:
    
     print fmt: "%s", REC->wiphy_name
    
    Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
    LKML-Reference: <20091113224009.GB23942@elte.hu>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index cc0d966..dacb8ef 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -159,7 +159,7 @@
 #undef __get_str
 
 #undef TP_printk
-#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
+#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)
 
 #undef TP_fast_assign
 #define TP_fast_assign(args...) args

Re: [PATCH][GIT PULL][v2.6.32] tracing: Fix event format export

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Ingo,
> 
> Please pull the latest tip/tracing/urgent tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/tracing/urgent
> 
> 
> Johannes Berg (1):
>       tracing: Fix event format export
> 
> ----
>  include/trace/ftrace.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Pulled, thanks Steve!

	Ingo

[tip:tracing/urgent] tracing: Fix event format export

[tip-bot for Johannes Berg]

Commit-ID:  811cb50baf63461ce0bdb234927046131fc7fa8b
Gitweb:     http://git.kernel.org/tip/811cb50baf63461ce0bdb234927046131fc7fa8b
Author:     Johannes Berg <johannes@sipsolutions.net>
AuthorDate: Fri, 13 Nov 2009 23:40:09 +0100
Committer:  Steven Rostedt <rostedt@goodmis.org>
CommitDate: Fri, 13 Nov 2009 22:20:34 -0500

tracing: Fix event format export

For some reason the export of the event print format to userspace
uses '#fmt' which breaks if the format string is anything but a plain
string, for example if it is built with macros then the macro names
are exported instead of their contents.

Use
	"\"%s\"", fmt
instead of
	"%s", #fmt
to export the string and not the way it is built.

For example, in net/mac80211/driver-trace.h for the trace event drv_start
there is:

        TP_printk(
                LOCAL_PR_FMT, LOCAL_PR_ARG
        )

Which use to produce:

 print fmt: LOCAL_PR_FMT, REC->wiphy_name

Now produces:

 print fmt: "%s", REC->wiphy_name

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
LKML-Reference: <20091113224009.GB23942@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/trace/ftrace.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index cc0d966..dacb8ef 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -159,7 +159,7 @@
 #undef __get_str
 
 #undef TP_printk
-#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
+#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)
 
 #undef TP_fast_assign
 #define TP_fast_assign(args...) args

Re: [tip:tracing/urgent] tracing: Fix event format export

[Steven Rostedt]

On Sat, 2009-11-14 at 10:18 +0000, tip-bot for Johannes Berg wrote:
> Commit-ID:  811cb50baf63461ce0bdb234927046131fc7fa8b
> Gitweb:     http://git.kernel.org/tip/811cb50baf63461ce0bdb234927046131fc7fa8b
> Author:     Johannes Berg <johannes@sipsolutions.net>
> AuthorDate: Fri, 13 Nov 2009 23:40:09 +0100
> Committer:  Steven Rostedt <rostedt@goodmis.org>
> CommitDate: Fri, 13 Nov 2009 22:20:34 -0500
> 
> tracing: Fix event format export
> 
> For some reason the export of the event print format to userspace
> uses '#fmt' which breaks if the format string is anything but a plain
> string, for example if it is built with macros then the macro names
> are exported instead of their contents.
> 
> Use
> 	"\"%s\"", fmt
> instead of
> 	"%s", #fmt
> to export the string and not the way it is built.
> 
> For example, in net/mac80211/driver-trace.h for the trace event drv_start
> there is:
> 
>         TP_printk(
>                 LOCAL_PR_FMT, LOCAL_PR_ARG
>         )
> 
> Which use to produce:
> 
>  print fmt: LOCAL_PR_FMT, REC->wiphy_name
> 
> Now produces:
> 
>  print fmt: "%s", REC->wiphy_name
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> LKML-Reference: <20091113224009.GB23942@elte.hu>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


Ingo,

Any reason that this did not make it into Mainline? 2.6.32 is out, and
Johannes Berg was depending on this for his users.

-- Steve

> ---
>  include/trace/ftrace.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index cc0d966..dacb8ef 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -159,7 +159,7 @@
>  #undef __get_str
>  
>  #undef TP_printk
> -#define TP_printk(fmt, args...) "%s, %s\n", #fmt, __stringify(args)
> +#define TP_printk(fmt, args...) "\"%s\", %s\n", fmt, __stringify(args)
>  
>  #undef TP_fast_assign
>  #define TP_fast_assign(args...) args

Re: [tip:tracing/urgent] tracing: Fix event format export

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 2009-11-14 at 10:18 +0000, tip-bot for Johannes Berg wrote:
> > Commit-ID:  811cb50baf63461ce0bdb234927046131fc7fa8b
> > Gitweb:     http://git.kernel.org/tip/811cb50baf63461ce0bdb234927046131fc7fa8b
> > Author:     Johannes Berg <johannes@sipsolutions.net>
> > AuthorDate: Fri, 13 Nov 2009 23:40:09 +0100
> > Committer:  Steven Rostedt <rostedt@goodmis.org>
> > CommitDate: Fri, 13 Nov 2009 22:20:34 -0500
> > 
> > tracing: Fix event format export
> > 
> > For some reason the export of the event print format to userspace
> > uses '#fmt' which breaks if the format string is anything but a plain
> > string, for example if it is built with macros then the macro names
> > are exported instead of their contents.
> > 
> > Use
> > 	"\"%s\"", fmt
> > instead of
> > 	"%s", #fmt
> > to export the string and not the way it is built.
> > 
> > For example, in net/mac80211/driver-trace.h for the trace event drv_start
> > there is:
> > 
> >         TP_printk(
> >                 LOCAL_PR_FMT, LOCAL_PR_ARG
> >         )
> > 
> > Which use to produce:
> > 
> >  print fmt: LOCAL_PR_FMT, REC->wiphy_name
> > 
> > Now produces:
> > 
> >  print fmt: "%s", REC->wiphy_name
> > 
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > LKML-Reference: <20091113224009.GB23942@elte.hu>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> 
> Ingo,
> 
> Any reason that this did not make it into Mainline? 2.6.32 is out, and 
> Johannes Berg was depending on this for his users.

Yeah, it narrowly missed the last -rc (-rc8), and we only push 
post-final-rc fixes for serious show-stopper regressions.

v2.6.32.1 will have this (and other) fixes. Johannes can cherry-pick the 
fix in a day or so once it hits Linus's tree in the merge window.

Thanks,

	Ingo

Re: [tip:tracing/urgent] tracing: Fix event format export

[Johannes Berg]

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

On Thu, 2009-12-03 at 15:55 +0100, Ingo Molnar wrote:

> > Any reason that this did not make it into Mainline? 2.6.32 is out, and 
> > Johannes Berg was depending on this for his users.
> 
> Yeah, it narrowly missed the last -rc (-rc8), and we only push 
> post-final-rc fixes for serious show-stopper regressions.

You have an interesting definition of "narrowly missed":

$ git show v2.6.32-rc8
...
Date:   Thu Nov 19 14:32:50 2009 -0800
...
$ git show --pretty=fuller 811cb50baf63461ce0bdb234927046131fc7fa8b
...
CommitDate: Fri Nov 13 22:20:34 2009 -0500

And Steven's pull request for this very simple fix was from the 14th.
That's 5 days until -rc8.

So maybe I have a personality problem, but suck it up -- you're wrong
this time.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [tip:tracing/urgent] tracing: Fix event format export

[Ingo Molnar]

* Johannes Berg <johannes@sipsolutions.net> wrote:

> On Thu, 2009-12-03 at 15:55 +0100, Ingo Molnar wrote:
> 
> > > Any reason that this did not make it into Mainline? 2.6.32 is out, and 
> > > Johannes Berg was depending on this for his users.
> > 
> > Yeah, it narrowly missed the last -rc (-rc8), and we only push 
> > post-final-rc fixes for serious show-stopper regressions.
> 
> You have an interesting definition of "narrowly missed":
> 
> $ git show v2.6.32-rc8
> ...
> Date:   Thu Nov 19 14:32:50 2009 -0800
> ...
> $ git show --pretty=fuller 811cb50baf63461ce0bdb234927046131fc7fa8b
> ...
> CommitDate: Fri Nov 13 22:20:34 2009 -0500

Yes, it came in late, after -rc7 and just two weeks before the final 
2.6.32 kernel was released. Please test linux-next or -tip more 
frequently if you want fixes to go upstream sooner.

> And Steven's pull request for this very simple fix was from the 14th. 
> That's 5 days until -rc8.

We are extra careful in late -rc's - and per Linus's request we are only 
pushing fixes for serious regressions, and even those we only push after 
careful testing. That's a few days commit life-time at minimum in the 
usual case.

> So maybe I have a personality problem, but suck it up -- you're wrong 
> this time.

No need to complain - the fix is upstream now and will be in 2.6.32.1 - 
within 1-2 weeks. If that's not fast enough for you then you can 
cherry-pick upstream commit 811cb50baf63461ce0bdb234927046131fc7fa8b 
into your tree right now.

Thanks,

	Ingo