[PATCH 0/3] Few patches, related to libtracevent APIs

[PATCH 0/3] Few patches, related to libtracevent APIs

[Tzvetomir Stoyanov]

This patchset introduces new libtracevent APIs, which can be used for
accessing struct tep_handler fields. It also changes the description
of few APIs, to match more closely their logic.
There are also fixes to coding style problems, introduced by previous patches.

Tzvetomir Stoyanov (3):
  tools/lib/traceevent: Change description of few APIs
  tools/lib/traceevent: Coding style fixes
  tools/lib/traceevent: Implement new traceevent APIs for accessing
    struct tep_handler fields

 tools/lib/traceevent/event-parse-api.c   | 190 +++++++++++++++++++----
 tools/lib/traceevent/event-parse-local.h |   2 +
 tools/lib/traceevent/event-parse.c       |   4 +-
 tools/lib/traceevent/event-parse.h       |  12 +-
 tools/lib/traceevent/plugin_kvm.c        |   2 +-
 5 files changed, 173 insertions(+), 37 deletions(-)

-- 
2.20.1

[PATCH 1/3] tools/lib/traceevent: Change description of few APIs

[Tzvetomir Stoyanov]

APIs descriptions should describe the purpose of the
function, its parameters and return value. While working
on man pages implementation, I noticed mismatches in the
descriptions of few APIs. This patch changes the description
of these APIs, making them consistent with the man pages:
tep_print_num_field(), tep_print_func_field(),
tep_get_header_page_size(), tep_get_long_size(),
tep_set_long_size(), tep_get_page_size() and
tep_set_page_size().

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
 tools/lib/traceevent/event-parse-api.c | 20 ++++++++++----------
 tools/lib/traceevent/event-parse.c     |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tools/lib/traceevent/event-parse-api.c b/tools/lib/traceevent/event-parse-api.c
index d463761a58f4..3686a221e981 100644
--- a/tools/lib/traceevent/event-parse-api.c
+++ b/tools/lib/traceevent/event-parse-api.c
@@ -43,8 +43,8 @@ int tep_get_events_count(struct tep_handle *tep)
  * @flag: flag, or combination of flags to be set
  * can be any combination from enum tep_flag
  *
- * This sets a flag or mbination of flags  from enum tep_flag
-  */
+ * This sets a flag or combination of flags from enum tep_flag
+ */
 void tep_set_flag(struct tep_handle *tep, int flag)
 {
 	if(tep)
@@ -140,10 +140,10 @@ void tep_set_cpus(struct tep_handle *pevent, int cpus)
 }
 
 /**
- * tep_get_long_size - get the size of a long integer on the current machine
+ * tep_get_long_size - get the size of a long integer on the traced machine
  * @pevent: a handle to the tep_handle
  *
- * This returns the size of a long integer on the current machine
+ * This returns the size of a long integer on the traced machine
  * If @pevent is NULL, 0 is returned.
  */
 int tep_get_long_size(struct tep_handle *pevent)
@@ -154,11 +154,11 @@ int tep_get_long_size(struct tep_handle *pevent)
 }
 
 /**
- * tep_set_long_size - set the size of a long integer on the current machine
+ * tep_set_long_size - set the size of a long integer on the traced machine
  * @pevent: a handle to the tep_handle
  * @size: size, in bytes, of a long integer
  *
- * This sets the size of a long integer on the current machine
+ * This sets the size of a long integer on the traced machine
  */
 void tep_set_long_size(struct tep_handle *pevent, int long_size)
 {
@@ -167,10 +167,10 @@ void tep_set_long_size(struct tep_handle *pevent, int long_size)
 }
 
 /**
- * tep_get_page_size - get the size of a memory page on the current machine
+ * tep_get_page_size - get the size of a memory page on the traced machine
  * @pevent: a handle to the tep_handle
  *
- * This returns the size of a memory page on the current machine
+ * This returns the size of a memory page on the traced machine
  * If @pevent is NULL, 0 is returned.
  */
 int tep_get_page_size(struct tep_handle *pevent)
