2014-01-15 01:45:44

by Namhyung Kim

[permalink] [raw]
Subject: [PATCHSET 00/17] tools lib traceevent: Plugin unload enhancement

Hello,

When plugins are loaded, they register various event and function
handlers. But they don't unregister even after the plugins unloaded
so that events could have refererences to non-existing handlers.

This patchset added relevant unregister functions to handle that.

Note that this is not a problem as of now since all of users unload
plugins after finishing their access to events. But being a generic
library, it should be handled properly IMHO.

The patch 1-4 are resend of the previous die removal series so
independent to this series.

I put the series on the 'libtraceevent/plugin-v1' branch in my tree

git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Any comments are welcome, thanks
Namhyung


Namhyung Kim (17):
tools lib traceevent: Add state member to struct trace_seq
tools lib traceevent: Check return value of realloc()
tools lib traceevent: Get rid of malloc_or_die() in trace_seq_init()
tools lib traceevent: Get rid of die() finally!!
tools lib traceevent: Make plugin unload function receive pevent
tools lib traceevent: Add pevent_unregister_event_handler()
tools lib traceevent: Add pevent_unregister_print_function()
tools lib traceevent: Unregister handler when function plugin
unloaded
tools lib traceevent: Unregister handler when hrtimer plugin unloaded
tools lib traceevent: Unregister handler when kmem plugin unloaded
tools lib traceevent: Unregister handler when kvm plugin unloaded
tools lib traceevent: Unregister handler when sched_switch plugin
unloaded
tools lib traceevent: Unregister handler when mac80211 plugin
unloaded
tools lib traceevent: Unregister handler when cfg80211 plugin
unloaded
tools lib traceevent: Unregister handler when jbd2 plugin unloaded
tools lib traceevent: Unregister handler when scsi plugin unloaded
tools lib traceevent: Unregister handler when xen plugin unloaded

tools/lib/traceevent/Makefile | 2 +-
tools/lib/traceevent/event-parse.c | 136 ++++++++++++++++++++++++++---
tools/lib/traceevent/event-parse.h | 19 +++-
tools/lib/traceevent/event-plugin.c | 4 +-
tools/lib/traceevent/event-utils.h | 4 -
tools/lib/traceevent/parse-utils.c | 44 ----------
tools/lib/traceevent/plugin_cfg80211.c | 7 ++
tools/lib/traceevent/plugin_function.c | 5 +-
tools/lib/traceevent/plugin_hrtimer.c | 10 +++
tools/lib/traceevent/plugin_jbd2.c | 11 +++
tools/lib/traceevent/plugin_kmem.c | 22 +++++
tools/lib/traceevent/plugin_kvm.c | 30 +++++++
tools/lib/traceevent/plugin_mac80211.c | 7 ++
tools/lib/traceevent/plugin_sched_switch.c | 12 +++
tools/lib/traceevent/plugin_scsi.c | 7 ++
tools/lib/traceevent/plugin_xen.c | 7 ++
tools/lib/traceevent/trace-seq.c | 58 ++++++++++--
tools/perf/util/trace-event.c | 2 +-
18 files changed, 310 insertions(+), 77 deletions(-)

--
1.7.11.7


2014-01-15 01:46:00

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 17/17] tools lib traceevent: Unregister handler when xen plugin unloaded

The function handler should be unregistered when the plugin unloaded
otherwise it'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_xen.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/tools/lib/traceevent/plugin_xen.c b/tools/lib/traceevent/plugin_xen.c
index e7794298f3a9..a65353f44302 100644
--- a/tools/lib/traceevent/plugin_xen.c
+++ b/tools/lib/traceevent/plugin_xen.c
@@ -128,3 +128,10 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
PEVENT_FUNC_ARG_VOID);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_print_function(pevent,
+ process_xen_hypercall_name,
+ "xen_hypercall_name");
+}
--
1.7.11.7

2014-01-15 01:45:58

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 16/17] tools lib traceevent: Unregister handler when scsi plugin unloaded

The function handler should be unregistered when the plugin unloaded
otherwise it'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_scsi.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/tools/lib/traceevent/plugin_scsi.c b/tools/lib/traceevent/plugin_scsi.c
index 6fb8e3e3fcad..64d688534518 100644
--- a/tools/lib/traceevent/plugin_scsi.c
+++ b/tools/lib/traceevent/plugin_scsi.c
@@ -421,3 +421,10 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
PEVENT_FUNC_ARG_VOID);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_print_function(pevent,
+ process_scsi_trace_parse_cdb,
+ "scsi_trace_parse_cdb");
+}
--
1.7.11.7

2014-01-15 01:45:55

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 15/17] tools lib traceevent: Unregister handler when jbd2 plugin unloaded

The function handlers should be unregistered when the plugin unloaded
otherwise they'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_jbd2.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/tools/lib/traceevent/plugin_jbd2.c b/tools/lib/traceevent/plugin_jbd2.c
index 2f93f81f0bac..88fe6fe872d5 100644
--- a/tools/lib/traceevent/plugin_jbd2.c
+++ b/tools/lib/traceevent/plugin_jbd2.c
@@ -66,3 +66,14 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
PEVENT_FUNC_ARG_VOID);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_print_function(pevent,
+ process_jbd2_dev_to_name,
+ "jbd2_dev_to_name");
+
+ pevent_unregister_print_function(pevent,
+ process_jiffies_to_msecs,
+ "jiffies_to_msecs");
+}
--
1.7.11.7

2014-01-15 01:45:51

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 09/17] tools lib traceevent: Unregister handler when hrtimer plugin unloaded

The timer handlers should be unregistered when the plugin unloaded
otherwise they'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_hrtimer.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/tools/lib/traceevent/plugin_hrtimer.c b/tools/lib/traceevent/plugin_hrtimer.c
index 0b0ebf30aa44..12bf14cc1152 100644
--- a/tools/lib/traceevent/plugin_hrtimer.c
+++ b/tools/lib/traceevent/plugin_hrtimer.c
@@ -76,3 +76,13 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
timer_start_handler, NULL);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_event_handler(pevent, -1,
+ "timer", "hrtimer_expire_entry",
+ timer_expire_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "timer", "hrtimer_start",
+ timer_start_handler, NULL);
+}
--
1.7.11.7

2014-01-15 01:45:49

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 03/17] tools lib traceevent: Get rid of malloc_or_die() in trace_seq_init()

Use plain malloc() and check its return value.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/trace-seq.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index 941d35d2cf87..339a0ffa21d5 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -47,8 +47,11 @@ void trace_seq_init(struct trace_seq *s)
s->len = 0;
s->readpos = 0;
s->buffer_size = TRACE_SEQ_BUF_SIZE;
- s->buffer = malloc_or_die(s->buffer_size);
- s->state = TRACE_SEQ__GOOD;
+ s->buffer = malloc(s->buffer_size);
+ if (s->buffer != NULL)
+ s->state = TRACE_SEQ__GOOD;
+ else
+ s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
}

/**
--
1.7.11.7

2014-01-15 01:46:55

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 14/17] tools lib traceevent: Unregister handler when cfg80211 plugin unloaded

The function handler should be unregistered when the plugin unloaded
otherwise it'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_cfg80211.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/tools/lib/traceevent/plugin_cfg80211.c b/tools/lib/traceevent/plugin_cfg80211.c
index dcab8e873c21..7b07d149557b 100644
--- a/tools/lib/traceevent/plugin_cfg80211.c
+++ b/tools/lib/traceevent/plugin_cfg80211.c
@@ -22,3 +22,10 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
PEVENT_FUNC_ARG_VOID);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_print_function(pevent,
+ process___le16_to_cpup,
+ "__le16_to_cpup");
+}
--
1.7.11.7

2014-01-15 01:47:04

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 11/17] tools lib traceevent: Unregister handler when kvm plugin unloaded

The kvm handlers should be unregistered when the plugin unloaded
otherwise they'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_kvm.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/tools/lib/traceevent/plugin_kvm.c b/tools/lib/traceevent/plugin_kvm.c
index a0e282c6b967..6a1eeadbf7a1 100644
--- a/tools/lib/traceevent/plugin_kvm.c
+++ b/tools/lib/traceevent/plugin_kvm.c
@@ -434,3 +434,33 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
PEVENT_FUNC_ARG_VOID);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_event_handler(pevent, -1, "kvm", "kvm_exit",
+ kvm_exit_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kvm", "kvm_emulate_insn",
+ kvm_emulate_insn_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_get_page",
+ kvm_mmu_get_page_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_sync_page",
+ kvm_mmu_print_role, NULL);
+
+ pevent_unregister_event_handler(pevent, -1,
+ "kvmmmu", "kvm_mmu_unsync_page",
+ kvm_mmu_print_role, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kvmmmu", "kvm_mmu_zap_page",
+ kvm_mmu_print_role, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kvmmmu",
+ "kvm_mmu_prepare_zap_page", kvm_mmu_print_role,
+ NULL);
+
+ pevent_unregister_print_function(pevent,
+ process_is_writable_pte,
+ "is_writable_pte");
+}
--
1.7.11.7

2014-01-15 01:47:02

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 12/17] tools lib traceevent: Unregister handler when sched_switch plugin unloaded

The event handlers should be unregistered when the plugin unloaded
otherwise they'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_sched_switch.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/tools/lib/traceevent/plugin_sched_switch.c b/tools/lib/traceevent/plugin_sched_switch.c
index fea3724aa24f..f1ce60065258 100644
--- a/tools/lib/traceevent/plugin_sched_switch.c
+++ b/tools/lib/traceevent/plugin_sched_switch.c
@@ -146,3 +146,15 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
sched_wakeup_handler, NULL);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_event_handler(pevent, -1, "sched", "sched_switch",
+ sched_switch_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "sched", "sched_wakeup",
+ sched_wakeup_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "sched", "sched_wakeup_new",
+ sched_wakeup_handler, NULL);
+}
--
1.7.11.7

2014-01-15 01:46:59

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 13/17] tools lib traceevent: Unregister handler when mac80211 plugin unloaded

The event handler should be unregistered when the plugin unloaded
otherwise it'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_mac80211.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/tools/lib/traceevent/plugin_mac80211.c b/tools/lib/traceevent/plugin_mac80211.c
index 558a3b91c046..7e15a0f1c2fd 100644
--- a/tools/lib/traceevent/plugin_mac80211.c
+++ b/tools/lib/traceevent/plugin_mac80211.c
@@ -93,3 +93,10 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
drv_bss_info_changed, NULL);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_event_handler(pevent, -1, "mac80211",
+ "drv_bss_info_changed",
+ drv_bss_info_changed, NULL);
+}
--
1.7.11.7

2014-01-15 01:48:13

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 10/17] tools lib traceevent: Unregister handler when kmem plugin unloaded

The kmem handlers should be unregistered when the plugin unloaded
otherwise they'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_kmem.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)

diff --git a/tools/lib/traceevent/plugin_kmem.c b/tools/lib/traceevent/plugin_kmem.c
index 7115c8037ea8..70650ff48d78 100644
--- a/tools/lib/traceevent/plugin_kmem.c
+++ b/tools/lib/traceevent/plugin_kmem.c
@@ -70,3 +70,25 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
call_site_handler, NULL);
return 0;
}
+
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
+{
+ pevent_unregister_event_handler(pevent, -1, "kmem", "kfree",
+ call_site_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kmem", "kmalloc",
+ call_site_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kmem", "kmalloc_node",
+ call_site_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kmem", "kmem_cache_alloc",
+ call_site_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kmem",
+ "kmem_cache_alloc_node",
+ call_site_handler, NULL);
+
+ pevent_unregister_event_handler(pevent, -1, "kmem", "kmem_cache_free",
+ call_site_handler, NULL);
+}
--
1.7.11.7

2014-01-15 01:45:45

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 01/17] tools lib traceevent: Add state member to struct trace_seq

The trace_seq->state is for tracking errors during the use of
trace_seq APIs and getting rid of die() in it.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/Makefile | 2 +-
tools/lib/traceevent/event-parse.h | 7 ++++++
tools/lib/traceevent/trace-seq.c | 46 +++++++++++++++++++++++++++++++++-----
3 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/tools/lib/traceevent/Makefile b/tools/lib/traceevent/Makefile
index f778d48ac609..56d52a33a3df 100644
--- a/tools/lib/traceevent/Makefile
+++ b/tools/lib/traceevent/Makefile
@@ -136,7 +136,7 @@ export Q VERBOSE

EVENT_PARSE_VERSION = $(EP_VERSION).$(EP_PATCHLEVEL).$(EP_EXTRAVERSION)

-INCLUDES = -I. $(CONFIG_INCLUDES)
+INCLUDES = -I. -I $(srctree)/../../include $(CONFIG_INCLUDES)

# Set compile option CFLAGS if not set elsewhere
CFLAGS ?= -g -Wall
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index cf5db9013f2c..3c890cb28db7 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -58,6 +58,12 @@ struct pevent_record {
#endif
};

+enum trace_seq_fail {
+ TRACE_SEQ__GOOD,
+ TRACE_SEQ__BUFFER_POISONED,
+ TRACE_SEQ__MEM_ALLOC_FAILED,
+};
+
/*
* Trace sequences are used to allow a function to call several other functions
* to create a string of data to use (up to a max of PAGE_SIZE).
@@ -68,6 +74,7 @@ struct trace_seq {
unsigned int buffer_size;
unsigned int len;
unsigned int readpos;
+ enum trace_seq_fail state;
};

void trace_seq_init(struct trace_seq *s);
diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index d7f2e68bc5b9..7f24e8989401 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -22,6 +22,7 @@
#include <string.h>
#include <stdarg.h>

+#include <asm/bug.h>
#include "event-parse.h"
#include "event-utils.h"

@@ -32,8 +33,9 @@
#define TRACE_SEQ_POISON ((void *)0xdeadbeef)
#define TRACE_SEQ_CHECK(s) \
do { \
- if ((s)->buffer == TRACE_SEQ_POISON) \
- die("Usage of trace_seq after it was destroyed"); \
+ if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
+ "Usage of trace_seq after it was destroyed")) \
+ (s)->state = TRACE_SEQ__BUFFER_POISONED; \
} while (0)

/**
@@ -46,6 +48,7 @@ void trace_seq_init(struct trace_seq *s)
s->readpos = 0;
s->buffer_size = TRACE_SEQ_BUF_SIZE;
s->buffer = malloc_or_die(s->buffer_size);
+ s->state = TRACE_SEQ__GOOD;
}

/**
@@ -80,8 +83,9 @@ static void expand_buffer(struct trace_seq *s)
{
s->buffer_size += TRACE_SEQ_BUF_SIZE;
s->buffer = realloc(s->buffer, s->buffer_size);
- if (!s->buffer)
- die("Can't allocate trace_seq buffer memory");
+ if (WARN_ONCE(!s->buffer,
+ "Can't allocate trace_seq buffer memory"))
+ s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
}

/**
@@ -108,6 +112,9 @@ trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
TRACE_SEQ_CHECK(s);

try_again:
+ if (s->state != TRACE_SEQ__GOOD)
+ return 0;
+
len = (s->buffer_size - 1) - s->len;

va_start(ap, fmt);
@@ -144,6 +151,9 @@ trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
TRACE_SEQ_CHECK(s);

try_again:
+ if (s->state != TRACE_SEQ__GOOD)
+ return 0;
+
len = (s->buffer_size - 1) - s->len;

ret = vsnprintf(s->buffer + s->len, len, fmt, args);
@@ -174,11 +184,17 @@ int trace_seq_puts(struct trace_seq *s, const char *str)

TRACE_SEQ_CHECK(s);

+ if (s->state != TRACE_SEQ__GOOD)
+ return 0;
+
len = strlen(str);

while (len > ((s->buffer_size - 1) - s->len))
expand_buffer(s);

+ if (s->state != TRACE_SEQ__GOOD)
+ return 0;
+
memcpy(s->buffer + s->len, str, len);
s->len += len;

@@ -189,9 +205,15 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)
{
TRACE_SEQ_CHECK(s);

+ if (s->state != TRACE_SEQ__GOOD)
+ return 0;
+
while (s->len >= (s->buffer_size - 1))
expand_buffer(s);

+ if (s->state != TRACE_SEQ__GOOD)
+ return 0;
+
s->buffer[s->len++] = c;

return 1;
@@ -201,6 +223,9 @@ void trace_seq_terminate(struct trace_seq *s)
{
TRACE_SEQ_CHECK(s);

+ if (s->state != TRACE_SEQ__GOOD)
+ return;
+
/* There's always one character left on the buffer */
s->buffer[s->len] = 0;
}
@@ -208,5 +233,16 @@ void trace_seq_terminate(struct trace_seq *s)
int trace_seq_do_printf(struct trace_seq *s)
{
TRACE_SEQ_CHECK(s);
- return printf("%.*s", s->len, s->buffer);
+
+ switch (s->state) {
+ case TRACE_SEQ__GOOD:
+ return printf("%.*s", s->len, s->buffer);
+ case TRACE_SEQ__BUFFER_POISONED:
+ puts("Usage of trace_seq after it was destroyed");
+ break;
+ case TRACE_SEQ__MEM_ALLOC_FAILED:
+ puts("Can't allocate trace_seq buffer memory");
+ break;
+ }
+ return -1;
}
--
1.7.11.7

