Re: [PATCH v3] tools lib traceevent: Hide non API functions

2020-09-29 Thread Steven Rostedt 
On Tue, 29 Sep 2020 20:35:21 +0300
"Tzvetomir Stoyanov (VMware)"  wrote:

> There are internal library functions, which are not declared as a static.
> They are used inside the library from different files. Hide them from
> the library users, as they are not part of the API.
> These functions are made hidden and are renamed without the prefix "tep_":
>  tep_free_plugin_paths
>  tep_peek_char
>  tep_buffer_init
>  tep_get_input_buf_ptr
>  tep_get_input_buf
>  tep_read_token
>  tep_free_token
>  tep_free_event
>  tep_free_format_field

I would mention in the change log about the __tep_parse_format() being made
static.

> 
> Link: 
> https://lore.kernel.org/linux-trace-devel/e4afdd82deb5e023d53231bb13e08dca78085fb0.ca...@decadent.org.uk/
> Reported-by: Ben Hutchings 
> Signed-off-by: Tzvetomir Stoyanov (VMware) 
> ---
> v1 of the patch is here: 
> https://lore.kernel.org/r/20200924070609.100771-2-tz.stoya...@gmail.com
> v2 changes (addressed Steven's comments):
>   - Removed leading underscores from the names of newly hidden internal
> functions.
> v3 changes (addressed Steven's comment):
>   - Moved comments from removed APIs to internal functions.
>   - Fixed a typo in patch description.
> 
>  tools/lib/traceevent/event-parse-api.c   |   8 +-
>  tools/lib/traceevent/event-parse-local.h |  24 +++--
>  tools/lib/traceevent/event-parse.c   | 125 ++-
>  tools/lib/traceevent/event-parse.h   |   8 --
>  tools/lib/traceevent/event-plugin.c  |   2 +-
>  tools/lib/traceevent/parse-filter.c  |  23 ++---
>  6 files changed, 83 insertions(+), 107 deletions(-)
> 

>  /**
> - * tep_peek_char - peek at the next character that will be read
> + * peek_char - peek at the next character that will be read
>   *
>   * Returns the next character read, or -1 if end of buffer.
>   */
> -int tep_peek_char(void)
> +__hidden  int peek_char(void)

Nit, but there's two spaces between "__hidden" and "int", should only be
one.

>  {
> - return __peek_char();
> + if (input_buf_ptr >= input_buf_siz)
> + return -1;
> +
> + return input_buf[input_buf_ptr];
>  }
>  

>  /**
> - * __tep_parse_format - parse the event format
> + * __parse_format - parse the event format
>   * @buf: the buffer storing the event format string
>   * @size: the size of @buf
>   * @sys: the system the event belongs to
> @@ -6762,9 +6741,9 @@ static int find_event_handle(struct tep_handle *tep, 
> struct tep_event *event)
>   *
>   * /sys/kernel/debug/tracing/events/.../.../format
>   */
> -enum tep_errno __tep_parse_format(struct tep_event **eventp,
> -   struct tep_handle *tep, const char *buf,
> -   unsigned long size, const char *sys)
> +static enum tep_errno __parse_format(struct tep_event **eventp,

Actually, we don't need the "__" prefix. Just call it "parse_format".


> +struct tep_handle *tep, const char 
> *buf,
> +unsigned long size, const char *sys)
>  {
>   struct tep_event *event;
>   int ret;

> @@ -959,7 +954,7 @@ process_filter(struct tep_event *event, struct 
> tep_filter_arg **parg,
>  
>   do {
>   free(token);
> - type = read_token();
> + type = filter_read_token();

Hmm, did you mean to change this from "read_token()" to
"filter_read_token()"?

-- Steve


>   switch (type) {
>   case TEP_EVENT_SQUOTE:
>   case TEP_EVENT_DQUOTE:
> @@ -1185,7 +1180,7 @@ process_event(struct tep_event *event, const char 
> *filter_str,
>  {
>   int ret;
>  
> - tep_buffer_init(filter_str, strlen(filter_str));
> + init_input_buf(filter_str, strlen(filter_str));
>  
>   ret = process_filter(event, parg, error_str, 0);
>   if (ret < 0)
> @@ -1243,7 +1238,7 @@ filter_event(struct tep_event_filter *filter, struct 
> tep_event *event,
>  static void filter_init_error_buf(struct tep_event_filter *filter)
>  {
>   /* clear buffer to reset show error */
> - tep_buffer_init("", 0);
> + init_input_buf("", 0);
>   filter->error_buffer[0] = '\0';
>  }
>

[PATCH v3] tools lib traceevent: Hide non API functions

2020-09-29 Thread Tzvetomir Stoyanov (VMware) 
There are internal library functions, which are not declared as a static.
They are used inside the library from different files. Hide them from
the library users, as they are not part of the API.
These functions are made hidden and are renamed without the prefix "tep_":
 tep_free_plugin_paths
 tep_peek_char
 tep_buffer_init
 tep_get_input_buf_ptr
 tep_get_input_buf
 tep_read_token
 tep_free_token
 tep_free_event
 tep_free_format_field

Link: 
https://lore.kernel.org/linux-trace-devel/e4afdd82deb5e023d53231bb13e08dca78085fb0.ca...@decadent.org.uk/
Reported-by: Ben Hutchings 
Signed-off-by: Tzvetomir Stoyanov (VMware) 
---
v1 of the patch is here: 
https://lore.kernel.org/r/20200924070609.100771-2-tz.stoya...@gmail.com
v2 changes (addressed Steven's comments):
  - Removed leading underscores from the names of newly hidden internal
functions.
v3 changes (addressed Steven's comment):
  - Moved comments from removed APIs to internal functions.
  - Fixed a typo in patch description.

 tools/lib/traceevent/event-parse-api.c   |   8 +-
 tools/lib/traceevent/event-parse-local.h |  24 +++--
 tools/lib/traceevent/event-parse.c   | 125 ++-
 tools/lib/traceevent/event-parse.h   |   8 --
 tools/lib/traceevent/event-plugin.c  |   2 +-
 tools/lib/traceevent/parse-filter.c  |  23 ++---
 6 files changed, 83 insertions(+), 107 deletions(-)

diff --git a/tools/lib/traceevent/event-parse-api.c 
b/tools/lib/traceevent/event-parse-api.c
index 4faf52a65791..f8361e45d446 100644
--- a/tools/lib/traceevent/event-parse-api.c
+++ b/tools/lib/traceevent/event-parse-api.c
@@ -92,7 +92,7 @@ bool tep_test_flag(struct tep_handle *tep, enum tep_flag flag)
return false;
 }
 
-unsigned short tep_data2host2(struct tep_handle *tep, unsigned short data)
+__hidden unsigned short data2host2(struct tep_handle *tep, unsigned short data)
 {
unsigned short swap;
 
@@ -105,7 +105,7 @@ unsigned short tep_data2host2(struct tep_handle *tep, 
unsigned short data)
return swap;
 }
 
-unsigned int tep_data2host4(struct tep_handle *tep, unsigned int data)
+__hidden unsigned int data2host4(struct tep_handle *tep, unsigned int data)
 {
unsigned int swap;
 
@@ -120,8 +120,8 @@ unsigned int tep_data2host4(struct tep_handle *tep, 
unsigned int data)
return swap;
 }
 
-unsigned long long
-tep_data2host8(struct tep_handle *tep, unsigned long long data)
+__hidden  unsigned long long
+data2host8(struct tep_handle *tep, unsigned long long data)
 {
unsigned long long swap;
 
diff --git a/tools/lib/traceevent/event-parse-local.h 
b/tools/lib/traceevent/event-parse-local.h
index d805a920af6f..fd4bbcfbb849 100644
--- a/tools/lib/traceevent/event-parse-local.h
+++ b/tools/lib/traceevent/event-parse-local.h
@@ -15,6 +15,8 @@ struct event_handler;
 struct func_resolver;
 struct tep_plugins_dir;
 
+#define __hidden __attribute__((visibility ("hidden")))
+
 struct tep_handle {
int ref_count;
 
@@ -102,12 +104,20 @@ struct tep_print_parse {
struct tep_print_arg*len_as_arg;
 };
 
-void tep_free_event(struct tep_event *event);
-void tep_free_format_field(struct tep_format_field *field);
-void tep_free_plugin_paths(struct tep_handle *tep);
-
-unsigned short tep_data2host2(struct tep_handle *tep, unsigned short data);
-unsigned int tep_data2host4(struct tep_handle *tep, unsigned int data);
-unsigned long long tep_data2host8(struct tep_handle *tep, unsigned long long 
data);
+void free_tep_event(struct tep_event *event);
+void free_tep_format_field(struct tep_format_field *field);
+void free_tep_plugin_paths(struct tep_handle *tep);
+
+unsigned short data2host2(struct tep_handle *tep, unsigned short data);
+unsigned int data2host4(struct tep_handle *tep, unsigned int data);
+unsigned long long data2host8(struct tep_handle *tep, unsigned long long data);
+
+/* access to the internal parser */
+int peek_char(void);
+void init_input_buf(const char *buf, unsigned long long size);
+unsigned long long get_input_buf_ptr(void);
+const char *get_input_buf(void);
+enum tep_event_type read_token(char **tok);
+void free_token(char *tok);
 
 #endif /* _PARSE_EVENTS_INT_H */
diff --git a/tools/lib/traceevent/event-parse.c 
b/tools/lib/traceevent/event-parse.c
index 5acc18b32606..590640e97ecc 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -54,19 +54,26 @@ static int show_warning = 1;
warning(fmt, ##__VA_ARGS__);\
} while (0)
 
-static void init_input_buf(const char *buf, unsigned long long size)
+/**
+ * init_input_buf - init buffer for parsing
+ * @buf: buffer to parse
+ * @size: the size of the buffer
+ *
+ * Initializes the internal buffer that tep_read_token() will parse.
+ */
+__hidden void init_input_buf(const char *buf, unsigned long long size)
 {
input_buf = buf;
input_buf_siz = size;
input_buf_ptr = 0;
 }
 
-const char *tep_get_input_buf(void)
+__hidden const char