@@ -181,11 +181,11 @@ int tep_get_page_size(struct tep_handle *pevent)
 }
 
 /**
- * tep_set_page_size - set the size of a memory page on the current machine
+ * tep_set_page_size - set the size of a memory page on the traced machine
  * @pevent: a handle to the tep_handle
  * @_page_size: size of a memory page, in bytes
  *
- * This sets the size of a memory page on the current machine
+ * This sets the size of a memory page on the traced machine
  */
 void tep_set_page_size(struct tep_handle *pevent, int _page_size)
 {
diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 87494c7c619d..69af77896283 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -6386,7 +6386,7 @@ int tep_get_any_field_val(struct trace_seq *s, struct tep_event *event,
  * @record: The record with the field name.
  * @err: print default error if failed.
  *
- * Returns: 0 on success, -1 field not found, or 1 if buffer is full.
+ * Returns: 1 on success, -1 field not found, or 0 if buffer is full.
  */
 int tep_print_num_field(struct trace_seq *s, const char *fmt,
 			struct tep_event *event, const char *name,
@@ -6418,7 +6418,7 @@ int tep_print_num_field(struct trace_seq *s, const char *fmt,
  * @record: The record with the field name.
  * @err: print default error if failed.
  *
- * Returns: 0 on success, -1 field not found, or 1 if buffer is full.
+ * Returns: 1 on success, -1 field not found, or 0 if buffer is full.
  */
 int tep_print_func_field(struct trace_seq *s, const char *fmt,
 			 struct tep_event *event, const char *name,
-- 
2.20.1

Re: [PATCH 1/3] tools/lib/traceevent: Change description of few APIs

[Matt Helsley]

> On Mar 19, 2019, at 4:19 AM, Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:
> 
> APIs descriptions should describe the purpose of the
> function, its parameters and return value. While working
> on man pages implementation, I noticed mismatches in the
> descriptions of few APIs. This patch changes the description
> of these APIs, making them consistent with the man pages:
> tep_print_num_field(), tep_print_func_field(),
> tep_get_header_page_size(), tep_get_long_size(),
> tep_set_long_size(), tep_get_page_size() and
> tep_set_page_size().
> 
> Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> ---
> tools/lib/traceevent/event-parse-api.c | 20 ++++++++++----------
> tools/lib/traceevent/event-parse.c     |  4 ++--
> 2 files changed, 12 insertions(+), 12 deletions(-)

<snip>

Reviewed-by: Matt Helsley <mhelsley@vmware.com>

[PATCH 2/3] tools/lib/traceevent: Coding style fixes

[Tzvetomir Stoyanov]

Fixed few coding style problems in event-parse-api.c

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
 tools/lib/traceevent/event-parse-api.c | 30 +++++++++++++-------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/tools/lib/traceevent/event-parse-api.c b/tools/lib/traceevent/event-parse-api.c
index 3686a221e981..3716a9142aef 100644
--- a/tools/lib/traceevent/event-parse-api.c
+++ b/tools/lib/traceevent/event-parse-api.c
@@ -32,7 +32,7 @@ struct tep_event *tep_get_first_event(struct tep_handle *tep)
  */
 int tep_get_events_count(struct tep_handle *tep)
 {
-	if(tep)
+	if (tep)
 		return tep->nr_events;
 	return 0;
 }
@@ -47,7 +47,7 @@ int tep_get_events_count(struct tep_handle *tep)
  */
 void tep_set_flag(struct tep_handle *tep, int flag)
 {
-	if(tep)
+	if (tep)
 		tep->flags |= flag;
 }
 
@@ -108,7 +108,7 @@ tep_data2host8(struct tep_handle *pevent, unsigned long long data)
  */
 int tep_get_header_page_size(struct tep_handle *pevent)
 {
-	if(pevent)
+	if (pevent)
 		return pevent->header_page_size_size;
 	return 0;
 }
@@ -122,7 +122,7 @@ int tep_get_header_page_size(struct tep_handle *pevent)
  */
 int tep_get_cpus(struct tep_handle *pevent)
 {
-	if(pevent)
+	if (pevent)
 		return pevent->cpus;
 	return 0;
 }
@@ -135,7 +135,7 @@ int tep_get_cpus(struct tep_handle *pevent)
  */
 void tep_set_cpus(struct tep_handle *pevent, int cpus)
 {
-	if(pevent)
+	if (pevent)
 		pevent->cpus = cpus;
 }
 
@@ -148,7 +148,7 @@ void tep_set_cpus(struct tep_handle *pevent, int cpus)
  */
 int tep_get_long_size(struct tep_handle *pevent)
 {
-	if(pevent)
+	if (pevent)
 		return pevent->long_size;
 	return 0;
 }
@@ -162,7 +162,7 @@ int tep_get_long_size(struct tep_handle *pevent)
  */
 void tep_set_long_size(struct tep_handle *pevent, int long_size)
 {
-	if(pevent)
+	if (pevent)
 		pevent->long_size = long_size;
 }
 
@@ -175,7 +175,7 @@ void tep_set_long_size(struct tep_handle *pevent, int long_size)
  */
 int tep_get_page_size(struct tep_handle *pevent)
 {
-	if(pevent)
+	if (pevent)
 		return pevent->page_size;
 	return 0;
 }
@@ -189,7 +189,7 @@ int tep_get_page_size(struct tep_handle *pevent)
  */
 void tep_set_page_size(struct tep_handle *pevent, int _page_size)
 {
-	if(pevent)
+	if (pevent)
 		pevent->page_size = _page_size;
 }
 
@@ -202,7 +202,7 @@ void tep_set_page_size(struct tep_handle *pevent, int _page_size)
  */
 int tep_file_bigendian(struct tep_handle *pevent)
 {
-	if(pevent)
+	if (pevent)
 		return pevent->file_bigendian;
 	return 0;
 }
@@ -216,7 +216,7 @@ int tep_file_bigendian(struct tep_handle *pevent)
  */
 void tep_set_file_bigendian(struct tep_handle *pevent, enum tep_endian endian)
 {
-	if(pevent)
+	if (pevent)
 		pevent->file_bigendian = endian;
 }
 
@@ -229,7 +229,7 @@ void tep_set_file_bigendian(struct tep_handle *pevent, enum tep_endian endian)
  */
 int tep_is_host_bigendian(struct tep_handle *pevent)
 {
-	if(pevent)
+	if (pevent)
 		return pevent->host_bigendian;
 	return 0;
 }
@@ -243,7 +243,7 @@ int tep_is_host_bigendian(struct tep_handle *pevent)
  */
 void tep_set_host_bigendian(struct tep_handle *pevent, enum tep_endian endian)
 {
-	if(pevent)
+	if (pevent)
 		pevent->host_bigendian = endian;
 }
 
@@ -256,7 +256,7 @@ void tep_set_host_bigendian(struct tep_handle *pevent, enum tep_endian endian)
  */
 int tep_is_latency_format(struct tep_handle *pevent)
 {
-	if(pevent)
+	if (pevent)
 		return pevent->latency_format;
 	return 0;
 }
@@ -270,6 +270,6 @@ int tep_is_latency_format(struct tep_handle *pevent)
   */
 void tep_set_latency_format(struct tep_handle *pevent, int lat)
 {
-	if(pevent)
+	if (pevent)
 		pevent->latency_format = lat;
 }
-- 
2.20.1

Re: [PATCH 2/3] tools/lib/traceevent: Coding style fixes

[Matt Helsley]

> On Mar 19, 2019, at 4:19 AM, Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:
> 
> Fixed few coding style problems in event-parse-api.c
> 
> Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> ---
> tools/lib/traceevent/event-parse-api.c | 30 +++++++++++++-------------
> 1 file changed, 15 insertions(+), 15 deletions(-)

Reviewed-by: Matt Helsley <mhelsley@vmware.com>

[PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Tzvetomir Stoyanov]

As struct tep_handler definition is not exposed as part of libtraceevent API, its fields
cannot be accessed directly by the library users. This patch implements new APIs, which
can be used to access the struct tep_handler fields:
tep_reset_flag(), tep_check_flag(), tep_set_parsing_failures(), tep_get_parsing_failures()
tep_get_header_page_ts_size(), tep_is_old_format(), tep_set_print_raw() and tep_set_test_filters()

Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
---
 tools/lib/traceevent/event-parse-api.c   | 140 +++++++++++++++++++++--
 tools/lib/traceevent/event-parse-local.h |   2 +
 tools/lib/traceevent/event-parse.h       |  12 +-
 tools/lib/traceevent/plugin_kvm.c        |   2 +-
 4 files changed, 146 insertions(+), 10 deletions(-)

diff --git a/tools/lib/traceevent/event-parse-api.c b/tools/lib/traceevent/event-parse-api.c
index 3716a9142aef..2bd2bf7c499d 100644
--- a/tools/lib/traceevent/event-parse-api.c
+++ b/tools/lib/traceevent/event-parse-api.c
@@ -8,6 +8,22 @@
 #include "event-parse-local.h"
 #include "event-utils.h"
 
+/**
+ * tep_get_event - returns the event with the given index
+ * @tep: a handle to the tep_handle
+ * @index: index of the requested event, in the range 0 .. nr_events
+ *
+ * This returns pointer to the element of the events array with the given index
+ * If @tep is NULL, or @index is not in the range 0 .. nr_events, NULL is returned.
+ */
+struct tep_event *tep_get_event(struct tep_handle *tep, int index)
+{
+	if (tep && tep->events && index < tep->nr_events)
+		return tep->events[index];
+
+	return NULL;
+}
+
 /**
  * tep_get_first_event - returns the first event in the events array
  * @tep: a handle to the tep_handle
@@ -17,10 +33,7 @@
  */
 struct tep_event *tep_get_first_event(struct tep_handle *tep)
 {
-	if (tep && tep->events)
-		return tep->events[0];
-
-	return NULL;
+	return tep_get_event(tep, 0);
 }
 
 /**
@@ -45,12 +58,41 @@ int tep_get_events_count(struct tep_handle *tep)
  *
  * This sets a flag or combination of flags from enum tep_flag
  */
-void tep_set_flag(struct tep_handle *tep, int flag)
+void tep_set_flag(struct tep_handle *tep, enum tep_flag flag)
 {
 	if (tep)
 		tep->flags |= flag;
 }
 
+/**
+ * tep_reset_flag - reset event parser flag
+ * @tep: a handle to the tep_handle
+ * @flag: flag, or combination of flags to be reseted
+ * can be any combination from enum tep_flag
+ *
+ * This resets a flag or combination of flags from enum tep_flag
+ */
+void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag)
+{
+	if (tep)
+		tep->flags &= ~flag;
+}
+
+/**
+ * tep_check_flag - check the state of event parser flag
+ * @tep: a handle to the tep_handle
+ * @flag: flag, or combination of flags to be checked
+ * can be any combination from enum tep_flag
+ *
+ * This checks the state of a flag or combination of flags from enum tep_flag
+ */
+int tep_check_flag(struct tep_handle *tep, enum tep_flag flag)
+{
+	if (tep)
+		return (tep->flags & flag);
+	return 0;
+}
+
 unsigned short tep_data2host2(struct tep_handle *pevent, unsigned short data)
 {
 	unsigned short swap;
@@ -113,6 +155,20 @@ int tep_get_header_page_size(struct tep_handle *pevent)
 	return 0;
 }
 
+/**
+ * tep_get_header_page_ts_size - get size of the time stamp in the header page
+ * @tep: a handle to the tep_handle
+ *
+ * This returns size of the time stamp in the header page
+ * If @tep is NULL, 0 is returned.
+ */
+int tep_get_header_page_ts_size(struct tep_handle *tep)
+{
+	if (tep)
+		return tep->header_page_ts_size;
+	return 0;
+}
+
 /**
  * tep_get_cpus - get the number of CPUs
  * @pevent: a handle to the tep_handle
@@ -194,13 +250,13 @@ void tep_set_page_size(struct tep_handle *pevent, int _page_size)
 }
 
 /**
- * tep_file_bigendian - get if the file is in big endian order
+ * tep_is_file_bigendian - get if the file is in big endian order
  * @pevent: a handle to the tep_handle
  *
  * This returns if the file is in big endian order
  * If @pevent is NULL, 0 is returned.
  */
-int tep_file_bigendian(struct tep_handle *pevent)
+int tep_is_file_bigendian(struct tep_handle *pevent)
 {
 	if (pevent)
 		return pevent->file_bigendian;
@@ -273,3 +329,73 @@ void tep_set_latency_format(struct tep_handle *pevent, int lat)
 	if (pevent)
 		pevent->latency_format = lat;
 }
+
+/**
+ * tep_set_parsing_failures - set parsing failures flag
+ * @tep: a handle to the tep_handle
+ * @parsing_failures: the new value of the parsing_failures flag
+ *
+ * This sets flag "parsing_failures" to the given count
+ */
+void tep_set_parsing_failures(struct tep_handle *tep, int parsing_failures)
+{
+	if (tep)
+		tep->parsing_failures = parsing_failures;
+}
+
+/**
+ * tep_get_parsing_failures - get the parsing failures flag
+ * @tep: a handle to the tep_handle
+ *
+ * This returns value of flag "parsing_failures"
+ * If @tep is NULL, 0 is returned.
+ */
+int tep_get_parsing_failures(struct tep_handle *tep)
+{
+	if (tep)
+		return tep->parsing_failures;
+	return 0;
+}
+
+/**
+ * tep_is_old_format - get if an old kernel is used
+ * @tep: a handle to the tep_handle
+ *
+ * This returns 1, if an old kernel is used to generate the tracing events or
+ * 0 if a new kernel is used. Old kernels did not have header page info.
+ * If @pevent is NULL, 0 is returned.
+ */
+int tep_is_old_format(struct tep_handle *tep)
+{
+	if (tep)
+		return tep->old_format;
+	return 0;
+}
+
+/**
+ * tep_set_print_raw - set a flag to force print in raw format
+ * @tep: a handle to the tep_handle
+ * @print_raw: the new value of the print_raw flag
+ *
+ * This sets a flag to force print in raw format
+ */
+void tep_set_print_raw(struct tep_handle *tep, int print_raw)
+{
+	if (tep)
+		tep->print_raw = print_raw;
+}
+
+/**
+ * tep_set_print_raw - set a flag to test a filter string
+ * @tep: a handle to the tep_handle
+ * @test_filters: the new value of the test_filters flag
+ *
+ * This sets a flag to fjust test a filter string. If this flag is set,
+ * when a filter string is added, then it will print the filters strings
+ * that were created and exit.
+ */
+void tep_set_test_filters(struct tep_handle *tep, int test_filters)
+{
+	if (tep)
+		tep->test_filters = test_filters;
+}
diff --git a/tools/lib/traceevent/event-parse-local.h b/tools/lib/traceevent/event-parse-local.h
index 35833ee32d6c..c5c8eb4c4ab7 100644
--- a/tools/lib/traceevent/event-parse-local.h
+++ b/tools/lib/traceevent/event-parse-local.h
@@ -83,6 +83,8 @@ struct tep_handle {
 	struct event_handler *handlers;
 	struct tep_function_handler *func_handlers;
 
+	int parsing_failures;
+
 	/* cache */
 	struct tep_event *last_event;
 
diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index aec48f2aea8a..f695688f38fd 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -408,7 +408,9 @@ void tep_print_plugins(struct trace_seq *s,
 /* tep_handle */
 typedef char *(tep_func_resolver_t)(void *priv,
 				    unsigned long long *addrp, char **modp);
-void tep_set_flag(struct tep_handle *tep, int flag);
+void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
+void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);
+int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);
 
 static inline int tep_host_bigendian(void)
 {
@@ -558,13 +560,19 @@ int tep_get_long_size(struct tep_handle *pevent);
 void tep_set_long_size(struct tep_handle *pevent, int long_size);
 int tep_get_page_size(struct tep_handle *pevent);
 void tep_set_page_size(struct tep_handle *pevent, int _page_size);
-int tep_file_bigendian(struct tep_handle *pevent);
+int tep_is_file_bigendian(struct tep_handle *pevent);
 void tep_set_file_bigendian(struct tep_handle *pevent, enum tep_endian endian);
 int tep_is_host_bigendian(struct tep_handle *pevent);
 void tep_set_host_bigendian(struct tep_handle *pevent, enum tep_endian endian);
 int tep_is_latency_format(struct tep_handle *pevent);
 void tep_set_latency_format(struct tep_handle *pevent, int lat);
 int tep_get_header_page_size(struct tep_handle *pevent);
+void tep_set_parsing_failures(struct tep_handle *tep, int parsing_failures);
+int tep_get_parsing_failures(struct tep_handle *tep);
+int tep_get_header_page_ts_size(struct tep_handle *tep);
+int tep_is_old_format(struct tep_handle *pevent);
+void tep_set_print_raw(struct tep_handle *tep, int print_raw);
+void tep_set_test_filters(struct tep_handle *tep, int test_filters);
 
 struct tep_handle *tep_alloc(void);
 void tep_free(struct tep_handle *pevent);
diff --git a/tools/lib/traceevent/plugin_kvm.c b/tools/lib/traceevent/plugin_kvm.c
index 64b9c25a1fd3..754050eea467 100644
--- a/tools/lib/traceevent/plugin_kvm.c
+++ b/tools/lib/traceevent/plugin_kvm.c
@@ -389,7 +389,7 @@ static int kvm_mmu_print_role(struct trace_seq *s, struct tep_record *record,
 	 * We can only use the structure if file is of the same
 	 * endianness.
 	 */
-	if (tep_file_bigendian(event->pevent) ==
+	if (tep_is_file_bigendian(event->pevent) ==
 	    tep_is_host_bigendian(event->pevent)) {
 
 		trace_seq_printf(s, "%u q%u%s %s%s %spae %snxe %swp%s%s%s",
-- 
2.20.1

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Matt Helsley]

> On Mar 19, 2019, at 4:19 AM, Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:
> 
> As struct tep_handler definition is not exposed as part of libtraceevent API, its fields
> cannot be accessed directly by the library users. This patch implements new APIs, which
> can be used to access the struct tep_handler fields:
> tep_reset_flag(), tep_check_flag(), tep_set_parsing_failures(), tep_get_parsing_failures()
> tep_get_header_page_ts_size(), tep_is_old_format(), tep_set_print_raw() and tep_set_test_filters()
> 
> Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> ---
> tools/lib/traceevent/event-parse-api.c   | 140 +++++++++++++++++++++--
> tools/lib/traceevent/event-parse-local.h |   2 +
> tools/lib/traceevent/event-parse.h       |  12 +-
> tools/lib/traceevent/plugin_kvm.c        |   2 +-
> 4 files changed, 146 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/lib/traceevent/event-parse-api.c b/tools/lib/traceevent/event-parse-api.c
> index 3716a9142aef..2bd2bf7c499d 100644
> --- a/tools/lib/traceevent/event-parse-api.c
> +++ b/tools/lib/traceevent/event-parse-api.c
> @@ -8,6 +8,22 @@
> #include "event-parse-local.h"
> #include "event-utils.h"
> 
> +/**
> + * tep_get_event - returns the event with the given index
> + * @tep: a handle to the tep_handle
> + * @index: index of the requested event, in the range 0 .. nr_events
> + *
> + * This returns pointer to the element of the events array with the given index
> + * If @tep is NULL, or @index is not in the range 0 .. nr_events, NULL is returned.
> + */
> +struct tep_event *tep_get_event(struct tep_handle *tep, int index)
> +{
> +	if (tep && tep->events && index < tep->nr_events)
> +		return tep->events[index];
> +
> +	return NULL;
> +}
> +
> /**
>  * tep_get_first_event - returns the first event in the events array
>  * @tep: a handle to the tep_handle
> @@ -17,10 +33,7 @@
>  */
> struct tep_event *tep_get_first_event(struct tep_handle *tep)
> {
> -	if (tep && tep->events)
> -		return tep->events[0];
> -
> -	return NULL;
> +	return tep_get_event(tep, 0);
> }
> 
> /**
> @@ -45,12 +58,41 @@ int tep_get_events_count(struct tep_handle *tep)
>  *
>  * This sets a flag or combination of flags from enum tep_flag
>  */
> -void tep_set_flag(struct tep_handle *tep, int flag)
> +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag)
> {
> 	if (tep)
> 		tep->flags |= flag;
> }
> 
> +/**
> + * tep_reset_flag - reset event parser flag
> + * @tep: a handle to the tep_handle
> + * @flag: flag, or combination of flags to be reseted

s/reseted/reset/

> + * can be any combination from enum tep_flag
> + *
> + * This resets a flag or combination of flags from enum tep_flag
> + */
> +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag)
> +{
> +	if (tep)
> +		tep->flags &= ~flag;
> +}

nit: rename to reset_flags since it’s one or more flags?

> +
> +/**
> + * tep_check_flag - check the state of event parser flag
> + * @tep: a handle to the tep_handle
> + * @flag: flag, or combination of flags to be checked
> + * can be any combination from enum tep_flag
> + *
> + * This checks the state of a flag or combination of flags from enum tep_flag
> + */
> +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag)
> +{
> +	if (tep)
> +		return (tep->flags & flag);
> +	return 0;
> +}

This returns a subset of the flags directly — it doesn’t really check them -- that’s up to the caller.
So  I’d say this is more of a “getter" than a “checker”.

If returning a “boolean” is the true intent of the API then it should be:

return (tep->flags & flag) == flag;

> +
> unsigned short tep_data2host2(struct tep_handle *pevent, unsigned short data)
> {
> 	unsigned short swap;
> @@ -113,6 +155,20 @@ int tep_get_header_page_size(struct tep_handle *pevent)
> 	return 0;
> }
> 
> +/**
> + * tep_get_header_page_ts_size - get size of the time stamp in the header page
> + * @tep: a handle to the tep_handle
> + *
> + * This returns size of the time stamp in the header page
> + * If @tep is NULL, 0 is returned.
> + */
> +int tep_get_header_page_ts_size(struct tep_handle *tep)
> +{
> +	if (tep)
> +		return tep->header_page_ts_size;
> +	return 0;
> +}
> +
> /**
>  * tep_get_cpus - get the number of CPUs
>  * @pevent: a handle to the tep_handle
> @@ -194,13 +250,13 @@ void tep_set_page_size(struct tep_handle *pevent, int _page_size)
> }
> 
> /**
> - * tep_file_bigendian - get if the file is in big endian order
> + * tep_is_file_bigendian - get if the file is in big endian order
>  * @pevent: a handle to the tep_handle
>  *
>  * This returns if the file is in big endian order
>  * If @pevent is NULL, 0 is returned.
>  */
> -int tep_file_bigendian(struct tep_handle *pevent)
> +int tep_is_file_bigendian(struct tep_handle *pevent)
> {
> 	if (pevent)
> 		return pevent->file_bigendian;

strictly speaking this is more of a getter than a boolean test

<snip>

> +/**
> + * tep_is_old_format - get if an old kernel is used
> + * @tep: a handle to the tep_handle
> + *
> + * This returns 1, if an old kernel is used to generate the tracing events or
> + * 0 if a new kernel is used. Old kernels did not have header page info.
> + * If @pevent is NULL, 0 is returned.
> + */
> +int tep_is_old_format(struct tep_handle *tep)
> +{
> +	if (tep)
> +		return tep->old_format;

Same here.

There may be value in restricting these to boolean returns. Otherwise this function of the API and those implemented like it could be “abused” as getters. That might complicate things in the future if, someday, it would be nice to change things around inside the opaque struct tep_handle’s  old_format field without breaking callers of tep_is_old_format().

int tep_is_foo(...)
{
	if (tep)
		return (tep->value & foo)  == foo; /* flag test */
…

or
		return !!(tep->value); /*  is it non-zero test */

Also, for  these “tep_is_” functions it might be more descriptive to use bool as a return value. Folks may have vetoed use of “bool" before I started looking at this project so feel free to ignore me if that’s the case.

<snip>

> diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> index aec48f2aea8a..f695688f38fd 100644
> --- a/tools/lib/traceevent/event-parse.h
> +++ b/tools/lib/traceevent/event-parse.h
> @@ -408,7 +408,9 @@ void tep_print_plugins(struct trace_seq *s,
> /* tep_handle */
> typedef char *(tep_func_resolver_t)(void *priv,
> 				    unsigned long long *addrp, char **modp);
> -void tep_set_flag(struct tep_handle *tep, int flag);
> +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
> +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);
> +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);
> 
> static inline int tep_host_bigendian(void)
> {

Does another series rename this to: tep_is_host_bigendian() ?

Cheers,
     -Matt

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Tzvetomir Stoyanov]

On Wed, Mar 20, 2019 at 6:32 PM Matt Helsley <mhelsley@vmware.com> wrote:
>
>
>
> > On Mar 19, 2019, at 4:19 AM, Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:
> >
> > As struct tep_handler definition is not exposed as part of libtraceevent API, its fields
> > cannot be accessed directly by the library users. This patch implements new APIs, which
> > can be used to access the struct tep_handler fields:
> > tep_reset_flag(), tep_check_flag(), tep_set_parsing_failures(), tep_get_parsing_failures()
> > tep_get_header_page_ts_size(), tep_is_old_format(), tep_set_print_raw() and tep_set_test_filters()
> >
> > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> > ---
> > tools/lib/traceevent/event-parse-api.c   | 140 +++++++++++++++++++++--
> > tools/lib/traceevent/event-parse-local.h |   2 +
> > tools/lib/traceevent/event-parse.h       |  12 +-
> > tools/lib/traceevent/plugin_kvm.c        |   2 +-
> > 4 files changed, 146 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/lib/traceevent/event-parse-api.c b/tools/lib/traceevent/event-parse-api.c
> > index 3716a9142aef..2bd2bf7c499d 100644
> > --- a/tools/lib/traceevent/event-parse-api.c
> > +++ b/tools/lib/traceevent/event-parse-api.c
> > @@ -8,6 +8,22 @@
> > #include "event-parse-local.h"
> > #include "event-utils.h"
> >
> > +/**
> > + * tep_get_event - returns the event with the given index
> > + * @tep: a handle to the tep_handle
> > + * @index: index of the requested event, in the range 0 .. nr_events
> > + *
> > + * This returns pointer to the element of the events array with the given index
> > + * If @tep is NULL, or @index is not in the range 0 .. nr_events, NULL is returned.
> > + */
> > +struct tep_event *tep_get_event(struct tep_handle *tep, int index)
> > +{
> > +     if (tep && tep->events && index < tep->nr_events)
> > +             return tep->events[index];
> > +
> > +     return NULL;
> > +}
> > +
> > /**
> >  * tep_get_first_event - returns the first event in the events array
> >  * @tep: a handle to the tep_handle
> > @@ -17,10 +33,7 @@
> >  */
> > struct tep_event *tep_get_first_event(struct tep_handle *tep)
> > {
> > -     if (tep && tep->events)
> > -             return tep->events[0];
> > -
> > -     return NULL;
> > +     return tep_get_event(tep, 0);
> > }
> >
> > /**
> > @@ -45,12 +58,41 @@ int tep_get_events_count(struct tep_handle *tep)
> >  *
> >  * This sets a flag or combination of flags from enum tep_flag
> >  */
> > -void tep_set_flag(struct tep_handle *tep, int flag)
> > +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag)
> > {
> >       if (tep)
> >               tep->flags |= flag;
> > }
> >
> > +/**
> > + * tep_reset_flag - reset event parser flag
> > + * @tep: a handle to the tep_handle
> > + * @flag: flag, or combination of flags to be reseted
>
> s/reseted/reset/
>
> > + * can be any combination from enum tep_flag
> > + *
> > + * This resets a flag or combination of flags from enum tep_flag
> > + */
> > +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag)
> > +{
> > +     if (tep)
> > +             tep->flags &= ~flag;
> > +}
>
> nit: rename to reset_flags since it’s one or more flags?
>
> > +
> > +/**
> > + * tep_check_flag - check the state of event parser flag
> > + * @tep: a handle to the tep_handle
> > + * @flag: flag, or combination of flags to be checked
> > + * can be any combination from enum tep_flag
> > + *
> > + * This checks the state of a flag or combination of flags from enum tep_flag
> > + */
> > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag)
> > +{
> > +     if (tep)
> > +             return (tep->flags & flag);
> > +     return 0;
> > +}
>
> This returns a subset of the flags directly — it doesn’t really check them -- that’s up to the caller.
> So  I’d say this is more of a “getter" than a “checker”.
>
> If returning a “boolean” is the true intent of the API then it should be:
>
> return (tep->flags & flag) == flag;
>
> > +
> > unsigned short tep_data2host2(struct tep_handle *pevent, unsigned short data)
> > {
> >       unsigned short swap;
> > @@ -113,6 +155,20 @@ int tep_get_header_page_size(struct tep_handle *pevent)
> >       return 0;
> > }
> >
> > +/**
> > + * tep_get_header_page_ts_size - get size of the time stamp in the header page
> > + * @tep: a handle to the tep_handle
> > + *
> > + * This returns size of the time stamp in the header page
> > + * If @tep is NULL, 0 is returned.
> > + */
> > +int tep_get_header_page_ts_size(struct tep_handle *tep)
> > +{
> > +     if (tep)
> > +             return tep->header_page_ts_size;
> > +     return 0;
> > +}
> > +
> > /**
> >  * tep_get_cpus - get the number of CPUs
> >  * @pevent: a handle to the tep_handle
> > @@ -194,13 +250,13 @@ void tep_set_page_size(struct tep_handle *pevent, int _page_size)
> > }
> >
> > /**
> > - * tep_file_bigendian - get if the file is in big endian order
> > + * tep_is_file_bigendian - get if the file is in big endian order
> >  * @pevent: a handle to the tep_handle
> >  *
> >  * This returns if the file is in big endian order
> >  * If @pevent is NULL, 0 is returned.
> >  */
> > -int tep_file_bigendian(struct tep_handle *pevent)
> > +int tep_is_file_bigendian(struct tep_handle *pevent)
> > {
> >       if (pevent)
> >               return pevent->file_bigendian;
>
> strictly speaking this is more of a getter than a boolean test
>
> <snip>
>
> > +/**
> > + * tep_is_old_format - get if an old kernel is used
> > + * @tep: a handle to the tep_handle
> > + *
> > + * This returns 1, if an old kernel is used to generate the tracing events or
> > + * 0 if a new kernel is used. Old kernels did not have header page info.
> > + * If @pevent is NULL, 0 is returned.
> > + */
> > +int tep_is_old_format(struct tep_handle *tep)
> > +{
> > +     if (tep)
> > +             return tep->old_format;
>
> Same here.
>
> There may be value in restricting these to boolean returns. Otherwise this function of the API and those implemented like it could be “abused” as getters. That might complicate things in the future if, someday, it would be nice to change things around inside the opaque struct tep_handle’s  old_format field without breaking callers of tep_is_old_format().
>
> int tep_is_foo(...)
> {
>         if (tep)
>                 return (tep->value & foo)  == foo; /* flag test */
> …
>
> or
>                 return !!(tep->value); /*  is it non-zero test */
>
> Also, for  these “tep_is_” functions it might be more descriptive to use bool as a return value. Folks may have vetoed use of “bool" before I started looking at this project so feel free to ignore me if that’s the case.
>

All these new APIs are used only in trace-cmd / kernelshark, so they
can be changed / renamed with minimal efforts.
Although the patch was originally intended as syncing trace-cmd repo
upstream, now is the right time to clean up the traceevent API,
I'll backport again renamed / changed  APIs in trace-cmd, when the
patched is merged upstream. I prefer to keep the "checker" names and
to
change the logic and returned types to "bool", as APIs callers (in
trace-cmd) expect it.

Thanks Matt

> <snip>
>
> > diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> > index aec48f2aea8a..f695688f38fd 100644
> > --- a/tools/lib/traceevent/event-parse.h
> > +++ b/tools/lib/traceevent/event-parse.h
> > @@ -408,7 +408,9 @@ void tep_print_plugins(struct trace_seq *s,
> > /* tep_handle */
> > typedef char *(tep_func_resolver_t)(void *priv,
> >                                   unsigned long long *addrp, char **modp);
> > -void tep_set_flag(struct tep_handle *tep, int flag);
> > +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
> > +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);
> > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);
> >
> > static inline int tep_host_bigendian(void)
> > {
>
> Does another series rename this to: tep_is_host_bigendian() ?
>
> Cheers,
>      -Matt
>


-- 

Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Tzvetomir Stoyanov]

On Wed, Mar 20, 2019 at 6:32 PM Matt Helsley <mhelsley@vmware.com> wrote:
>
<snit>
> >
> > static inline int tep_host_bigendian(void)
> > {
>
> Does another series rename this to: tep_is_host_bigendian() ?
>
> Cheers,
>      -Matt
>

This one is a little bit confusing, there are two different APIs:
int tep_host_bigendian(void) - used in perf, checks the byte order of
the local machine
int tep_is_host_bigendian(struct tep_handle *pevent) - checks the
order, stored in the tep_handler

I remember that we discussed this with Steven last year, and decided
to keep both of them. I'm not
aware of all use cases, and what is the purpose of tep_is_host_bigendian() API ?

-- 

Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Steven Rostedt]

On Thu, 21 Mar 2019 10:03:22 +0000
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:

> This one is a little bit confusing, there are two different APIs:
> int tep_host_bigendian(void) - used in perf, checks the byte order of
> the local machine
> int tep_is_host_bigendian(struct tep_handle *pevent) - checks the
> order, stored in the tep_handler
> 
> I remember that we discussed this with Steven last year, and decided
> to keep both of them. I'm not
> aware of all use cases, and what is the purpose of tep_is_host_bigendian() API ?

Yes, there's history to these names. I'll take a look at them and see
how they can change to something less confusing.

-- Steve

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Steven Rostedt]

On Wed, 20 Mar 2019 16:32:25 +0000
Matt Helsley <mhelsley@vmware.com> wrote:

> > +/**
> > + * tep_check_flag - check the state of event parser flag
> > + * @tep: a handle to the tep_handle
> > + * @flag: flag, or combination of flags to be checked
> > + * can be any combination from enum tep_flag
> > + *
> > + * This checks the state of a flag or combination of flags from enum tep_flag
> > + */
> > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag)
> > +{
> > +	if (tep)
> > +		return (tep->flags & flag);
> > +	return 0;
> > +}  
> 
> This returns a subset of the flags directly — it doesn’t really check them -- that’s up to the caller.
> So  I’d say this is more of a “getter" than a “checker”.

If we want to be consistent with the kernel, the proper name is "test"

  tep_test_flag()


> 
> If returning a “boolean” is the true intent of the API then it should be:
> 
> return (tep->flags & flag) == flag;

Hmm, this has some side effects that would require documentation. It
makes it only return true if all flags are set, which is not what we
would want.

Since we are only dealing with one flag, (the parameter is an enum, so
multiple flags would not be correct), keeping the return value as is is
the proper approach.

But I'm fine with making the return value a bool and rename it to
"tep_test_flag()"

-- Steve

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Steven Rostedt]

On Wed, 20 Mar 2019 16:32:25 +0000
Matt Helsley <mhelsley@vmware.com> wrote:

> > 				    unsigned long long *addrp, char **modp);
> > -void tep_set_flag(struct tep_handle *tep, int flag);
> > +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
> > +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);
> > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);
> > 
> > static inline int tep_host_bigendian(void)
> > {  
> 
> Does another series rename this to: tep_is_host_bigendian() ?

I've been looking at the API and I think we do need to make it more
consistent. What we currently have is:


> void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
> void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);