2014-01-15 01:48:33

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 08/17] tools lib traceevent: Unregister handler when function plugin unloaded

The function handler should be unregistered when the plugin unloaded
otherwise it'll try to access invalid memory.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/plugin_function.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
index 39461485f9a7..80ba4ff1fe84 100644
--- a/tools/lib/traceevent/plugin_function.c
+++ b/tools/lib/traceevent/plugin_function.c
@@ -148,6 +148,9 @@ void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
{
int i, x;

+ pevent_unregister_event_handler(pevent, -1, "ftrace", "function",
+ function_handler, NULL);
+
for (i = 0; i <= cpus; i++) {
for (x = 0; x < fstack[i].size && fstack[i].stack[x]; x++)
free(fstack[i].stack[x]);
--
1.7.11.7

2014-01-15 01:48:56

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 07/17] tools lib traceevent: Add pevent_unregister_print_function()

When a plugin unloaded it needs to unregister its print handler from
pevent. So add unregister function to do it.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/event-parse.c | 23 +++++++++++++++++++++++
tools/lib/traceevent/event-parse.h | 2 ++
2 files changed, 25 insertions(+)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 05ae4b4ed0c8..fe962fbb156c 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -5560,6 +5560,29 @@ int pevent_register_print_function(struct pevent *pevent,
return ret;
}

+/**
+ * pevent_unregister_print_function - unregister a helper function
+ * @pevent: the handle to the pevent
+ * @func: the function to process the helper function
+ * @name: the name of the helper function
+ *
+ * This function removes existing print handler for function @name.
+ *
+ * Returns 0 if the handler was removed successully, -1 otherwise.
+ */
+int pevent_unregister_print_function(struct pevent *pevent,
+ pevent_func_handler func, char *name)
+{
+ struct pevent_function_handler *func_handle;
+
+ func_handle = find_func_handler(pevent, name);
+ if (func_handle && func_handle->func == func) {
+ remove_func_handler(pevent, name);
+ return 0;
+ }
+ return -1;
+}
+
static struct event_format *pevent_search_event(struct pevent *pevent, int id,
const char *sys_name,
const char *event_name)
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index c48acfbc6230..791c539374c7 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -631,6 +631,8 @@ int pevent_register_print_function(struct pevent *pevent,
pevent_func_handler func,
enum pevent_func_arg_type ret_type,
char *name, ...);
+int pevent_unregister_print_function(struct pevent *pevent,
+ pevent_func_handler func, char *name);

struct format_field *pevent_find_common_field(struct event_format *event, const char *name);
struct format_field *pevent_find_field(struct event_format *event, const char *name);
--
1.7.11.7

2014-01-15 01:48:57

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 06/17] tools lib traceevent: Add pevent_unregister_event_handler()

When a plugin unloaded it needs to unregister its handler from pevent.
So add unregister function to do it.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/event-parse.c | 113 ++++++++++++++++++++++++++++++++-----
tools/lib/traceevent/event-parse.h | 3 +
2 files changed, 102 insertions(+), 14 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 2ce565a73dd5..05ae4b4ed0c8 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -5560,6 +5560,29 @@ int pevent_register_print_function(struct pevent *pevent,
return ret;
}

+static struct event_format *pevent_search_event(struct pevent *pevent, int id,
+ const char *sys_name,
+ const char *event_name)
+{
+ struct event_format *event;
+
+ if (id >= 0) {
+ /* search by id */
+ event = pevent_find_event(pevent, id);
+ if (!event)
+ return NULL;
+ if (event_name && (strcmp(event_name, event->name) != 0))
+ return NULL;
+ if (sys_name && (strcmp(sys_name, event->system) != 0))
+ return NULL;
+ } else {
+ event = pevent_find_event_by_name(pevent, sys_name, event_name);
+ if (!event)
+ return NULL;
+ }
+ return event;
+}
+
/**
* pevent_register_event_handler - register a way to parse an event
* @pevent: the handle to the pevent
@@ -5584,20 +5607,9 @@ int pevent_register_event_handler(struct pevent *pevent, int id,
struct event_format *event;
struct event_handler *handle;

- if (id >= 0) {
- /* search by id */
- event = pevent_find_event(pevent, id);
- if (!event)
- goto not_found;
- if (event_name && (strcmp(event_name, event->name) != 0))
- goto not_found;
- if (sys_name && (strcmp(sys_name, event->system) != 0))
- goto not_found;
- } else {
- event = pevent_find_event_by_name(pevent, sys_name, event_name);
- if (!event)
- goto not_found;
- }
+ event = pevent_search_event(pevent, id, sys_name, event_name);
+ if (event == NULL)
+ goto not_found;

pr_stat("overriding event (%d) %s:%s with new print handler",
event->id, event->system, event->name);
@@ -5637,6 +5649,79 @@ int pevent_register_event_handler(struct pevent *pevent, int id,
return -1;
}

