Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH] tools lib traceevent: Hide non API functions
Date: Fri, 25 Sep 2020 10:17:24 -0400
Message-ID: <20200925101724.64e55c61@oasis.local.home> (raw)
In-Reply-To: <20200924070609.100771-2-tz.stoyanov@gmail.com>

On Thu, 24 Sep 2020 10:06:09 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:

> There are internal library functions, which are not decalred 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

You have tep_free_plugin_paths twice.

>  tep_free_plugin_paths
> 
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  tools/lib/traceevent/event-parse-api.c   |  8 +--
>  tools/lib/traceevent/event-parse-local.h | 24 +++++--
>  tools/lib/traceevent/event-parse.c       | 88 ++++++------------------
>  tools/lib/traceevent/event-parse.h       |  8 ---
>  tools/lib/traceevent/event-plugin.c      |  2 +-
>  tools/lib/traceevent/parse-filter.c      | 23 +++----
>  6 files changed, 52 insertions(+), 101 deletions(-)
> 
> diff --git a/tools/lib/traceevent/event-parse-api.c b/tools/lib/traceevent/event-parse-api.c
> index 4faf52a65791..23c4a00ae45b 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)

As these are hidden, we don't need the leading underscore. Thus
data2host2 and data2host4, etc, is fine.

>  {
>  	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..d9417437679a 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);

Same for __peek_char(). We can call it peek_char().

Other than that, I think this looks good.

When sending the next version, I would send it to Arnaldo directly and
Cc this list, LKML, and myself. Then I'll just ack it for Arnaldo to
take it directly from you.

Add v2 in the subject '[PATCH v2]' and state the differences of v1
after the '---' of your signed off by. You could also add,

 V1 found here: https://lore.kernel.org/r/20200924070609.100771-2-tz.stoyanov@gmail.com

so that Arnaldo knows where to find v1, as I don't think he's on this
list.

Thanks!

-- Steve

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-24  7:06 [PATCH] trace-cmd libtraceevent: Fix build warning on 32-bit arches Tzvetomir Stoyanov (VMware)
2020-09-24  7:06 ` [PATCH] tools lib traceevent: Hide non API functions Tzvetomir Stoyanov (VMware)
2020-09-25 14:17   ` Steven Rostedt [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200925101724.64e55c61@oasis.local.home \
    --to=rostedt@goodmis.org \
    --cc=linux-trace-devel@vger.kernel.org \
    --cc=tz.stoyanov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git