(I'll ignore parameters)

Rename to: void tep_clear_flag()

> int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);

Rename to: bool tep_test_flag()


> 
> int tep_host_bigendian(void);

Rename to: bool tep_is_bigendian()

(Break from the confusion to what a "host" is)

> 
> int tep_set_function_resolver(struct tep_handle *pevent,
> 			      tep_func_resolver_t *func, void *priv);
> void tep_reset_function_resolver(struct tep_handle *pevent);
> int tep_register_comm(struct tep_handle *pevent, const char *comm, int pid);
> int tep_override_comm(struct tep_handle *pevent, const char *comm, int pid);
> int tep_register_trace_clock(struct tep_handle *pevent, const char *trace_clock);
> int tep_register_function(struct tep_handle *pevent, char *name,
> 			  unsigned long long addr, char *mod);
> int tep_register_print_string(struct tep_handle *pevent, const char *fmt,
> 			      unsigned long long addr);
> int tep_pid_is_registered(struct tep_handle *pevent, int pid);

Rename to: bool tep_is_pid_registered()


> 
> int tep_get_cpus(struct tep_handle *pevent);
> void tep_set_cpus(struct tep_handle *pevent, int cpus);
> int tep_get_long_size(struct tep_handle *pevent);
> void tep_set_long_size(struct tep_handle *pevent, int long_size);
> int tep_get_page_size(struct tep_handle *pevent);
> void tep_set_page_size(struct tep_handle *pevent, int _page_size);
> int tep_file_bigendian(struct tep_handle *pevent);