+static int handle_matches(struct event_handler *handler, int id,
+ const char *sys_name, const char *event_name,
+ pevent_event_handler_func func, void *context)
+{
+ if (id >= 0 && id != handler->id)
+ return 0;
+
+ if (event_name && (strcmp(event_name, handler->event_name) != 0))
+ return 0;
+
+ if (sys_name && (strcmp(sys_name, handler->sys_name) != 0))
+ return 0;
+
+ if (func != handler->func || context != handler->context)
+ return 0;
+
+ return 1;
+}
+
+/**
+ * pevent_unregister_event_handler - unregister an existing event handler
+ * @pevent: the handle to the pevent
+ * @id: the id of the event to unregister
+ * @sys_name: the system name the handler belongs to
+ * @event_name: the name of the event handler
+ * @func: the function to call to parse the event information
+ * @context: the data to be passed to @func
+ *
+ * This function removes existing event handler (parser).
+ *
+ * If @id is >= 0, then it is used to find the event.
+ * else @sys_name and @event_name are used.
+ *
+ * Returns 0 if handler was removed successfully, -1 if event was not found.
+ */
+int pevent_unregister_event_handler(struct pevent *pevent, int id,
+ const char *sys_name, const char *event_name,
+ pevent_event_handler_func func, void *context)
+{
+ struct event_format *event;
+ struct event_handler *handle;
+ struct event_handler **next;
+
+ event = pevent_search_event(pevent, id, sys_name, event_name);
+ if (event == NULL)
+ goto not_found;
+
+ if (event->handler == func && event->context == context) {
+ pr_stat("overriding event (%d) %s:%s with default print handler",
+ event->id, event->system, event->name);
+
+ event->handler = NULL;
+ event->context = NULL;
+ return 0;
+ }
+
+not_found:
+ for (next = &pevent->handlers; *next; next = &(*next)->next) {
+ handle = *next;
+ if (handle_matches(handle, id, sys_name, event_name,
+ func, context))
+ break;
+ }
+
+ if (!(*next))
+ return -1;
+
+ *next = handle->next;
+ free_handler(handle);
+
+ return 0;
+}
+
/**
* pevent_alloc - create a pevent handle
*/
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index a3beca56cb35..c48acfbc6230 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -624,6 +624,9 @@ int pevent_print_func_field(struct trace_seq *s, const char *fmt,
int pevent_register_event_handler(struct pevent *pevent, int id,
const char *sys_name, const char *event_name,
pevent_event_handler_func func, void *context);
+int pevent_unregister_event_handler(struct pevent *pevent, int id,
+ const char *sys_name, const char *event_name,
+ pevent_event_handler_func func, void *context);
int pevent_register_print_function(struct pevent *pevent,
pevent_func_handler func,
enum pevent_func_arg_type ret_type,
--
1.7.11.7

2014-01-15 01:49:29

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 05/17] tools lib traceevent: Make plugin unload function receive pevent

The PEVENT_PLUGIN_UNLOADER function might need some cleanup using
pevent like unregister some handlers. So pass pevent as argument.

Cc: Jiri Olsa <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/event-parse.h | 7 ++++---
tools/lib/traceevent/event-plugin.c | 4 ++--
tools/lib/traceevent/plugin_function.c | 2 +-
tools/perf/util/trace-event.c | 2 +-
4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 3c890cb28db7..a3beca56cb35 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -105,7 +105,7 @@ typedef int (*pevent_event_handler_func)(struct trace_seq *s,
void *context);

typedef int (*pevent_plugin_load_func)(struct pevent *pevent);
-typedef int (*pevent_plugin_unload_func)(void);
+typedef int (*pevent_plugin_unload_func)(struct pevent *pevent);

struct plugin_option {
struct plugin_option *next;
@@ -130,7 +130,7 @@ struct plugin_option {
* PEVENT_PLUGIN_UNLOADER: (optional)
* The function called just before unloading
*
- * int PEVENT_PLUGIN_UNLOADER(void)
+ * int PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
*
* PEVENT_PLUGIN_OPTIONS: (optional)
* Plugin options that can be set before loading
@@ -411,7 +411,8 @@ enum pevent_errno {
struct plugin_list;

struct plugin_list *traceevent_load_plugins(struct pevent *pevent);
-void traceevent_unload_plugins(struct plugin_list *plugin_list);
+void traceevent_unload_plugins(struct plugin_list *plugin_list,
+ struct pevent *pevent);

struct cmdline;
struct cmdline_list;
diff --git a/tools/lib/traceevent/event-plugin.c b/tools/lib/traceevent/event-plugin.c
index 125f5676bcb5..0c8bf6780e4d 100644
--- a/tools/lib/traceevent/event-plugin.c
+++ b/tools/lib/traceevent/event-plugin.c
@@ -197,7 +197,7 @@ traceevent_load_plugins(struct pevent *pevent)
}

void
-traceevent_unload_plugins(struct plugin_list *plugin_list)
+traceevent_unload_plugins(struct plugin_list *plugin_list, struct pevent *pevent)
{
pevent_plugin_unload_func func;
struct plugin_list *list;
@@ -207,7 +207,7 @@ traceevent_unload_plugins(struct plugin_list *plugin_list)
plugin_list = list->next;
func = dlsym(list->handle, PEVENT_PLUGIN_UNLOADER_NAME);
if (func)
- func();
+ func(pevent);
dlclose(list->handle);
free(list->name);
free(list);
diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
index aad92ad5e96f..39461485f9a7 100644
--- a/tools/lib/traceevent/plugin_function.c
+++ b/tools/lib/traceevent/plugin_function.c
@@ -144,7 +144,7 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
return 0;
}

-void PEVENT_PLUGIN_UNLOADER(void)
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
{
int i, x;

diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index d9f5f6137ab3..6322d37164c5 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -34,8 +34,8 @@ int trace_event__init(struct trace_event *t)

void trace_event__cleanup(struct trace_event *t)
{
+ traceevent_unload_plugins(t->plugin_list, t->pevent);
pevent_free(t->pevent);
- traceevent_unload_plugins(t->plugin_list);
}

static struct event_format*
--
1.7.11.7

2014-01-15 01:49:48

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 04/17] tools lib traceevent: Get rid of die() finally!!

Now all of its users were gone. :)

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/event-utils.h | 4 ----
tools/lib/traceevent/parse-utils.c | 44 --------------------------------------
2 files changed, 48 deletions(-)

diff --git a/tools/lib/traceevent/event-utils.h b/tools/lib/traceevent/event-utils.h
index e76c9acb92cd..d1dc2170e402 100644
--- a/tools/lib/traceevent/event-utils.h
+++ b/tools/lib/traceevent/event-utils.h
@@ -23,18 +23,14 @@
#include <ctype.h>

/* Can be overridden */
-void die(const char *fmt, ...);
-void *malloc_or_die(unsigned int size);
void warning(const char *fmt, ...);
void pr_stat(const char *fmt, ...);
void vpr_stat(const char *fmt, va_list ap);

/* Always available */
-void __die(const char *fmt, ...);
void __warning(const char *fmt, ...);
void __pr_stat(const char *fmt, ...);

-void __vdie(const char *fmt, ...);
void __vwarning(const char *fmt, ...);
void __vpr_stat(const char *fmt, ...);

diff --git a/tools/lib/traceevent/parse-utils.c b/tools/lib/traceevent/parse-utils.c
index bba701cf10e6..eda07fa31dca 100644
--- a/tools/lib/traceevent/parse-utils.c
+++ b/tools/lib/traceevent/parse-utils.c
@@ -25,40 +25,6 @@

#define __weak __attribute__((weak))

-void __vdie(const char *fmt, va_list ap)
-{
- int ret = errno;
-
- if (errno)
- perror("trace-cmd");
- else
- ret = -1;
-
- fprintf(stderr, " ");
- vfprintf(stderr, fmt, ap);
-
- fprintf(stderr, "\n");
- exit(ret);
-}
-
-void __die(const char *fmt, ...)
-{
- va_list ap;
-
- va_start(ap, fmt);
- __vdie(fmt, ap);
- va_end(ap);
-}
-
-void __weak die(const char *fmt, ...)
-{
- va_list ap;
-
- va_start(ap, fmt);
- __vdie(fmt, ap);
- va_end(ap);
-}
-
void __vwarning(const char *fmt, va_list ap)
{
if (errno)
@@ -117,13 +83,3 @@ void __weak pr_stat(const char *fmt, ...)
__vpr_stat(fmt, ap);
va_end(ap);
}
-
-void __weak *malloc_or_die(unsigned int size)
-{
- void *data;
-
- data = malloc(size);
- if (!data)
- die("malloc");
- return data;
-}
--
1.7.11.7

2014-01-15 01:50:10

by Namhyung Kim

[permalink] [raw]
Subject: [PATCH 02/17] tools lib traceevent: Check return value of realloc()

If realloc() fails, it'll leak the buffer. Also increate buffer size
only if the allocation succeeded.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/trace-seq.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index 7f24e8989401..941d35d2cf87 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -81,11 +81,16 @@ void trace_seq_destroy(struct trace_seq *s)

static void expand_buffer(struct trace_seq *s)
{
- s->buffer_size += TRACE_SEQ_BUF_SIZE;
- s->buffer = realloc(s->buffer, s->buffer_size);
- if (WARN_ONCE(!s->buffer,
- "Can't allocate trace_seq buffer memory"))
+ char *buf;
+
+ buf = realloc(s->buffer, s->buffer_size + TRACE_SEQ_BUF_SIZE);
+ if (WARN_ONCE(!buf, "Can't allocate trace_seq buffer memory")) {
s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
+ return;
+ }
+
+ s->buffer = buf;
+ s->buffer_size += TRACE_SEQ_BUF_SIZE;
}

/**
--
1.7.11.7

2014-01-15 02:01:07

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 01/17] tools lib traceevent: Add state member to struct trace_seq

On Wed, 15 Jan 2014 10:45:24 +0900
Namhyung Kim <[email protected]> wrote:

>
> @@ -32,8 +33,9 @@
> #define TRACE_SEQ_POISON ((void *)0xdeadbeef)
> #define TRACE_SEQ_CHECK(s) \
> do { \
> - if ((s)->buffer == TRACE_SEQ_POISON) \
> - die("Usage of trace_seq after it was destroyed"); \
> + if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
> + "Usage of trace_seq after it was destroyed")) \
> + (s)->state = TRACE_SEQ__BUFFER_POISONED; \
> } while (0)
>
> /**
> @@ -46,6 +48,7 @@ void trace_seq_init(struct trace_seq *s)
> s->readpos = 0;
> s->buffer_size = TRACE_SEQ_BUF_SIZE;
> s->buffer = malloc_or_die(s->buffer_size);
> + s->state = TRACE_SEQ__GOOD;
> }
>
> /**
> @@ -80,8 +83,9 @@ static void expand_buffer(struct trace_seq *s)
> {
> s->buffer_size += TRACE_SEQ_BUF_SIZE;
> s->buffer = realloc(s->buffer, s->buffer_size);
> - if (!s->buffer)
> - die("Can't allocate trace_seq buffer memory");
> + if (WARN_ONCE(!s->buffer,
> + "Can't allocate trace_seq buffer memory"))
> + s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
> }
>
> /**
> @@ -108,6 +112,9 @@ trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
> TRACE_SEQ_CHECK(s);
>
> try_again:
> + if (s->state != TRACE_SEQ__GOOD)
> + return 0;
> +
> len = (s->buffer_size - 1) - s->len;
>
> va_start(ap, fmt);
> @@ -144,6 +151,9 @@ trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
> TRACE_SEQ_CHECK(s);
>
> try_again:
> + if (s->state != TRACE_SEQ__GOOD)
> + return 0;
> +
> len = (s->buffer_size - 1) - s->len;
>
> ret = vsnprintf(s->buffer + s->len, len, fmt, args);
> @@ -174,11 +184,17 @@ int trace_seq_puts(struct trace_seq *s, const char *str)
>
> TRACE_SEQ_CHECK(s);
>
> + if (s->state != TRACE_SEQ__GOOD)
> + return 0;
> +
> len = strlen(str);
>
> while (len > ((s->buffer_size - 1) - s->len))
> expand_buffer(s);
>
> + if (s->state != TRACE_SEQ__GOOD)
> + return 0;
> +
> memcpy(s->buffer + s->len, str, len);
> s->len += len;
>
> @@ -189,9 +205,15 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)
> {
> TRACE_SEQ_CHECK(s);
>
> + if (s->state != TRACE_SEQ__GOOD)
> + return 0;
> +

Instead of adding all of these, we can extend the macro
TRACE_SEQ_CHECK() which does a
if (s->state != TRACE_SEQ__GOOD)
return;

and a TRACE_SEQ_CHECK_RET() that does a return 0;

-- Steve

> while (s->len >= (s->buffer_size - 1))
> expand_buffer(s);
>
> + if (s->state != TRACE_SEQ__GOOD)
> + return 0;
> +
> s->buffer[s->len++] = c;
>
> return 1;
> @@ -201,6 +223,9 @@ void trace_seq_terminate(struct trace_seq *s)
> {
> TRACE_SEQ_CHECK(s);
>
> + if (s->state != TRACE_SEQ__GOOD)
> + return;
> +
> /* There's always one character left on the buffer */
> s->buffer[s->len] = 0;
> }
> @@ -208,5 +233,16 @@ void trace_seq_terminate(struct trace_seq *s)
> int trace_seq_do_printf(struct trace_seq *s)
> {
> TRACE_SEQ_CHECK(s);
> - return printf("%.*s", s->len, s->buffer);
> +
> + switch (s->state) {
> + case TRACE_SEQ__GOOD:
> + return printf("%.*s", s->len, s->buffer);
> + case TRACE_SEQ__BUFFER_POISONED:
> + puts("Usage of trace_seq after it was destroyed");
> + break;
> + case TRACE_SEQ__MEM_ALLOC_FAILED:
> + puts("Can't allocate trace_seq buffer memory");
> + break;
> + }
> + return -1;
> }

2014-01-15 02:03:55

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 02/17] tools lib traceevent: Check return value of realloc()

On Wed, 15 Jan 2014 10:45:25 +0900
Namhyung Kim <[email protected]> wrote:

> If realloc() fails, it'll leak the buffer. Also increate buffer size
> only if the allocation succeeded.
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---

