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