[PATCH] perf trace: Handle legacy syscalls

[PATCH] perf trace: Handle legacy syscalls

[David Ahern]

Currently the code skips the first field with the expectation that it is
'nr'. But older kernels do not have the 'nr' field:

    field:int nr;   offset:8;   size:4; signed:1;

Change perf-trace to drop the field if it exists after parsing the format
file.

This fixes the off-by-one problem with older kernels (e.g., RHEL6). e.g,
perf-trace shows this for write:

    1.515 ( 0.006 ms): dd/4245 write(buf: 2</dev/pts/0>, count: 140733837536224                      ) = 26

where 2 is really the fd, the huge number is really the buf address, etc.
With this patch you get the more appropriate:

    1.813 ( 0.003 ms): dd/6330 write(fd: 2</dev/pts/0>, buf: 0x7fff22fc81f0, count: 25               ) = 25

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 tools/lib/traceevent/event-parse.c | 11 ++++++++---
 tools/lib/traceevent/event-parse.h |  1 +
 tools/perf/builtin-trace.c         | 16 +++++++++++++---
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index afe20ed9fac8..e8a29e730dfb 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -6228,15 +6228,20 @@ void pevent_ref(struct pevent *pevent)
 	pevent->ref_count++;
 }
 
+void free_format_field(struct format_field *field)
+{
+	free(field->type);
+	free(field->name);
+	free(field);
+}
+
 static void free_format_fields(struct format_field *field)
 {
 	struct format_field *next;
 
 	while (field) {
 		next = field->next;
-		free(field->type);
-		free(field->name);
-		free(field);
+		free_format_field(field);
 		field = next;
 	}
 }
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 5b4efc062320..a548fac646f6 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -619,6 +619,7 @@ enum pevent_errno pevent_parse_format(struct pevent *pevent,
 				      const char *buf,
 				      unsigned long size, const char *sys);
 void pevent_free_format(struct event_format *event);
+void free_format_field(struct format_field *field);
 
 void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
 			   const char *name, struct pevent_record *record,
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index dcd950ef2fd7..5caeefeda48a 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1442,14 +1442,24 @@ static int syscall__set_arg_fmts(struct syscall *sc)
 	struct format_field *field;
 	int idx = 0;
 
-	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields - 1, sizeof(void *));
+	field = sc->tp_format->format.fields;
+	/* drop nr field - not relevant here; does not exist on older kernels */
+	if (field && strcmp(field->name, "nr") == 0) {
+		struct format_field *next = field->next;
+
+		free_format_field(field);
+		sc->tp_format->format.fields = next;
+		sc->tp_format->format.nr_fields--;
+	}
+
+	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields, sizeof(void *));
 	if (sc->arg_scnprintf == NULL)
 		return -1;
 
 	if (sc->fmt)
 		sc->arg_parm = sc->fmt->arg_parm;
 