Acked-by: Steven Rostedt <[email protected]>

-- Steve

2014-01-15 02:05:50

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 03/17] tools lib traceevent: Get rid of malloc_or_die() in trace_seq_init()

On Wed, 15 Jan 2014 10:45:26 +0900
Namhyung Kim <[email protected]> wrote:

> Use plain malloc() and check its return value.
>
> Signed-off-by: Namhyung Kim <[email protected]>

Acked-by: Steven Rostedt <[email protected]>

-- Steve

> ---
> tools/lib/traceevent/trace-seq.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
> index 941d35d2cf87..339a0ffa21d5 100644
> --- a/tools/lib/traceevent/trace-seq.c
> +++ b/tools/lib/traceevent/trace-seq.c
> @@ -47,8 +47,11 @@ void trace_seq_init(struct trace_seq *s)
> s->len = 0;
> s->readpos = 0;
> s->buffer_size = TRACE_SEQ_BUF_SIZE;
> - s->buffer = malloc_or_die(s->buffer_size);
> - s->state = TRACE_SEQ__GOOD;
> + s->buffer = malloc(s->buffer_size);
> + if (s->buffer != NULL)
> + s->state = TRACE_SEQ__GOOD;
> + else
> + s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
> }
>
> /**

2014-01-15 02:24:30

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 05/17] tools lib traceevent: Make plugin unload function receive pevent

On Wed, 15 Jan 2014 10:45:28 +0900
Namhyung Kim <[email protected]> wrote:

> The PEVENT_PLUGIN_UNLOADER function might need some cleanup using
> pevent like unregister some handlers. So pass pevent as argument.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>

Acked-by: Steven Rostedt <[email protected]>

> ---
> tools/lib/traceevent/event-parse.h | 7 ++++---
> tools/lib/traceevent/event-plugin.c | 4 ++--
> tools/lib/traceevent/plugin_function.c | 2 +-
> tools/perf/util/trace-event.c | 2 +-
> 4 files changed, 8 insertions(+), 7 deletions(-)
>

2014-01-15 02:49:35

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH 01/17] tools lib traceevent: Add state member to struct trace_seq

Hi Steve,

On Tue, 14 Jan 2014 21:00:58 -0500, Steven Rostedt wrote:
> On Wed, 15 Jan 2014 10:45:24 +0900
> Namhyung Kim <[email protected]> wrote:
>
>>
>> @@ -32,8 +33,9 @@
>> #define TRACE_SEQ_POISON ((void *)0xdeadbeef)
>> #define TRACE_SEQ_CHECK(s) \
>> do { \
>> - if ((s)->buffer == TRACE_SEQ_POISON) \
>> - die("Usage of trace_seq after it was destroyed"); \
>> + if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
>> + "Usage of trace_seq after it was destroyed")) \
>> + (s)->state = TRACE_SEQ__BUFFER_POISONED; \
>> } while (0)
>>
>> @@ -189,9 +205,15 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)
>> {
>> TRACE_SEQ_CHECK(s);
>>
>> + if (s->state != TRACE_SEQ__GOOD)
>> + return 0;
>> +
>
> Instead of adding all of these, we can extend the macro
> TRACE_SEQ_CHECK() which does a
> if (s->state != TRACE_SEQ__GOOD)
> return;
>
> and a TRACE_SEQ_CHECK_RET() that does a return 0;

Oh, it looks better. But I'd like to TRACE_SEQ_CHECK() as is for some
cases. How about this?


>From 0b43e4f8a9689ab47bdf55d5eddf122f300577e7 Mon Sep 17 00:00:00 2001
From: Namhyung Kim <[email protected]>
Date: Thu, 19 Dec 2013 18:17:44 +0900
Subject: [PATCH] tools lib traceevent: Add state member to struct trace_seq

The trace_seq->state is for tracking errors during the use of
trace_seq APIs and getting rid of die() in it.

Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/Makefile | 2 +-
tools/lib/traceevent/event-parse.h | 7 +++++
tools/lib/traceevent/trace-seq.c | 53 +++++++++++++++++++++++++++++---------
3 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/tools/lib/traceevent/Makefile b/tools/lib/traceevent/Makefile
index f778d48ac609..56d52a33a3df 100644
--- a/tools/lib/traceevent/Makefile
+++ b/tools/lib/traceevent/Makefile
@@ -136,7 +136,7 @@ export Q VERBOSE

EVENT_PARSE_VERSION = $(EP_VERSION).$(EP_PATCHLEVEL).$(EP_EXTRAVERSION)

-INCLUDES = -I. $(CONFIG_INCLUDES)
+INCLUDES = -I. -I $(srctree)/../../include $(CONFIG_INCLUDES)

# Set compile option CFLAGS if not set elsewhere
CFLAGS ?= -g -Wall
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index cf5db9013f2c..3c890cb28db7 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -58,6 +58,12 @@ struct pevent_record {
#endif
};

+enum trace_seq_fail {
+ TRACE_SEQ__GOOD,
+ TRACE_SEQ__BUFFER_POISONED,
+ TRACE_SEQ__MEM_ALLOC_FAILED,
+};
+
/*
* Trace sequences are used to allow a function to call several other functions
* to create a string of data to use (up to a max of PAGE_SIZE).
@@ -68,6 +74,7 @@ struct trace_seq {
unsigned int buffer_size;
unsigned int len;
unsigned int readpos;
+ enum trace_seq_fail state;
};

void trace_seq_init(struct trace_seq *s);
diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index d7f2e68bc5b9..c39048b0de08 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -22,6 +22,7 @@
#include <string.h>
#include <stdarg.h>

+#include <asm/bug.h>
#include "event-parse.h"
#include "event-utils.h"

@@ -32,10 +33,21 @@
#define TRACE_SEQ_POISON ((void *)0xdeadbeef)
#define TRACE_SEQ_CHECK(s) \
do { \
- if ((s)->buffer == TRACE_SEQ_POISON) \
- die("Usage of trace_seq after it was destroyed"); \
+ if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
+ "Usage of trace_seq after it was destroyed")) \
+ (s)->state = TRACE_SEQ__BUFFER_POISONED; \
} while (0)

+#define TRACE_SEQ_CHECK_RET_N(s, n) \
+do { \
+ TRACE_SEQ_CHECK(s); \
+ if ((s)->state != TRACE_SEQ__GOOD) \
+ return n; \
+} while (0)
+
+#define TRACE_SEQ_CHECK_RET(s) TRACE_SEQ_CHECK_RET_N(s, )
+#define TRACE_SEQ_CHECK_RET0(s) TRACE_SEQ_CHECK_RET_N(s, 0)
+
/**
* trace_seq_init - initialize the trace_seq structure
* @s: a pointer to the trace_seq structure to initialize
@@ -46,6 +58,7 @@ void trace_seq_init(struct trace_seq *s)
s->readpos = 0;
s->buffer_size = TRACE_SEQ_BUF_SIZE;
s->buffer = malloc_or_die(s->buffer_size);
+ s->state = TRACE_SEQ__GOOD;
}

/**
@@ -80,8 +93,9 @@ static void expand_buffer(struct trace_seq *s)
{
s->buffer_size += TRACE_SEQ_BUF_SIZE;
s->buffer = realloc(s->buffer, s->buffer_size);
- if (!s->buffer)
- die("Can't allocate trace_seq buffer memory");
+ if (WARN_ONCE(!s->buffer,
+ "Can't allocate trace_seq buffer memory"))
+ s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
}

/**
@@ -105,9 +119,9 @@ trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
int len;
int ret;

- TRACE_SEQ_CHECK(s);
-
try_again:
+ TRACE_SEQ_CHECK_RET0(s);
+
len = (s->buffer_size - 1) - s->len;

va_start(ap, fmt);
@@ -141,9 +155,9 @@ trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
int len;
int ret;

- TRACE_SEQ_CHECK(s);
-
try_again:
+ TRACE_SEQ_CHECK_RET0(s);
+
len = (s->buffer_size - 1) - s->len;

ret = vsnprintf(s->buffer + s->len, len, fmt, args);
@@ -172,13 +186,15 @@ int trace_seq_puts(struct trace_seq *s, const char *str)
{
int len;

- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET0(s);

len = strlen(str);

while (len > ((s->buffer_size - 1) - s->len))
expand_buffer(s);

+ TRACE_SEQ_CHECK_RET0(s);
+
memcpy(s->buffer + s->len, str, len);
s->len += len;

@@ -187,11 +203,13 @@ int trace_seq_puts(struct trace_seq *s, const char *str)

int trace_seq_putc(struct trace_seq *s, unsigned char c)
{
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET0(s);

while (s->len >= (s->buffer_size - 1))
expand_buffer(s);

+ TRACE_SEQ_CHECK_RET0(s);
+
s->buffer[s->len++] = c;

return 1;
@@ -199,7 +217,7 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)

void trace_seq_terminate(struct trace_seq *s)
{
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET(s);

/* There's always one character left on the buffer */
s->buffer[s->len] = 0;
@@ -208,5 +226,16 @@ void trace_seq_terminate(struct trace_seq *s)
int trace_seq_do_printf(struct trace_seq *s)
{
TRACE_SEQ_CHECK(s);
- return printf("%.*s", s->len, s->buffer);
+
+ switch (s->state) {
+ case TRACE_SEQ__GOOD:
+ return printf("%.*s", s->len, s->buffer);
+ case TRACE_SEQ__BUFFER_POISONED:
+ puts("Usage of trace_seq after it was destroyed");
+ break;
+ case TRACE_SEQ__MEM_ALLOC_FAILED:
+ puts("Can't allocate trace_seq buffer memory");
+ break;
+ }
+ return -1;
}
--
1.7.11.7

2014-01-15 02:57:00

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 01/17] tools lib traceevent: Add state member to struct trace_seq

On Wed, 15 Jan 2014 11:49:28 +0900
Namhyung Kim <[email protected]> wrote:

> Hi Steve,
>
> On Tue, 14 Jan 2014 21:00:58 -0500, Steven Rostedt wrote:
> > On Wed, 15 Jan 2014 10:45:24 +0900
> > Namhyung Kim <[email protected]> wrote:
> >
> >>
> >> @@ -32,8 +33,9 @@
> >> #define TRACE_SEQ_POISON ((void *)0xdeadbeef)
> >> #define TRACE_SEQ_CHECK(s) \
> >> do { \
> >> - if ((s)->buffer == TRACE_SEQ_POISON) \
> >> - die("Usage of trace_seq after it was destroyed"); \
> >> + if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
> >> + "Usage of trace_seq after it was destroyed")) \
> >> + (s)->state = TRACE_SEQ__BUFFER_POISONED; \
> >> } while (0)
> >>
> >> @@ -189,9 +205,15 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)
> >> {
> >> TRACE_SEQ_CHECK(s);
> >>
> >> + if (s->state != TRACE_SEQ__GOOD)
> >> + return 0;
> >> +
> >
> > Instead of adding all of these, we can extend the macro
> > TRACE_SEQ_CHECK() which does a
> > if (s->state != TRACE_SEQ__GOOD)
> > return;
> >
> > and a TRACE_SEQ_CHECK_RET() that does a return 0;
>
> Oh, it looks better. But I'd like to TRACE_SEQ_CHECK() as is for some
> cases. How about this?
>
>

Looks good to me.

Acked-by: Steven Rostedt <[email protected]>

I'll try to look at the rest of the patches tomorrow.

-- Steve

2014-01-15 05:03:25

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH 01/17] tools lib traceevent: Add state member to struct trace_seq

On Tue, 14 Jan 2014 21:56:55 -0500, Steven Rostedt wrote:
> On Wed, 15 Jan 2014 11:49:28 +0900
> Namhyung Kim <[email protected]> wrote:
>> Oh, it looks better. But I'd like to TRACE_SEQ_CHECK() as is for some
>> cases. How about this?
>
> Looks good to me.
>
> Acked-by: Steven Rostedt <[email protected]>
>
> I'll try to look at the rest of the patches tomorrow.

Thanks, and I found that trace_seq_destroy() should use
TRACE_SEQ_TRACE_RET instead. Here's an updated patch.

Thanks,
Namhyung