Rename to: bool tep_is_file_bigendian()

> void tep_set_file_bigendian(struct tep_handle *pevent, enum tep_endian endian);
> int tep_is_host_bigendian(struct tep_handle *pevent);

Rename to: bool tep_is_host_bigendian()

> void tep_set_host_bigendian(struct tep_handle *pevent, enum tep_endian endian);
> int tep_is_latency_format(struct tep_handle *pevent);

Rename to: bool tep_is_latency_format()

> void tep_set_latency_format(struct tep_handle *pevent, int lat);
> int tep_get_header_page_size(struct tep_handle *pevent);
> void tep_set_parsing_failures(struct tep_handle *tep, int parsing_failures);
> int tep_get_parsing_failures(struct tep_handle *tep);
> int tep_get_header_page_ts_size(struct tep_handle *tep);

Rename to: int tep_get_header_timestamp_size()

> int tep_is_old_format(struct tep_handle *pevent);

Rename to: bool tep_is_old_format()

These changes may need to be broken up into separate patches.

-- Steve

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Tzvetomir Stoyanov]

On Thu, Mar 21, 2019 at 9:46 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 20 Mar 2019 16:32:25 +0000
> Matt Helsley <mhelsley@vmware.com> wrote:
>
> > >                                 unsigned long long *addrp, char **modp);
> > > -void tep_set_flag(struct tep_handle *tep, int flag);
> > > +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
> > > +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);
> > > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);
> > >
> > > static inline int tep_host_bigendian(void)
> > > {
> >
> > Does another series rename this to: tep_is_host_bigendian() ?
>
> I've been looking at the API and I think we do need to make it more
> consistent. What we currently have is:
>
>
> > void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
> > void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);
>
> (I'll ignore parameters)
>
> Rename to: void tep_clear_flag()
>
> > int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);
>
> Rename to: bool tep_test_flag()
>
>
> >
> > int tep_host_bigendian(void);
>
> Rename to: bool tep_is_bigendian()