-	for (field = sc->tp_format->format.fields->next; field; field = field->next) {
+	for (field = sc->tp_format->format.fields; field; field = field->next) {
 		if (sc->fmt && sc->fmt->arg_scnprintf[idx])
 			sc->arg_scnprintf[idx] = sc->fmt->arg_scnprintf[idx];
 		else if (field->flags & FIELD_IS_POINTER)
@@ -1547,7 +1557,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
 			.thread = thread,
 		};
 
-		for (field = sc->tp_format->format.fields->next; field;
+		for (field = sc->tp_format->format.fields; field;
 		     field = field->next, ++arg.idx, bit <<= 1) {
 			if (arg.mask & bit)
 				continue;
-- 
2.2.1

Re: [PATCH] perf trace: Handle legacy syscalls

[Arnaldo Carvalho de Melo]

Em Thu, Mar 19, 2015 at 12:36:21PM -0600, David Ahern escreveu:
> Currently the code skips the first field with the expectation that it is
> 'nr'. But older kernels do not have the 'nr' field:
> 
>     field:int nr;   offset:8;   size:4; signed:1;
> 
> Change perf-trace to drop the field if it exists after parsing the format
> file.
> 
> This fixes the off-by-one problem with older kernels (e.g., RHEL6). e.g,
> perf-trace shows this for write:
> 
>     1.515 ( 0.006 ms): dd/4245 write(buf: 2</dev/pts/0>, count: 140733837536224                      ) = 26
> 
> where 2 is really the fd, the huge number is really the buf address, etc.
> With this patch you get the more appropriate:
> 
>     1.813 ( 0.003 ms): dd/6330 write(fd: 2</dev/pts/0>, buf: 0x7fff22fc81f0, count: 25               ) = 25
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  tools/lib/traceevent/event-parse.c | 11 ++++++++---
>  tools/lib/traceevent/event-parse.h |  1 +
>  tools/perf/builtin-trace.c         | 16 +++++++++++++---
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> index afe20ed9fac8..e8a29e730dfb 100644
> --- a/tools/lib/traceevent/event-parse.c
> +++ b/tools/lib/traceevent/event-parse.c
> @@ -6228,15 +6228,20 @@ void pevent_ref(struct pevent *pevent)
>  	pevent->ref_count++;
>  }
>  
> +void free_format_field(struct format_field *field)
> +{
> +	free(field->type);
> +	free(field->name);
> +	free(field);
> +}
> +
>  static void free_format_fields(struct format_field *field)
>  {
>  	struct format_field *next;
>  
>  	while (field) {
>  		next = field->next;
> -		free(field->type);
> -		free(field->name);
> -		free(field);
> +		free_format_field(field);
>  		field = next;
>  	}
>  }
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index 5b4efc062320..a548fac646f6 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -619,6 +619,7 @@ enum pevent_errno pevent_parse_format(struct pevent *pevent,
>  				      const char *buf,
>  				      unsigned long size, const char *sys);
>  void pevent_free_format(struct event_format *event);
> +void free_format_field(struct format_field *field);
>  
>  void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
>  			   const char *name, struct pevent_record *record,
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index dcd950ef2fd7..5caeefeda48a 100644


Up to here we should have a separate patch, one that I would like to
have an Acked-by: Rostedt, ok?

But then I don't think we need to do that, i.e. we can just have a
boolean we set at some point to tell that we need to skip the first
entry.

I'll try to cook up a patch for that.

- Arnaldo

> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1442,14 +1442,24 @@ static int syscall__set_arg_fmts(struct syscall *sc)
>  	struct format_field *field;
>  	int idx = 0;
>  
> -	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields - 1, sizeof(void *));
> +	field = sc->tp_format->format.fields;
> +	/* drop nr field - not relevant here; does not exist on older kernels */
> +	if (field && strcmp(field->name, "nr") == 0) {
> +		struct format_field *next = field->next;
> +
> +		free_format_field(field);
> +		sc->tp_format->format.fields = next;
> +		sc->tp_format->format.nr_fields--;
> +	}
> +
> +	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields, sizeof(void *));
>  	if (sc->arg_scnprintf == NULL)
>  		return -1;
>  
>  	if (sc->fmt)
>  		sc->arg_parm = sc->fmt->arg_parm;
>  
> -	for (field = sc->tp_format->format.fields->next; field; field = field->next) {
> +	for (field = sc->tp_format->format.fields; field; field = field->next) {
>  		if (sc->fmt && sc->fmt->arg_scnprintf[idx])
>  			sc->arg_scnprintf[idx] = sc->fmt->arg_scnprintf[idx];
>  		else if (field->flags & FIELD_IS_POINTER)
> @@ -1547,7 +1557,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
>  			.thread = thread,
>  		};
>  
> -		for (field = sc->tp_format->format.fields->next; field;
> +		for (field = sc->tp_format->format.fields; field;
>  		     field = field->next, ++arg.idx, bit <<= 1) {
>  			if (arg.mask & bit)
>  				continue;
> -- 
> 2.2.1

Re: [PATCH] perf trace: Handle legacy syscalls

[Steven Rostedt]

On Fri, 20 Mar 2015 12:32:26 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Thu, Mar 19, 2015 at 12:36:21PM -0600, David Ahern escreveu:
> > Currently the code skips the first field with the expectation that it is
> > 'nr'. But older kernels do not have the 'nr' field:
> > 
> >     field:int nr;   offset:8;   size:4; signed:1;
> > 
> > Change perf-trace to drop the field if it exists after parsing the format
> > file.
> > 
> > This fixes the off-by-one problem with older kernels (e.g., RHEL6). e.g,
> > perf-trace shows this for write:
> > 
> >     1.515 ( 0.006 ms): dd/4245 write(buf: 2</dev/pts/0>, count: 140733837536224                      ) = 26
> > 
> > where 2 is really the fd, the huge number is really the buf address, etc.
> > With this patch you get the more appropriate:
> > 
> >     1.813 ( 0.003 ms): dd/6330 write(fd: 2</dev/pts/0>, buf: 0x7fff22fc81f0, count: 25               ) = 25
> > 
> > Signed-off-by: David Ahern <dsahern@gmail.com>
> > ---
> >  tools/lib/traceevent/event-parse.c | 11 ++++++++---
> >  tools/lib/traceevent/event-parse.h |  1 +
> >  tools/perf/builtin-trace.c         | 16 +++++++++++++---
> >  3 files changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
> > index afe20ed9fac8..e8a29e730dfb 100644
> > --- a/tools/lib/traceevent/event-parse.c
> > +++ b/tools/lib/traceevent/event-parse.c
> > @@ -6228,15 +6228,20 @@ void pevent_ref(struct pevent *pevent)
> >  	pevent->ref_count++;
> >  }
> >  
> > +void free_format_field(struct format_field *field)
> > +{
> > +	free(field->type);
> > +	free(field->name);
> > +	free(field);
> > +}
> > +
> >  static void free_format_fields(struct format_field *field)
> >  {
> >  	struct format_field *next;
> >  
> >  	while (field) {
> >  		next = field->next;
> > -		free(field->type);
> > -		free(field->name);
> > -		free(field);
> > +		free_format_field(field);
> >  		field = next;
> >  	}
> >  }
> > diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> > index 5b4efc062320..a548fac646f6 100644
> > --- a/tools/lib/traceevent/event-parse.h
> > +++ b/tools/lib/traceevent/event-parse.h
> > @@ -619,6 +619,7 @@ enum pevent_errno pevent_parse_format(struct pevent *pevent,
> >  				      const char *buf,
> >  				      unsigned long size, const char *sys);
> >  void pevent_free_format(struct event_format *event);
> > +void free_format_field(struct format_field *field);

Can we make it "pevent_free_format_field" to keep consistency.

> >  
> >  void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
> >  			   const char *name, struct pevent_record *record,
> > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > index dcd950ef2fd7..5caeefeda48a 100644
> 
> 
> Up to here we should have a separate patch, one that I would like to
> have an Acked-by: Rostedt, ok?

I'm fine with the change if the name of the function is updated.

-- Steve

> 
> But then I don't think we need to do that, i.e. we can just have a
> boolean we set at some point to tell that we need to skip the first
> entry.
> 
> I'll try to cook up a patch for that.

Re: [PATCH] perf trace: Handle legacy syscalls

[Arnaldo Carvalho de Melo]

Em Fri, Mar 20, 2015 at 11:40:11AM -0400, Steven Rostedt escreveu:
> On Fri, 20 Mar 2015 12:32:26 -0300
> Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > Em Thu, Mar 19, 2015 at 12:36:21PM -0600, David Ahern escreveu:
> > > +++ b/tools/lib/traceevent/event-parse.c
> > > @@ -6228,15 +6228,20 @@ void pevent_ref(struct pevent *pevent)
> > >  	pevent->ref_count++;
> > >  }
> > >  
> > > +void free_format_field(struct format_field *field)
> > > +{
> > > +	free(field->type);
> > > +	free(field->name);
> > > +	free(field);
> > > +}
> > > +
> > >  static void free_format_fields(struct format_field *field)
> > >  {
> > >  	struct format_field *next;
> > >  
> > >  	while (field) {
> > >  		next = field->next;
> > > -		free(field->type);
> > > -		free(field->name);
> > > -		free(field);
> > > +		free_format_field(field);
> > >  		field = next;
> > >  	}
> > >  }
> > > diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> > > index 5b4efc062320..a548fac646f6 100644
> > > --- a/tools/lib/traceevent/event-parse.h
> > > +++ b/tools/lib/traceevent/event-parse.h
> > > @@ -619,6 +619,7 @@ enum pevent_errno pevent_parse_format(struct pevent *pevent,
> > >  				      const char *buf,
> > >  				      unsigned long size, const char *sys);
> > >  void pevent_free_format(struct event_format *event);
> > > +void free_format_field(struct format_field *field);
> 
> Can we make it "pevent_free_format_field" to keep consistency.
> 
> > >  
> > >  void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
> > >  			   const char *name, struct pevent_record *record,
> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > > index dcd950ef2fd7..5caeefeda48a 100644
> > 
> > 
> > Up to here we should have a separate patch, one that I would like to
> > have an Acked-by: Rostedt, ok?
> 
> I'm fine with the change if the name of the function is updated.

Ok, so I'll put it in a separate patch, authored by David, with your
Acked-by, it may be useful at some point besides being used in the
libtraceevent internal free_format_fields() static function.

- Arnaldo

Re: [PATCH] perf trace: Handle legacy syscalls

[David Ahern]

On 3/20/15 9:32 AM, Arnaldo Carvalho de Melo wrote:
> But then I don't think we need to do that, i.e. we can just have a
> boolean we set at some point to tell that we need to skip the first
> entry.
>
> I'll try to cook up a patch for that.

why have a boolean that is checked every time through the loop when its 
value will always be the same for a give run? why not just remove the 
entry as I suggested?

Re: [PATCH] perf trace: Handle legacy syscalls

[David Ahern]

On 3/20/15 9:44 AM, Arnaldo Carvalho de Melo wrote:
> Ok, so I'll put it in a separate patch, authored by David, with your
> Acked-by, it may be useful at some point besides being used in the
> libtraceevent internal free_format_fields() static function.

Works for me. Thanks, Arnaldo.

Re: [PATCH] perf trace: Handle legacy syscalls

[Arnaldo Carvalho de Melo]

Em Fri, Mar 20, 2015 at 09:48:10AM -0600, David Ahern escreveu:
> On 3/20/15 9:32 AM, Arnaldo Carvalho de Melo wrote:
> >But then I don't think we need to do that, i.e. we can just have a
> >boolean we set at some point to tell that we need to skip the first
> >entry.
> >
> >I'll try to cook up a patch for that.
> 
> why have a boolean that is checked every time through the loop when its
> value will always be the same for a give run? why not just remove the entry
> as I suggested?

First gut reaction was: hey, we got that from a library, libtraceevent,
that could have other users, etc, better not to change it behind its
back.

I.e. something like this, based on your approach, but not modifying the
data structures received from libtraceevent:

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 6af6bcec930e..001c6ae9a1b1 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1135,6 +1135,8 @@ static struct syscall_fmt *syscall_fmt__find(const char *name)
 
 struct syscall {
 	struct event_format *tp_format;
+	int		    nr_args;
+	struct format_field *args;
 	const char	    *name;
 	bool		    filtered;
 	bool		    is_exit;
@@ -1442,14 +1444,14 @@ static int syscall__set_arg_fmts(struct syscall *sc)
 	struct format_field *field;
 	int idx = 0;
 
-	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields - 1, sizeof(void *));
+	sc->arg_scnprintf = calloc(sc->nr_args, sizeof(void *));
 	if (sc->arg_scnprintf == NULL)
 		return -1;
 
 	if (sc->fmt)
 		sc->arg_parm = sc->fmt->arg_parm;
 
-	for (field = sc->tp_format->format.fields->next; field; field = field->next) {
+	for (field = sc->args; field; field = field->next) {
 		if (sc->fmt && sc->fmt->arg_scnprintf[idx])
 			sc->arg_scnprintf[idx] = sc->fmt->arg_scnprintf[idx];
 		else if (field->flags & FIELD_IS_POINTER)
@@ -1515,6 +1517,14 @@ static int trace__read_syscall_info(struct trace *trace, int id)
 	if (sc->tp_format == NULL)
 		return -1;
 
+	sc->args = sc->tp_format->format.fields;
+	sc->nr_args = sc->tp_format->format.nr_fields;
+	/* drop nr field - not relevant here; does not exist on older kernels */
+	if (sc->args && strcmp(sc->args->name, "nr") == 0) {
+		sc->args = sc->args->next;
+		--sc->nr_args;
+	}
+
 	sc->is_exit = !strcmp(name, "exit_group") || !strcmp(name, "exit");
 
 	return syscall__set_arg_fmts(sc);
@@ -1537,7 +1547,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
 	unsigned char *p;
 	unsigned long val;
 
-	if (sc->tp_format != NULL) {
+	if (sc->args != NULL) {
 		struct format_field *field;
 		u8 bit = 1;
 		struct syscall_arg arg = {
@@ -1547,7 +1557,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
 			.thread = thread,
 		};
 
-		for (field = sc->tp_format->format.fields->next; field;
+		for (field = sc->args; field;
 		     field = field->next, ++arg.idx, bit <<= 1) {
 			if (arg.mask & bit)
 				continue;

Re: [PATCH] perf trace: Handle legacy syscalls

[David Ahern]

On 3/20/15 10:06 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 20, 2015 at 09:48:10AM -0600, David Ahern escreveu:
>> On 3/20/15 9:32 AM, Arnaldo Carvalho de Melo wrote:
>>> But then I don't think we need to do that, i.e. we can just have a
>>> boolean we set at some point to tell that we need to skip the first
>>> entry.
>>>
>>> I'll try to cook up a patch for that.
>>
>> why have a boolean that is checked every time through the loop when its
>> value will always be the same for a give run? why not just remove the entry
>> as I suggested?
>
> First gut reaction was: hey, we got that from a library, libtraceevent,
> that could have other users, etc, better not to change it behind its
> back.
>
> I.e. something like this, based on your approach, but not modifying the
> data structures received from libtraceevent:
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 6af6bcec930e..001c6ae9a1b1 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1135,6 +1135,8 @@ static struct syscall_fmt *syscall_fmt__find(const char *name)
>
>   struct syscall {
>   	struct event_format *tp_format;
> +	int		    nr_args;
> +	struct format_field *args;
>   	const char	    *name;
>   	bool		    filtered;
>   	bool		    is_exit;
> @@ -1442,14 +1444,14 @@ static int syscall__set_arg_fmts(struct syscall *sc)
>   	struct format_field *field;
>   	int idx = 0;
>
> -	sc->arg_scnprintf = calloc(sc->tp_format->format.nr_fields - 1, sizeof(void *));
> +	sc->arg_scnprintf = calloc(sc->nr_args, sizeof(void *));
>   	if (sc->arg_scnprintf == NULL)
>   		return -1;
>
>   	if (sc->fmt)
>   		sc->arg_parm = sc->fmt->arg_parm;
>
> -	for (field = sc->tp_format->format.fields->next; field; field = field->next) {
> +	for (field = sc->args; field; field = field->next) {
>   		if (sc->fmt && sc->fmt->arg_scnprintf[idx])
>   			sc->arg_scnprintf[idx] = sc->fmt->arg_scnprintf[idx];
>   		else if (field->flags & FIELD_IS_POINTER)
> @@ -1515,6 +1517,14 @@ static int trace__read_syscall_info(struct trace *trace, int id)
>   	if (sc->tp_format == NULL)
>   		return -1;
>
> +	sc->args = sc->tp_format->format.fields;
> +	sc->nr_args = sc->tp_format->format.nr_fields;
> +	/* drop nr field - not relevant here; does not exist on older kernels */
> +	if (sc->args && strcmp(sc->args->name, "nr") == 0) {
> +		sc->args = sc->args->next;
> +		--sc->nr_args;
> +	}
> +
>   	sc->is_exit = !strcmp(name, "exit_group") || !strcmp(name, "exit");
>
>   	return syscall__set_arg_fmts(sc);
> @@ -1537,7 +1547,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
>   	unsigned char *p;
>   	unsigned long val;
>
> -	if (sc->tp_format != NULL) {
> +	if (sc->args != NULL) {
>   		struct format_field *field;
>   		u8 bit = 1;
>   		struct syscall_arg arg = {
> @@ -1547,7 +1557,7 @@ static size_t syscall__scnprintf_args(struct syscall *sc, char *bf, size_t size,
>   			.thread = thread,
>   		};
>
> -		for (field = sc->tp_format->format.fields->next; field;
> +		for (field = sc->args; field;
>   		     field = field->next, ++arg.idx, bit <<= 1) {
>   			if (arg.mask & bit)
>   				continue;
>

Seems reasonable.

Re: [PATCH] perf trace: Handle legacy syscalls

[Arnaldo Carvalho de Melo]

Em Fri, Mar 20, 2015 at 10:12:33AM -0600, David Ahern escreveu:
> On 3/20/15 10:06 AM, Arnaldo Carvalho de Melo wrote:
> >-		for (field = sc->tp_format->format.fields->next; field;
> >+		for (field = sc->args; field;
> >  		     field = field->next, ++arg.idx, bit <<= 1) {
> >  			if (arg.mask & bit)
> >  				continue;
> >
> 
> Seems reasonable.

Ok, so I'll add this one, with a Based-on-a-patch-by: you, with your
Acked-by, ok?

- Arnaldo

Re: [PATCH] perf trace: Handle legacy syscalls

[David Ahern]

On 3/20/15 10:26 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 20, 2015 at 10:12:33AM -0600, David Ahern escreveu:
>> On 3/20/15 10:06 AM, Arnaldo Carvalho de Melo wrote:
>>> -		for (field = sc->tp_format->format.fields->next; field;
>>> +		for (field = sc->args; field;
>>>   		     field = field->next, ++arg.idx, bit <<= 1) {
>>>   			if (arg.mask & bit)
>>>   				continue;
>>>
>>
>> Seems reasonable.
>
> Ok, so I'll add this one, with a Based-on-a-patch-by: you, with your
> Acked-by, ok?

yes. thanks for re-working it. it's been in my queue for over a year.

[tip:perf/core] tools lib traceevent: Add destructor for format_field

[tip-bot for David Ahern]

Commit-ID:  00ae1127a03d20f5ef89f3c1fe7d4720270fc2a5
Gitweb:     http://git.kernel.org/tip/00ae1127a03d20f5ef89f3c1fe7d4720270fc2a5
Author:     David Ahern <dsahern@gmail.com>
AuthorDate: Thu, 19 Mar 2015 12:36:21 -0600
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Sat, 21 Mar 2015 14:53:38 -0300

tools lib traceevent: Add destructor for format_field

Move the calls that frees the resources allocated for a struct format_field to
a separate routine.

Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1426790181-19118-1-git-send-email-dsahern@gmail.com
[ Split this part from a larger patch, added pevent_ prefix as requested by Steven ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/event-parse.c | 11 ++++++++---
 tools/lib/traceevent/event-parse.h |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index afe20ed..d7c37a7 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -6228,15 +6228,20 @@ void pevent_ref(struct pevent *pevent)
 	pevent->ref_count++;
 }
 
+void pevent_free_format_field(struct format_field *field)
+{
+	free(field->type);
+	free(field->name);
+	free(field);
+}
+
 static void free_format_fields(struct format_field *field)
 {
 	struct format_field *next;
 
 	while (field) {
 		next = field->next;
-		free(field->type);
-		free(field->name);
-		free(field);
+		pevent_free_format_field(field);
 		field = next;
 	}
 }
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 5b4efc0..6abda54 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -619,6 +619,7 @@ enum pevent_errno pevent_parse_format(struct pevent *pevent,
 				      const char *buf,
 				      unsigned long size, const char *sys);
 void pevent_free_format(struct event_format *event);
+void pevent_free_format_field(struct format_field *field);
 
 void *pevent_get_field_raw(struct trace_seq *s, struct event_format *event,
 			   const char *name, struct pevent_record *record,