>From 539a5dd1cb3ba4cb03c283115a9a84f8e345514e Mon Sep 17 00:00:00 2001
From: Namhyung Kim <[email protected]>
Date: Thu, 19 Dec 2013 18:17:44 +0900
Subject: [PATCH] tools lib traceevent: Add state member to struct trace_seq

The trace_seq->state is for tracking errors during the use of
trace_seq APIs and getting rid of die() in it.

Acked-by: Steven Rostedt <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
tools/lib/traceevent/Makefile | 2 +-
tools/lib/traceevent/event-parse.h | 7 +++++
tools/lib/traceevent/trace-seq.c | 55 +++++++++++++++++++++++++++++---------
3 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/tools/lib/traceevent/Makefile b/tools/lib/traceevent/Makefile
index f778d48ac609..56d52a33a3df 100644
--- a/tools/lib/traceevent/Makefile
+++ b/tools/lib/traceevent/Makefile
@@ -136,7 +136,7 @@ export Q VERBOSE

EVENT_PARSE_VERSION = $(EP_VERSION).$(EP_PATCHLEVEL).$(EP_EXTRAVERSION)

-INCLUDES = -I. $(CONFIG_INCLUDES)
+INCLUDES = -I. -I $(srctree)/../../include $(CONFIG_INCLUDES)

# Set compile option CFLAGS if not set elsewhere
CFLAGS ?= -g -Wall
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index cf5db9013f2c..3c890cb28db7 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -58,6 +58,12 @@ struct pevent_record {
#endif
};

+enum trace_seq_fail {
+ TRACE_SEQ__GOOD,
+ TRACE_SEQ__BUFFER_POISONED,
+ TRACE_SEQ__MEM_ALLOC_FAILED,
+};
+
/*
* Trace sequences are used to allow a function to call several other functions
* to create a string of data to use (up to a max of PAGE_SIZE).
@@ -68,6 +74,7 @@ struct trace_seq {
unsigned int buffer_size;
unsigned int len;
unsigned int readpos;
+ enum trace_seq_fail state;
};

void trace_seq_init(struct trace_seq *s);
diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index d7f2e68bc5b9..f7112138e6af 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -22,6 +22,7 @@
#include <string.h>
#include <stdarg.h>

+#include <asm/bug.h>
#include "event-parse.h"
#include "event-utils.h"

@@ -32,10 +33,21 @@
#define TRACE_SEQ_POISON ((void *)0xdeadbeef)
#define TRACE_SEQ_CHECK(s) \
do { \
- if ((s)->buffer == TRACE_SEQ_POISON) \
- die("Usage of trace_seq after it was destroyed"); \
+ if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
+ "Usage of trace_seq after it was destroyed")) \
+ (s)->state = TRACE_SEQ__BUFFER_POISONED; \
} while (0)

+#define TRACE_SEQ_CHECK_RET_N(s, n) \
+do { \
+ TRACE_SEQ_CHECK(s); \
+ if ((s)->state != TRACE_SEQ__GOOD) \
+ return n; \
+} while (0)
+
+#define TRACE_SEQ_CHECK_RET(s) TRACE_SEQ_CHECK_RET_N(s, )
+#define TRACE_SEQ_CHECK_RET0(s) TRACE_SEQ_CHECK_RET_N(s, 0)
+
/**
* trace_seq_init - initialize the trace_seq structure
* @s: a pointer to the trace_seq structure to initialize
@@ -46,6 +58,7 @@ void trace_seq_init(struct trace_seq *s)
s->readpos = 0;
s->buffer_size = TRACE_SEQ_BUF_SIZE;
s->buffer = malloc_or_die(s->buffer_size);
+ s->state = TRACE_SEQ__GOOD;
}

/**
@@ -71,7 +84,7 @@ void trace_seq_destroy(struct trace_seq *s)
{
if (!s)
return;
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET(s);
free(s->buffer);
s->buffer = TRACE_SEQ_POISON;
}
@@ -80,8 +93,9 @@ static void expand_buffer(struct trace_seq *s)
{
s->buffer_size += TRACE_SEQ_BUF_SIZE;
s->buffer = realloc(s->buffer, s->buffer_size);
- if (!s->buffer)
- die("Can't allocate trace_seq buffer memory");
+ if (WARN_ONCE(!s->buffer,
+ "Can't allocate trace_seq buffer memory"))
+ s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
}

/**
@@ -105,9 +119,9 @@ trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
int len;
int ret;

- TRACE_SEQ_CHECK(s);
-
try_again:
+ TRACE_SEQ_CHECK_RET0(s);
+
len = (s->buffer_size - 1) - s->len;

va_start(ap, fmt);
@@ -141,9 +155,9 @@ trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
int len;
int ret;

- TRACE_SEQ_CHECK(s);
-
try_again:
+ TRACE_SEQ_CHECK_RET0(s);
+
len = (s->buffer_size - 1) - s->len;

ret = vsnprintf(s->buffer + s->len, len, fmt, args);
@@ -172,13 +186,15 @@ int trace_seq_puts(struct trace_seq *s, const char *str)
{
int len;

- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET0(s);

len = strlen(str);

while (len > ((s->buffer_size - 1) - s->len))
expand_buffer(s);

+ TRACE_SEQ_CHECK_RET0(s);
+
memcpy(s->buffer + s->len, str, len);
s->len += len;

@@ -187,11 +203,13 @@ int trace_seq_puts(struct trace_seq *s, const char *str)

int trace_seq_putc(struct trace_seq *s, unsigned char c)
{
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET0(s);

while (s->len >= (s->buffer_size - 1))
expand_buffer(s);

+ TRACE_SEQ_CHECK_RET0(s);
+
s->buffer[s->len++] = c;

return 1;
@@ -199,7 +217,7 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)

void trace_seq_terminate(struct trace_seq *s)
{
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET(s);

/* There's always one character left on the buffer */
s->buffer[s->len] = 0;
@@ -208,5 +226,16 @@ void trace_seq_terminate(struct trace_seq *s)
int trace_seq_do_printf(struct trace_seq *s)
{
TRACE_SEQ_CHECK(s);
- return printf("%.*s", s->len, s->buffer);
+
+ switch (s->state) {
+ case TRACE_SEQ__GOOD:
+ return printf("%.*s", s->len, s->buffer);
+ case TRACE_SEQ__BUFFER_POISONED:
+ puts("Usage of trace_seq after it was destroyed");
+ break;
+ case TRACE_SEQ__MEM_ALLOC_FAILED:
+ puts("Can't allocate trace_seq buffer memory");
+ break;
+ }
+ return -1;
}
--
1.7.11.7

2014-01-15 13:39:48

by Jiri Olsa

[permalink] [raw]
Subject: Re: [PATCHSET 00/17] tools lib traceevent: Plugin unload enhancement

On Wed, Jan 15, 2014 at 10:45:23AM +0900, Namhyung Kim wrote:
> Hello,
>
> When plugins are loaded, they register various event and function
> handlers. But they don't unregister even after the plugins unloaded
> so that events could have refererences to non-existing handlers.
>
> This patchset added relevant unregister functions to handle that.
>
> Note that this is not a problem as of now since all of users unload
> plugins after finishing their access to events. But being a generic
> library, it should be handled properly IMHO.
>
> The patch 1-4 are resend of the previous die removal series so
> independent to this series.
>
> I put the series on the 'libtraceevent/plugin-v1' branch in my tree
>
> git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
>
> Any comments are welcome, thanks
> Namhyung

looks ok,

Reviewed-by: Jiri Olsa <[email protected]>

jirka

2014-01-15 14:21:06

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 01/17] tools lib traceevent: Add state member to struct trace_seq

Em Wed, Jan 15, 2014 at 02:03:20PM +0900, Namhyung Kim escreveu:
> On Tue, 14 Jan 2014 21:56:55 -0500, Steven Rostedt wrote:
> > On Wed, 15 Jan 2014 11:49:28 +0900
> > Namhyung Kim <[email protected]> wrote:
> >> Oh, it looks better. But I'd like to TRACE_SEQ_CHECK() as is for some
> >> cases. How about this?
> >
> > Looks good to me.
> >
> > Acked-by: Steven Rostedt <[email protected]>
> >
> > I'll try to look at the rest of the patches tomorrow.

Hi Steven, can I keep your ackd-by?

> Thanks, and I found that trace_seq_destroy() should use
> TRACE_SEQ_TRACE_RET instead. Here's an updated patch.
>
> Thanks,
> Namhyung
>
>
> From 539a5dd1cb3ba4cb03c283115a9a84f8e345514e Mon Sep 17 00:00:00 2001
> From: Namhyung Kim <[email protected]>
> Date: Thu, 19 Dec 2013 18:17:44 +0900
> Subject: [PATCH] tools lib traceevent: Add state member to struct trace_seq
>
> The trace_seq->state is for tracking errors during the use of
> trace_seq APIs and getting rid of die() in it.
>
> Acked-by: Steven Rostedt <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/lib/traceevent/Makefile | 2 +-
> tools/lib/traceevent/event-parse.h | 7 +++++
> tools/lib/traceevent/trace-seq.c | 55 +++++++++++++++++++++++++++++---------
> 3 files changed, 50 insertions(+), 14 deletions(-)
>
> diff --git a/tools/lib/traceevent/Makefile b/tools/lib/traceevent/Makefile
> index f778d48ac609..56d52a33a3df 100644
> --- a/tools/lib/traceevent/Makefile
> +++ b/tools/lib/traceevent/Makefile
> @@ -136,7 +136,7 @@ export Q VERBOSE
>
> EVENT_PARSE_VERSION = $(EP_VERSION).$(EP_PATCHLEVEL).$(EP_EXTRAVERSION)
>
> -INCLUDES = -I. $(CONFIG_INCLUDES)
> +INCLUDES = -I. -I $(srctree)/../../include $(CONFIG_INCLUDES)
>
> # Set compile option CFLAGS if not set elsewhere
> CFLAGS ?= -g -Wall
> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index cf5db9013f2c..3c890cb28db7 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -58,6 +58,12 @@ struct pevent_record {
> #endif
> };
>
> +enum trace_seq_fail {
> + TRACE_SEQ__GOOD,
> + TRACE_SEQ__BUFFER_POISONED,
> + TRACE_SEQ__MEM_ALLOC_FAILED,
> +};
> +
> /*
> * Trace sequences are used to allow a function to call several other functions
> * to create a string of data to use (up to a max of PAGE_SIZE).
> @@ -68,6 +74,7 @@ struct trace_seq {
> unsigned int buffer_size;
> unsigned int len;
> unsigned int readpos;
> + enum trace_seq_fail state;
> };
>
> void trace_seq_init(struct trace_seq *s);
> diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
> index d7f2e68bc5b9..f7112138e6af 100644
> --- a/tools/lib/traceevent/trace-seq.c
> +++ b/tools/lib/traceevent/trace-seq.c
> @@ -22,6 +22,7 @@
> #include <string.h>
> #include <stdarg.h>
>
> +#include <asm/bug.h>
> #include "event-parse.h"
> #include "event-utils.h"
>
> @@ -32,10 +33,21 @@
> #define TRACE_SEQ_POISON ((void *)0xdeadbeef)
> #define TRACE_SEQ_CHECK(s) \
> do { \
> - if ((s)->buffer == TRACE_SEQ_POISON) \
> - die("Usage of trace_seq after it was destroyed"); \
> + if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
> + "Usage of trace_seq after it was destroyed")) \
> + (s)->state = TRACE_SEQ__BUFFER_POISONED; \
> } while (0)
>
> +#define TRACE_SEQ_CHECK_RET_N(s, n) \
> +do { \
> + TRACE_SEQ_CHECK(s); \
> + if ((s)->state != TRACE_SEQ__GOOD) \
> + return n; \
> +} while (0)
> +
> +#define TRACE_SEQ_CHECK_RET(s) TRACE_SEQ_CHECK_RET_N(s, )
> +#define TRACE_SEQ_CHECK_RET0(s) TRACE_SEQ_CHECK_RET_N(s, 0)
> +
> /**
> * trace_seq_init - initialize the trace_seq structure
> * @s: a pointer to the trace_seq structure to initialize
> @@ -46,6 +58,7 @@ void trace_seq_init(struct trace_seq *s)
> s->readpos = 0;
> s->buffer_size = TRACE_SEQ_BUF_SIZE;
> s->buffer = malloc_or_die(s->buffer_size);
> + s->state = TRACE_SEQ__GOOD;
> }
>
> /**
> @@ -71,7 +84,7 @@ void trace_seq_destroy(struct trace_seq *s)
> {
> if (!s)
> return;
> - TRACE_SEQ_CHECK(s);
> + TRACE_SEQ_CHECK_RET(s);
> free(s->buffer);
> s->buffer = TRACE_SEQ_POISON;
> }
> @@ -80,8 +93,9 @@ static void expand_buffer(struct trace_seq *s)
> {
> s->buffer_size += TRACE_SEQ_BUF_SIZE;
> s->buffer = realloc(s->buffer, s->buffer_size);
> - if (!s->buffer)
> - die("Can't allocate trace_seq buffer memory");
> + if (WARN_ONCE(!s->buffer,
> + "Can't allocate trace_seq buffer memory"))
> + s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
> }
>
> /**
> @@ -105,9 +119,9 @@ trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
> int len;
> int ret;
>
> - TRACE_SEQ_CHECK(s);
> -
> try_again:
> + TRACE_SEQ_CHECK_RET0(s);
> +
> len = (s->buffer_size - 1) - s->len;
>
> va_start(ap, fmt);
> @@ -141,9 +155,9 @@ trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
> int len;
> int ret;
>
> - TRACE_SEQ_CHECK(s);
> -
> try_again:
> + TRACE_SEQ_CHECK_RET0(s);
> +
> len = (s->buffer_size - 1) - s->len;
>
> ret = vsnprintf(s->buffer + s->len, len, fmt, args);
> @@ -172,13 +186,15 @@ int trace_seq_puts(struct trace_seq *s, const char *str)
> {
> int len;
>
> - TRACE_SEQ_CHECK(s);
> + TRACE_SEQ_CHECK_RET0(s);
>
> len = strlen(str);
>
> while (len > ((s->buffer_size - 1) - s->len))
> expand_buffer(s);
>
> + TRACE_SEQ_CHECK_RET0(s);
> +
> memcpy(s->buffer + s->len, str, len);
> s->len += len;
>
> @@ -187,11 +203,13 @@ int trace_seq_puts(struct trace_seq *s, const char *str)
>
> int trace_seq_putc(struct trace_seq *s, unsigned char c)
> {
> - TRACE_SEQ_CHECK(s);
> + TRACE_SEQ_CHECK_RET0(s);
>
> while (s->len >= (s->buffer_size - 1))
> expand_buffer(s);
>
> + TRACE_SEQ_CHECK_RET0(s);
> +
> s->buffer[s->len++] = c;
>
> return 1;
> @@ -199,7 +217,7 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)
>
> void trace_seq_terminate(struct trace_seq *s)
> {
> - TRACE_SEQ_CHECK(s);
> + TRACE_SEQ_CHECK_RET(s);
>
> /* There's always one character left on the buffer */
> s->buffer[s->len] = 0;
> @@ -208,5 +226,16 @@ void trace_seq_terminate(struct trace_seq *s)
> int trace_seq_do_printf(struct trace_seq *s)
> {
> TRACE_SEQ_CHECK(s);
> - return printf("%.*s", s->len, s->buffer);
> +
> + switch (s->state) {
> + case TRACE_SEQ__GOOD:
> + return printf("%.*s", s->len, s->buffer);
> + case TRACE_SEQ__BUFFER_POISONED:
> + puts("Usage of trace_seq after it was destroyed");
> + break;
> + case TRACE_SEQ__MEM_ALLOC_FAILED:
> + puts("Can't allocate trace_seq buffer memory");
> + break;
> + }
> + return -1;
> }
> --
> 1.7.11.7

