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

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 29 Sep 2020 20:35:21 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> 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.camel@decadent.org.uk/
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
> v1 of the patch is here: https://lore.kernel.org/r/20200924070609.100771-2-tz.stoyanov@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(&token);
> +		type = filter_read_token(&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';
>  }
>