Steven, int tep_host_bigendian(void) actually checks the endianness of
the host -
I think it makes sens "host" to be part of API's name. The other one:
int tep_is_host_bigendian(struct tep_handle *pevent) checks the
endianness stored
in the tep_handle, I think we should remove "host" from its name.

>
> (Break from the confusion to what a "host" is)
>
> >
> > int tep_set_function_resolver(struct tep_handle *pevent,
> >                             tep_func_resolver_t *func, void *priv);
> > void tep_reset_function_resolver(struct tep_handle *pevent);
> > int tep_register_comm(struct tep_handle *pevent, const char *comm, int pid);
> > int tep_override_comm(struct tep_handle *pevent, const char *comm, int pid);
> > int tep_register_trace_clock(struct tep_handle *pevent, const char *trace_clock);
> > int tep_register_function(struct tep_handle *pevent, char *name,
> >                         unsigned long long addr, char *mod);
> > int tep_register_print_string(struct tep_handle *pevent, const char *fmt,
> >                             unsigned long long addr);
> > int tep_pid_is_registered(struct tep_handle *pevent, int pid);
>
> Rename to: bool tep_is_pid_registered()
>
>
> >
> > int tep_get_cpus(struct tep_handle *pevent);
> > void tep_set_cpus(struct tep_handle *pevent, int cpus);
> > int tep_get_long_size(struct tep_handle *pevent);
> > void tep_set_long_size(struct tep_handle *pevent, int long_size);
> > int tep_get_page_size(struct tep_handle *pevent);
> > void tep_set_page_size(struct tep_handle *pevent, int _page_size);
> > int tep_file_bigendian(struct tep_handle *pevent);
>
> Rename to: bool tep_is_file_bigendian()
>
> > void tep_set_file_bigendian(struct tep_handle *pevent, enum tep_endian endian);
> > int tep_is_host_bigendian(struct tep_handle *pevent);
>
> Rename to: bool tep_is_host_bigendian()
>
> > void tep_set_host_bigendian(struct tep_handle *pevent, enum tep_endian endian);
> > int tep_is_latency_format(struct tep_handle *pevent);
>
> Rename to: bool tep_is_latency_format()
>
> > void tep_set_latency_format(struct tep_handle *pevent, int lat);
> > int tep_get_header_page_size(struct tep_handle *pevent);
> > void tep_set_parsing_failures(struct tep_handle *tep, int parsing_failures);
> > int tep_get_parsing_failures(struct tep_handle *tep);
> > int tep_get_header_page_ts_size(struct tep_handle *tep);
>
> Rename to: int tep_get_header_timestamp_size()
>
> > int tep_is_old_format(struct tep_handle *pevent);
>
> Rename to: bool tep_is_old_format()
>
> These changes may need to be broken up into separate patches.
>
> -- Steve