2014-01-15 14:25:35

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 01/17] tools lib traceevent: Add state member to struct trace_seq

On Wed, 15 Jan 2014 11:15:19 -0300
Arnaldo Carvalho de Melo <[email protected]> wrote:

> Em Wed, Jan 15, 2014 at 02:03:20PM +0900, Namhyung Kim escreveu:
> > On Tue, 14 Jan 2014 21:56:55 -0500, Steven Rostedt wrote:
> > > On Wed, 15 Jan 2014 11:49:28 +0900
> > > Namhyung Kim <[email protected]> wrote:
> > >> Oh, it looks better. But I'd like to TRACE_SEQ_CHECK() as is for some
> > >> cases. How about this?
> > >
> > > Looks good to me.
> > >
> > > Acked-by: Steven Rostedt <[email protected]>
> > >
> > > I'll try to look at the rest of the patches tomorrow.
>
> Hi Steven, can I keep your ackd-by?
>

Yep.

-- Steve

2014-01-15 16:18:43

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 06/17] tools lib traceevent: Add pevent_unregister_event_handler()

On Wed, 15 Jan 2014 10:45:29 +0900
Namhyung Kim <[email protected]> wrote:

> pr_stat("overriding event (%d) %s:%s with new print handler",
> event->id, event->system, event->name);
> @@ -5637,6 +5649,79 @@ int pevent_register_event_handler(struct pevent *pevent, int id,
> return -1;
> }
>
> +static int handle_matches(struct event_handler *handler, int id,
> + const char *sys_name, const char *event_name,
> + pevent_event_handler_func func, void *context)
> +{
> + if (id >= 0 && id != handler->id)
> + return 0;
> +
> + if (event_name && (strcmp(event_name, handler->event_name) != 0))
> + return 0;
> +
> + if (sys_name && (strcmp(sys_name, handler->sys_name) != 0))
> + return 0;
> +
> + if (func != handler->func || context != handler->context)
> + return 0;
> +
> + return 1;
> +}
> +
> +/**
> + * pevent_unregister_event_handler - unregister an existing event handler
> + * @pevent: the handle to the pevent
> + * @id: the id of the event to unregister
> + * @sys_name: the system name the handler belongs to
> + * @event_name: the name of the event handler
> + * @func: the function to call to parse the event information
> + * @context: the data to be passed to @func
> + *
> + * This function removes existing event handler (parser).
> + *
> + * If @id is >= 0, then it is used to find the event.
> + * else @sys_name and @event_name are used.
> + *
> + * Returns 0 if handler was removed successfully, -1 if event was not found.
> + */
> +int pevent_unregister_event_handler(struct pevent *pevent, int id,
> + const char *sys_name, const char *event_name,
> + pevent_event_handler_func func, void *context)
> +{
> + struct event_format *event;
> + struct event_handler *handle;
> + struct event_handler **next;
> +
> + event = pevent_search_event(pevent, id, sys_name, event_name);
> + if (event == NULL)
> + goto not_found;
> +
> + if (event->handler == func && event->context == context) {
> + pr_stat("overriding event (%d) %s:%s with default print handler",

Should we use the word "overriding" again? Perhaps saying:

"removing override print handler (%d) %s:%s. Going back to default handler."

Or something?

-- Steve

> + event->id, event->system, event->name);
> +
> + event->handler = NULL;
> + event->context = NULL;
> + return 0;
> + }
> +
> +not_found:
> + for (next = &pevent->handlers; *next; next = &(*next)->next) {
> + handle = *next;
> + if (handle_matches(handle, id, sys_name, event_name,
> + func, context))
> + break;
> + }
> +
> + if (!(*next))
> + return -1;
> +
> + *next = handle->next;
> + free_handler(handle);
> +
> + return 0;
> +}
> +

2014-01-15 16:20:02

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 07/17] tools lib traceevent: Add pevent_unregister_print_function()

On Wed, 15 Jan 2014 10:45:30 +0900
Namhyung Kim <[email protected]> wrote:

> When a plugin unloaded it needs to unregister its print handler from
> pevent. So add unregister function to do it.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---

Acked-by: Steven Rostedt <[email protected]>

-- Steve

2014-01-15 16:21:20

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 08/17] tools lib traceevent: Unregister handler when function plugin unloaded

On Wed, 15 Jan 2014 10:45:31 +0900
Namhyung Kim <[email protected]> wrote:

> The function handler should be unregistered when the plugin unloaded
> otherwise it'll try to access invalid memory.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>

Acked-by: Steven Rostedt <[email protected]>

-- Steve


> ---
> tools/lib/traceevent/plugin_function.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
> index 39461485f9a7..80ba4ff1fe84 100644
> --- a/tools/lib/traceevent/plugin_function.c
> +++ b/tools/lib/traceevent/plugin_function.c
> @@ -148,6 +148,9 @@ void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
> {
> int i, x;
>
> + pevent_unregister_event_handler(pevent, -1, "ftrace", "function",
> + function_handler, NULL);
> +
> for (i = 0; i <= cpus; i++) {
> for (x = 0; x < fstack[i].size && fstack[i].stack[x]; x++)
> free(fstack[i].stack[x]);

2014-01-15 16:22:13

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 09/17] tools lib traceevent: Unregister handler when hrtimer plugin unloaded

On Wed, 15 Jan 2014 10:45:32 +0900
Namhyung Kim <[email protected]> wrote:

> The timer handlers should be unregistered when the plugin unloaded
> otherwise they'll try to access invalid memory.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>

Acked-by: Steven Rostedt <[email protected]>

-- Steve

2014-01-15 18:12:26

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 06/17] tools lib traceevent: Add pevent_unregister_event_handler()

Em Wed, Jan 15, 2014 at 11:18:39AM -0500, Steven Rostedt escreveu:
> On Wed, 15 Jan 2014 10:45:29 +0900
> Namhyung Kim <[email protected]> wrote:
>
> > pr_stat("overriding event (%d) %s:%s with new print handler",
> > event->id, event->system, event->name);
> > @@ -5637,6 +5649,79 @@ int pevent_register_event_handler(struct pevent *pevent, int id,
> > return -1;
> > }
> >
> > +static int handle_matches(struct event_handler *handler, int id,
> > + const char *sys_name, const char *event_name,
> > + pevent_event_handler_func func, void *context)
> > +{
> > + if (id >= 0 && id != handler->id)
> > + return 0;
> > +
> > + if (event_name && (strcmp(event_name, handler->event_name) != 0))
> > + return 0;
> > +
> > + if (sys_name && (strcmp(sys_name, handler->sys_name) != 0))
> > + return 0;
> > +
> > + if (func != handler->func || context != handler->context)
> > + return 0;
> > +
> > + return 1;
> > +}
> > +
> > +/**
> > + * pevent_unregister_event_handler - unregister an existing event handler
> > + * @pevent: the handle to the pevent
> > + * @id: the id of the event to unregister
> > + * @sys_name: the system name the handler belongs to
> > + * @event_name: the name of the event handler
> > + * @func: the function to call to parse the event information
> > + * @context: the data to be passed to @func
> > + *
> > + * This function removes existing event handler (parser).
> > + *
> > + * If @id is >= 0, then it is used to find the event.
> > + * else @sys_name and @event_name are used.
> > + *
> > + * Returns 0 if handler was removed successfully, -1 if event was not found.
> > + */
> > +int pevent_unregister_event_handler(struct pevent *pevent, int id,
> > + const char *sys_name, const char *event_name,
> > + pevent_event_handler_func func, void *context)
> > +{
> > + struct event_format *event;
> > + struct event_handler *handle;
> > + struct event_handler **next;
> > +
> > + event = pevent_search_event(pevent, id, sys_name, event_name);
> > + if (event == NULL)
> > + goto not_found;
> > +
> > + if (event->handler == func && event->context == context) {
> > + pr_stat("overriding event (%d) %s:%s with default print handler",
>
> Should we use the word "overriding" again? Perhaps saying:
>
> "removing override print handler (%d) %s:%s. Going back to default handler."
>
> Or something?

Applied up to the patch just before this one, waiting for a resolution.

> -- Steve
>
> > + event->id, event->system, event->name);
> > +
> > + event->handler = NULL;
> > + event->context = NULL;
> > + return 0;
> > + }
> > +
> > +not_found:
> > + for (next = &pevent->handlers; *next; next = &(*next)->next) {
> > + handle = *next;
> > + if (handle_matches(handle, id, sys_name, event_name,
> > + func, context))
> > + break;
> > + }
> > +
> > + if (!(*next))
> > + return -1;
> > +
> > + *next = handle->next;
> > + free_handler(handle);
> > +
> > + return 0;
> > +}
> > +