-- 

Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Steven Rostedt]

On Fri, 22 Mar 2019 11:33:47 +0000
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:


> > >
> > > int tep_host_bigendian(void);  
> >
> > Rename to: bool tep_is_bigendian()  
> 
> Steven, int tep_host_bigendian(void) actually checks the endianness of
> the host -
> I think it makes sens "host" to be part of API's name. The other one:
> int tep_is_host_bigendian(struct tep_handle *pevent) checks the
> endianness stored
> in the tep_handle, I think we should remove "host" from its name.
> 

I hate the word "host" here. Because what would you get if you run this
on a guest? You get the "guest" endianess (which may possibly be not
the same as the host (if emulated)).


I want to keep this as "tep_is_bigendian()" as it doesn't take a
parameter, and is obvious to what it is returning.

> > > int tep_file_bigendian(struct tep_handle *pevent);  
> >
> > Rename to: bool tep_is_file_bigendian()
> >  
> > > void tep_set_file_bigendian(struct tep_handle *pevent, enum tep_endian endian);
> > > int tep_is_host_bigendian(struct tep_handle *pevent);  
> >
> > Rename to: bool tep_is_host_bigendian()
> >  

Let's rename this to:

void tep_set_local_bigendian()
bool tep_is_local_bigendian()

It can be argued that this could be for the above (test this machine),
but since the "test this machine" doesn't take a parameter, but "test
the pevent bigendian" does, I want it to be consistent with
tep_is_file_bigendian(), which also takes a parameter.

   bool tep_is_bigendian(void);
   bool tep_is_file_bigendian(struct tep_handle *tep);
   bool tep_is_local_bigendian(struct tep_handle *tep);

seems more consistent than:

   bool tep_is_local_bigendian(void);
   bool tep_is_file_bigendian(struct tep_handle *tep);
   bool tep_is_bigendian(struct tep_handle *tep);

So lets go with the first one.

Make sense?

-- Steve

Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Tzvetomir Stoyanov]

On Fri, Mar 22, 2019 at 2:16 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 22 Mar 2019 11:33:47 +0000
> Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:
>
>
> > > >
> > > > int tep_host_bigendian(void);
> > >
> > > Rename to: bool tep_is_bigendian()
> >
> > Steven, int tep_host_bigendian(void) actually checks the endianness of
> > the host -
> > I think it makes sens "host" to be part of API's name. The other one:
> > int tep_is_host_bigendian(struct tep_handle *pevent) checks the
> > endianness stored
> > in the tep_handle, I think we should remove "host" from its name.
> >
>
> I hate the word "host" here. Because what would you get if you run this
> on a guest? You get the "guest" endianess (which may possibly be not
> the same as the host (if emulated)).
>
>
> I want to keep this as "tep_is_bigendian()" as it doesn't take a
> parameter, and is obvious to what it is returning.
>
> > > > int tep_file_bigendian(struct tep_handle *pevent);
> > >
> > > Rename to: bool tep_is_file_bigendian()
> > >
> > > > void tep_set_file_bigendian(struct tep_handle *pevent, enum tep_endian endian);
> > > > int tep_is_host_bigendian(struct tep_handle *pevent);
> > >
> > > Rename to: bool tep_is_host_bigendian()
> > >
>
> Let's rename this to:
>
> void tep_set_local_bigendian()
> bool tep_is_local_bigendian()
>
> It can be argued that this could be for the above (test this machine),
> but since the "test this machine" doesn't take a parameter, but "test
> the pevent bigendian" does, I want it to be consistent with
> tep_is_file_bigendian(), which also takes a parameter.
>
>    bool tep_is_bigendian(void);
>    bool tep_is_file_bigendian(struct tep_handle *tep);
>    bool tep_is_local_bigendian(struct tep_handle *tep);
>
> seems more consistent than:
>
>    bool tep_is_local_bigendian(void);
>    bool tep_is_file_bigendian(struct tep_handle *tep);
>    bool tep_is_bigendian(struct tep_handle *tep);
>
> So lets go with the first one.
>
> Make sense?
>
> -- Steve

Yes, thanks Steven

-- 

Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center