2014-01-15 20:33:01

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 13/17] tools lib traceevent: Unregister handler when mac80211 plugin unloaded

On Wed, 15 Jan 2014 10:45:36 +0900
Namhyung Kim <[email protected]> wrote:

> The event handler should be unregistered when the plugin unloaded
> otherwise it'll try to access invalid memory.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>

For PATCH 10-13,

Acked-by: Steven Rostedt <[email protected]>

-- Steve

2014-01-15 20:34:46

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 14/17] tools lib traceevent: Unregister handler when cfg80211 plugin unloaded

On Wed, 15 Jan 2014 10:45:37 +0900
Namhyung Kim <[email protected]> wrote:

> The function handler should be unregistered when the plugin unloaded
> otherwise it'll try to access invalid memory.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/lib/traceevent/plugin_cfg80211.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tools/lib/traceevent/plugin_cfg80211.c b/tools/lib/traceevent/plugin_cfg80211.c
> index dcab8e873c21..7b07d149557b 100644
> --- a/tools/lib/traceevent/plugin_cfg80211.c
> +++ b/tools/lib/traceevent/plugin_cfg80211.c
> @@ -22,3 +22,10 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
> PEVENT_FUNC_ARG_VOID);
> return 0;
> }
> +
> +void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
> +{
> + pevent_unregister_print_function(pevent,
> + process___le16_to_cpup,
> + "__le16_to_cpup");
> +}

Can we format this a little nicer:

pevent_unregister_print_function(pevent, process___le16_to_cpup,
"__le16_to_cpup");

I know the register had that separated, but that's because it had args
too, and it made it look nicer. But this don't work here. It looks odd.

-- Steve

2014-01-15 20:36:05

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 15/17] tools lib traceevent: Unregister handler when jbd2 plugin unloaded

On Wed, 15 Jan 2014 10:45:38 +0900
Namhyung Kim <[email protected]> wrote:

> The function handlers should be unregistered when the plugin unloaded
> otherwise they'll try to access invalid memory.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/lib/traceevent/plugin_jbd2.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/tools/lib/traceevent/plugin_jbd2.c b/tools/lib/traceevent/plugin_jbd2.c
> index 2f93f81f0bac..88fe6fe872d5 100644
> --- a/tools/lib/traceevent/plugin_jbd2.c
> +++ b/tools/lib/traceevent/plugin_jbd2.c
> @@ -66,3 +66,14 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
> PEVENT_FUNC_ARG_VOID);
> return 0;
> }
> +
> +void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
> +{
> + pevent_unregister_print_function(pevent,
> + process_jbd2_dev_to_name,
> + "jbd2_dev_to_name");
> +
> + pevent_unregister_print_function(pevent,
> + process_jiffies_to_msecs,
> + "jiffies_to_msecs");
> +}

Same here. I don't know about others, but I think it looks a bit nicer
if we move the function up to the first line.

-- Steve

2014-01-15 20:36:30

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 16/17] tools lib traceevent: Unregister handler when scsi plugin unloaded

On Wed, 15 Jan 2014 10:45:39 +0900
Namhyung Kim <[email protected]> wrote:

> The function handler should be unregistered when the plugin unloaded
> otherwise it'll try to access invalid memory.
>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> tools/lib/traceevent/plugin_scsi.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tools/lib/traceevent/plugin_scsi.c b/tools/lib/traceevent/plugin_scsi.c
> index 6fb8e3e3fcad..64d688534518 100644
> --- a/tools/lib/traceevent/plugin_scsi.c
> +++ b/tools/lib/traceevent/plugin_scsi.c
> @@ -421,3 +421,10 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
> PEVENT_FUNC_ARG_VOID);
> return 0;
> }
> +
> +void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
> +{
> + pevent_unregister_print_function(pevent,
> + process_scsi_trace_parse_cdb,
> + "scsi_trace_parse_cdb");
> +}

Seems we have a trend ;-)

Same for patch 17.

-- Steve

2014-01-16 00:00:32

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH 06/17] tools lib traceevent: Add pevent_unregister_event_handler()

Hi Steve,

2014-01-16 AM 1:18, Steven Rostedt wrote:
> On Wed, 15 Jan 2014 10:45:29 +0900
> Namhyung Kim <[email protected]> wrote:
>> + if (event->handler == func && event->context == context) {
>> + pr_stat("overriding event (%d) %s:%s with default print handler",
>
> Should we use the word "overriding" again? Perhaps saying:
>
> "removing override print handler (%d) %s:%s. Going back to default handler."
>
> Or something?

What about this:

"removing override handler for event (%d) %s:%s. Going back to default
handler."

Thanks,
Namhyung

2014-01-16 01:53:08

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 06/17] tools lib traceevent: Add pevent_unregister_event_handler()

On Thu, 16 Jan 2014 09:00:28 +0900
Namhyung Kim <[email protected]> wrote:

> Hi Steve,
>
> 2014-01-16 AM 1:18, Steven Rostedt wrote:
> > On Wed, 15 Jan 2014 10:45:29 +0900
> > Namhyung Kim <[email protected]> wrote:
> >> + if (event->handler == func && event->context == context) {
> >> + pr_stat("overriding event (%d) %s:%s with default print handler",
> >
> > Should we use the word "overriding" again? Perhaps saying:
> >
> > "removing override print handler (%d) %s:%s. Going back to default handler."
> >
> > Or something?
>
> What about this:
>
> "removing override handler for event (%d) %s:%s. Going back to default
> handler."
>

Sounds good.

-- Steve

2014-01-16 01:58:13

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH 06/17] tools lib traceevent: Add pevent_unregister_event_handler()

On Wed, 15 Jan 2014 20:52:59 -0500, Steven Rostedt wrote:
> On Thu, 16 Jan 2014 09:00:28 +0900
> Namhyung Kim <[email protected]> wrote:
>
>> Hi Steve,
>>
>> 2014-01-16 AM 1:18, Steven Rostedt wrote:
>> > On Wed, 15 Jan 2014 10:45:29 +0900
>> > Namhyung Kim <[email protected]> wrote:
>> >> + if (event->handler == func && event->context == context) {
>> >> + pr_stat("overriding event (%d) %s:%s with default print handler",
>> >
>> > Should we use the word "overriding" again? Perhaps saying:
>> >
>> > "removing override print handler (%d) %s:%s. Going back to default handler."
>> >
>> > Or something?
>>
>> What about this:
>>
>> "removing override handler for event (%d) %s:%s. Going back to default
>> handler."
>>
>
> Sounds good.

Thanks, can I add your ack with this change?

Namhyung

2014-01-16 02:05:34

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 06/17] tools lib traceevent: Add pevent_unregister_event_handler()

On Thu, 16 Jan 2014 10:58:05 +0900
Namhyung Kim <[email protected]> wrote:

> Thanks, can I add your ack with this change?

Yeah, go ahead.

-- Steve

Subject: [tip:perf/core] tools lib traceevent: Check return value of realloc()

Commit-ID: 3026bba3c37711234771349ca020d9a85e572f60
Gitweb: http://git.kernel.org/tip/3026bba3c37711234771349ca020d9a85e572f60
Author: Namhyung Kim <[email protected]>
AuthorDate: Wed, 15 Jan 2014 10:45:25 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 15 Jan 2014 15:10:28 -0300

tools lib traceevent: Check return value of realloc()

If realloc() fails, it'll leak the buffer. Also increate buffer size
only if the allocation succeeded.

Signed-off-by: Namhyung Kim <[email protected]>
Reviewed-by: Jiri Olsa <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/lib/traceevent/trace-seq.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index f711213..e454a2c 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -91,11 +91,16 @@ void trace_seq_destroy(struct trace_seq *s)

static void expand_buffer(struct trace_seq *s)
{
- s->buffer_size += TRACE_SEQ_BUF_SIZE;
- s->buffer = realloc(s->buffer, s->buffer_size);
- if (WARN_ONCE(!s->buffer,
- "Can't allocate trace_seq buffer memory"))
+ char *buf;
+
+ buf = realloc(s->buffer, s->buffer_size + TRACE_SEQ_BUF_SIZE);
+ if (WARN_ONCE(!buf, "Can't allocate trace_seq buffer memory")) {
s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
+ return;
+ }
+
+ s->buffer = buf;
+ s->buffer_size += TRACE_SEQ_BUF_SIZE;
}

/**

Subject: [tip:perf/core] tools lib traceevent: Get rid of die() finally!!

Commit-ID: e825e756f84eab0e68d7d6644c018c3412748406
Gitweb: http://git.kernel.org/tip/e825e756f84eab0e68d7d6644c018c3412748406
Author: Namhyung Kim <[email protected]>
AuthorDate: Wed, 15 Jan 2014 10:45:27 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 15 Jan 2014 15:10:36 -0300

tools lib traceevent: Get rid of die() finally!!

Now all of its users were gone. :)

Signed-off-by: Namhyung Kim <[email protected]>
Reviewed-by: Jiri Olsa <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/lib/traceevent/event-utils.h | 4 ----
tools/lib/traceevent/parse-utils.c | 44 --------------------------------------
2 files changed, 48 deletions(-)

diff --git a/tools/lib/traceevent/event-utils.h b/tools/lib/traceevent/event-utils.h
index e76c9ac..d1dc217 100644
--- a/tools/lib/traceevent/event-utils.h
+++ b/tools/lib/traceevent/event-utils.h
@@ -23,18 +23,14 @@
#include <ctype.h>

/* Can be overridden */
-void die(const char *fmt, ...);
-void *malloc_or_die(unsigned int size);
void warning(const char *fmt, ...);
void pr_stat(const char *fmt, ...);
void vpr_stat(const char *fmt, va_list ap);

/* Always available */
-void __die(const char *fmt, ...);
void __warning(const char *fmt, ...);
void __pr_stat(const char *fmt, ...);

-void __vdie(const char *fmt, ...);
void __vwarning(const char *fmt, ...);
void __vpr_stat(const char *fmt, ...);

diff --git a/tools/lib/traceevent/parse-utils.c b/tools/lib/traceevent/parse-utils.c
index bba701c..eda07fa 100644
--- a/tools/lib/traceevent/parse-utils.c
+++ b/tools/lib/traceevent/parse-utils.c
@@ -25,40 +25,6 @@

#define __weak __attribute__((weak))

-void __vdie(const char *fmt, va_list ap)
-{
- int ret = errno;
-
- if (errno)
- perror("trace-cmd");
- else
- ret = -1;
-
- fprintf(stderr, " ");
- vfprintf(stderr, fmt, ap);
-
- fprintf(stderr, "\n");
- exit(ret);
-}
-
-void __die(const char *fmt, ...)
-{
- va_list ap;
-
- va_start(ap, fmt);
- __vdie(fmt, ap);
- va_end(ap);
-}
-
-void __weak die(const char *fmt, ...)
-{
- va_list ap;
-
- va_start(ap, fmt);
- __vdie(fmt, ap);
- va_end(ap);
-}
-
void __vwarning(const char *fmt, va_list ap)
{
if (errno)
@@ -117,13 +83,3 @@ void __weak pr_stat(const char *fmt, ...)
__vpr_stat(fmt, ap);
va_end(ap);
}
-
-void __weak *malloc_or_die(unsigned int size)
-{
- void *data;
-
- data = malloc(size);
- if (!data)
- die("malloc");
- return data;
-}

Subject: [tip:perf/core] tools lib traceevent: Make plugin unload function receive pevent

Commit-ID: 8d0c2224ca6e04ba51c403805e7e1e2ca536520b
Gitweb: http://git.kernel.org/tip/8d0c2224ca6e04ba51c403805e7e1e2ca536520b
Author: Namhyung Kim <[email protected]>
AuthorDate: Wed, 15 Jan 2014 10:45:28 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 15 Jan 2014 15:10:40 -0300

tools lib traceevent: Make plugin unload function receive pevent

The PEVENT_PLUGIN_UNLOADER function might need some cleanup using pevent
like unregister some handlers. So pass pevent as argument.

Signed-off-by: Namhyung Kim <[email protected]>
Reviewed-by: Jiri Olsa <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/lib/traceevent/event-parse.h | 7 ++++---
tools/lib/traceevent/event-plugin.c | 4 ++--
tools/lib/traceevent/plugin_function.c | 2 +-
tools/perf/util/trace-event.c | 2 +-
4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 3c890cb..a3beca5 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -105,7 +105,7 @@ typedef int (*pevent_event_handler_func)(struct trace_seq *s,
void *context);

typedef int (*pevent_plugin_load_func)(struct pevent *pevent);
-typedef int (*pevent_plugin_unload_func)(void);
+typedef int (*pevent_plugin_unload_func)(struct pevent *pevent);

struct plugin_option {
struct plugin_option *next;
@@ -130,7 +130,7 @@ struct plugin_option {
* PEVENT_PLUGIN_UNLOADER: (optional)
* The function called just before unloading
*
- * int PEVENT_PLUGIN_UNLOADER(void)
+ * int PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
*
* PEVENT_PLUGIN_OPTIONS: (optional)
* Plugin options that can be set before loading
@@ -411,7 +411,8 @@ enum pevent_errno {
struct plugin_list;

struct plugin_list *traceevent_load_plugins(struct pevent *pevent);
-void traceevent_unload_plugins(struct plugin_list *plugin_list);
+void traceevent_unload_plugins(struct plugin_list *plugin_list,
+ struct pevent *pevent);

struct cmdline;
struct cmdline_list;
diff --git a/tools/lib/traceevent/event-plugin.c b/tools/lib/traceevent/event-plugin.c
index 125f567..0c8bf67 100644
--- a/tools/lib/traceevent/event-plugin.c
+++ b/tools/lib/traceevent/event-plugin.c
@@ -197,7 +197,7 @@ traceevent_load_plugins(struct pevent *pevent)
}

void
-traceevent_unload_plugins(struct plugin_list *plugin_list)
+traceevent_unload_plugins(struct plugin_list *plugin_list, struct pevent *pevent)
{
pevent_plugin_unload_func func;
struct plugin_list *list;
@@ -207,7 +207,7 @@ traceevent_unload_plugins(struct plugin_list *plugin_list)
plugin_list = list->next;
func = dlsym(list->handle, PEVENT_PLUGIN_UNLOADER_NAME);
if (func)
- func();
+ func(pevent);
dlclose(list->handle);
free(list->name);
free(list);
diff --git a/tools/lib/traceevent/plugin_function.c b/tools/lib/traceevent/plugin_function.c
index aad92ad..3946148 100644
--- a/tools/lib/traceevent/plugin_function.c
+++ b/tools/lib/traceevent/plugin_function.c
@@ -144,7 +144,7 @@ int PEVENT_PLUGIN_LOADER(struct pevent *pevent)
return 0;
}

-void PEVENT_PLUGIN_UNLOADER(void)
+void PEVENT_PLUGIN_UNLOADER(struct pevent *pevent)
{
int i, x;

diff --git a/tools/perf/util/trace-event.c b/tools/perf/util/trace-event.c
index d9f5f61..6322d37 100644
--- a/tools/perf/util/trace-event.c
+++ b/tools/perf/util/trace-event.c
@@ -34,8 +34,8 @@ int trace_event__init(struct trace_event *t)

void trace_event__cleanup(struct trace_event *t)
{
+ traceevent_unload_plugins(t->plugin_list, t->pevent);
pevent_free(t->pevent);
- traceevent_unload_plugins(t->plugin_list);
}

static struct event_format*

Subject: [tip:perf/core] tools lib traceevent: Get rid of malloc_or_die() in trace_seq_init()

Commit-ID: 504586e0954bcf9550dfdea37d3234174ed1d68f
Gitweb: http://git.kernel.org/tip/504586e0954bcf9550dfdea37d3234174ed1d68f
Author: Namhyung Kim <[email protected]>
AuthorDate: Wed, 15 Jan 2014 10:45:26 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 15 Jan 2014 15:10:32 -0300

tools lib traceevent: Get rid of malloc_or_die() in trace_seq_init()

Use plain malloc() and check its return value.

Signed-off-by: Namhyung Kim <[email protected]>
Reviewed-by: Jiri Olsa <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/lib/traceevent/trace-seq.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index e454a2c..ec3bd16 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -57,8 +57,11 @@ void trace_seq_init(struct trace_seq *s)
s->len = 0;
s->readpos = 0;
s->buffer_size = TRACE_SEQ_BUF_SIZE;
- s->buffer = malloc_or_die(s->buffer_size);
- s->state = TRACE_SEQ__GOOD;
+ s->buffer = malloc(s->buffer_size);
+ if (s->buffer != NULL)
+ s->state = TRACE_SEQ__GOOD;
+ else
+ s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
}

/**

Subject: [tip:perf/core] tools lib traceevent: Add state member to struct trace_seq

Commit-ID: 3c6d8d84423932f1d9949179c6acdf2405515ee4
Gitweb: http://git.kernel.org/tip/3c6d8d84423932f1d9949179c6acdf2405515ee4
Author: Namhyung Kim <[email protected]>
AuthorDate: Thu, 19 Dec 2013 18:17:44 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 15 Jan 2014 15:10:19 -0300

tools lib traceevent: Add state member to struct trace_seq

The trace_seq->state is for tracking errors during the use of trace_seq
APIs and getting rid of die() in it.

Signed-off-by: Namhyung Kim <[email protected]>
Reviewed-by: Jiri Olsa <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
Cc: Frederic Weisbecker <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/lib/traceevent/Makefile | 2 +-
tools/lib/traceevent/event-parse.h | 7 +++++
tools/lib/traceevent/trace-seq.c | 55 +++++++++++++++++++++++++++++---------
3 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/tools/lib/traceevent/Makefile b/tools/lib/traceevent/Makefile
index f778d48..56d52a3 100644
--- a/tools/lib/traceevent/Makefile
+++ b/tools/lib/traceevent/Makefile
@@ -136,7 +136,7 @@ export Q VERBOSE

EVENT_PARSE_VERSION = $(EP_VERSION).$(EP_PATCHLEVEL).$(EP_EXTRAVERSION)

-INCLUDES = -I. $(CONFIG_INCLUDES)
+INCLUDES = -I. -I $(srctree)/../../include $(CONFIG_INCLUDES)

# Set compile option CFLAGS if not set elsewhere
CFLAGS ?= -g -Wall
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index cf5db90..3c890cb 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -58,6 +58,12 @@ struct pevent_record {
#endif
};

+enum trace_seq_fail {
+ TRACE_SEQ__GOOD,
+ TRACE_SEQ__BUFFER_POISONED,
+ TRACE_SEQ__MEM_ALLOC_FAILED,
+};
+
/*
* Trace sequences are used to allow a function to call several other functions
* to create a string of data to use (up to a max of PAGE_SIZE).
@@ -68,6 +74,7 @@ struct trace_seq {
unsigned int buffer_size;
unsigned int len;
unsigned int readpos;
+ enum trace_seq_fail state;
};

void trace_seq_init(struct trace_seq *s);
diff --git a/tools/lib/traceevent/trace-seq.c b/tools/lib/traceevent/trace-seq.c
index d7f2e68..f711213 100644
--- a/tools/lib/traceevent/trace-seq.c
+++ b/tools/lib/traceevent/trace-seq.c
@@ -22,6 +22,7 @@
#include <string.h>
#include <stdarg.h>

+#include <asm/bug.h>
#include "event-parse.h"
#include "event-utils.h"

@@ -32,10 +33,21 @@
#define TRACE_SEQ_POISON ((void *)0xdeadbeef)
#define TRACE_SEQ_CHECK(s) \
do { \
- if ((s)->buffer == TRACE_SEQ_POISON) \
- die("Usage of trace_seq after it was destroyed"); \
+ if (WARN_ONCE((s)->buffer == TRACE_SEQ_POISON, \
+ "Usage of trace_seq after it was destroyed")) \
+ (s)->state = TRACE_SEQ__BUFFER_POISONED; \
} while (0)

+#define TRACE_SEQ_CHECK_RET_N(s, n) \
+do { \
+ TRACE_SEQ_CHECK(s); \
+ if ((s)->state != TRACE_SEQ__GOOD) \
+ return n; \
+} while (0)
+
+#define TRACE_SEQ_CHECK_RET(s) TRACE_SEQ_CHECK_RET_N(s, )
+#define TRACE_SEQ_CHECK_RET0(s) TRACE_SEQ_CHECK_RET_N(s, 0)
+
/**
* trace_seq_init - initialize the trace_seq structure
* @s: a pointer to the trace_seq structure to initialize
@@ -46,6 +58,7 @@ void trace_seq_init(struct trace_seq *s)
s->readpos = 0;
s->buffer_size = TRACE_SEQ_BUF_SIZE;
s->buffer = malloc_or_die(s->buffer_size);
+ s->state = TRACE_SEQ__GOOD;
}

/**
@@ -71,7 +84,7 @@ void trace_seq_destroy(struct trace_seq *s)
{
if (!s)
return;
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET(s);
free(s->buffer);
s->buffer = TRACE_SEQ_POISON;
}
@@ -80,8 +93,9 @@ static void expand_buffer(struct trace_seq *s)
{
s->buffer_size += TRACE_SEQ_BUF_SIZE;
s->buffer = realloc(s->buffer, s->buffer_size);
- if (!s->buffer)
- die("Can't allocate trace_seq buffer memory");
+ if (WARN_ONCE(!s->buffer,
+ "Can't allocate trace_seq buffer memory"))
+ s->state = TRACE_SEQ__MEM_ALLOC_FAILED;
}

/**
@@ -105,9 +119,9 @@ trace_seq_printf(struct trace_seq *s, const char *fmt, ...)
int len;
int ret;

- TRACE_SEQ_CHECK(s);
-
try_again:
+ TRACE_SEQ_CHECK_RET0(s);
+
len = (s->buffer_size - 1) - s->len;

va_start(ap, fmt);
@@ -141,9 +155,9 @@ trace_seq_vprintf(struct trace_seq *s, const char *fmt, va_list args)
int len;
int ret;

- TRACE_SEQ_CHECK(s);
-
try_again:
+ TRACE_SEQ_CHECK_RET0(s);
+
len = (s->buffer_size - 1) - s->len;

ret = vsnprintf(s->buffer + s->len, len, fmt, args);
@@ -172,13 +186,15 @@ int trace_seq_puts(struct trace_seq *s, const char *str)
{
int len;

- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET0(s);

len = strlen(str);

while (len > ((s->buffer_size - 1) - s->len))
expand_buffer(s);

+ TRACE_SEQ_CHECK_RET0(s);
+
memcpy(s->buffer + s->len, str, len);
s->len += len;

@@ -187,11 +203,13 @@ int trace_seq_puts(struct trace_seq *s, const char *str)

int trace_seq_putc(struct trace_seq *s, unsigned char c)
{
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET0(s);

while (s->len >= (s->buffer_size - 1))
expand_buffer(s);

+ TRACE_SEQ_CHECK_RET0(s);
+
s->buffer[s->len++] = c;

return 1;
@@ -199,7 +217,7 @@ int trace_seq_putc(struct trace_seq *s, unsigned char c)

void trace_seq_terminate(struct trace_seq *s)
{
- TRACE_SEQ_CHECK(s);
+ TRACE_SEQ_CHECK_RET(s);

/* There's always one character left on the buffer */
s->buffer[s->len] = 0;
@@ -208,5 +226,16 @@ void trace_seq_terminate(struct trace_seq *s)
int trace_seq_do_printf(struct trace_seq *s)
{
TRACE_SEQ_CHECK(s);
- return printf("%.*s", s->len, s->buffer);
+
+ switch (s->state) {
+ case TRACE_SEQ__GOOD:
+ return printf("%.*s", s->len, s->buffer);
+ case TRACE_SEQ__BUFFER_POISONED:
+ puts("Usage of trace_seq after it was destroyed");
+ break;
+ case TRACE_SEQ__MEM_ALLOC_FAILED:
+ puts("Can't allocate trace_seq buffer memory");
+ break;
+ }
+ return -1;
}