[PATCH v3 0/2] tracing: Add and use event_command parsing func helpers

[PATCH v3 0/2] tracing: Add and use event_command parsing func helpers

[Tom Zanussi]

With more event commands being implemented, it's been pointed out that
it would make sense to clean up the existing ones and make it easier
to implement new ones without copying a lot of boilerplate.  The main
culprit here is the event_command.func() callback - the rest of the
event_command infrastructure has default implementations that work for
most implementations.  The func() callback is a little different in
that every new command needs to customize parsing to some extent.

This patchset attempts to help clean that up and make it easier for
new users to deal with.

v3: broke up event_trigger_check() into smaller functions instead of
    parameterizing it, and added function documentation.

v2: removed unused event_trigger_remove(). No change in functionality.

The following changes since commit 67d4f6e3bf5dddced226fbf19704cdbbb0c98847:

  ftrace/samples: Add missing prototype for my_direct_func (2021-11-01 20:56:51 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/cleanup-hist-func-v3

Tom Zanussi (2):
  tracing: Add helper functions to simplify event_command callback
    handling
  tracing: Have existing event_command implementations use helpers

 kernel/trace/trace.h                |  23 ++
 kernel/trace/trace_events_hist.c    |  51 +---
 kernel/trace/trace_events_trigger.c | 439 ++++++++++++++++++++--------
 3 files changed, 361 insertions(+), 152 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] tracing: Add helper functions to simplify event_command callback handling

[Tom Zanussi]

The event_command.func() callback is responsible for parsing and
registering triggers.  The existing command implementions for this
callback duplicate a lot of the same code, so to clean up and
consolidate those implementations, introduce a handful of helper
functions for implementors to use.

This also makes it easier for new commands to be implemented and
allows them to focus more on the customizations they provide rather
than obscuring and complicating it with boilerplate code.

Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace.h                |  23 +++
 kernel/trace/trace_events_trigger.c | 286 ++++++++++++++++++++++++++++
 2 files changed, 309 insertions(+)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 6b60ab9475ed..66cc32aca2e8 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1594,6 +1594,29 @@ get_named_trigger_data(struct event_trigger_data *data);
 extern int register_event_command(struct event_command *cmd);
 extern int unregister_event_command(struct event_command *cmd);
 extern int register_trigger_hist_enable_disable_cmds(void);
+extern bool event_trigger_check_remove(char *cmd);
+extern bool event_trigger_empty_param(char *param);
+extern int event_trigger_separate_filter(char **trigger, char **param, bool param_required);
+extern struct event_trigger_data *
+event_trigger_alloc(struct event_command *cmd_ops,
+		    char *cmd,
+		    char *param,
+		    void *private_data);
+extern int event_trigger_parse_num(char *trigger,
+				   struct event_trigger_data *trigger_data);
+extern int event_trigger_set_filter(struct event_command *cmd_ops,
+				    struct trace_event_file *file,
+				    char *param,
+				    struct event_trigger_data *trigger_data);
+extern void event_trigger_reset_filter(struct event_command *cmd_ops,
+				       struct event_trigger_data *trigger_data);
+extern int event_trigger_register(struct event_command *cmd_ops,
+				  struct trace_event_file *file,
+				  char *glob,
+				  char *cmd,
+				  char *trigger,
+				  struct event_trigger_data *trigger_data,
+				  int *n_registered);
 
 /**
  * struct event_trigger_ops - callbacks for trace event triggers
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index 3d5c07239a2a..a873f4e8be04 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -621,6 +621,292 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
 		data->ops->free(data->ops, data);
 }
 
+/*
+ * Event trigger parsing helper functions.
+ *
+ * These functions help make it easier to write an event trigger
+ * parsing function i.e. the struct event_command.func() callback
+ * function responsible for parsing and registering a trigger command
+ * written to the 'trigger' file.
+ *
+ * There are a few different categories of event trigger covered by
+ * these helpers:
+ *
+ *  - triggers that don't require a parameter e.g. traceon
+ *  - triggers that do require a parameter e.g. enable_event and hist
+ *  - triggers that though they may not require a param may support an
+ *    optional 'n' param (n = number of times the trigger should fire)
+ *    e.g.: traceon:5 or enable_event:sys:event:n
+ *  - triggers that do not support an 'n' param e.g. hist
+ *
+ * These functions can be used or ignored as necessary - it all
+ * depends on the complexity of the trigger, and the granularity of
+ * the functions supported reflects the fact that some implementations
+ * may need to customize certain aspects of their implementations and
+ * won't need certain functions.  For instance, the hist trigger
+ * implementation doesn't use event_trigger_separate_filter() because
+ * it has special requirements for handling the filter.
+ */
+
+/**
+ * event_trigger_check_remove - check whether an event trigger specifies remove
+ * @glob: The trigger command string, with optional remove(!) operator
+ *
+ * The event trigger callback implementations pass in 'glob' as a
+ * parameter.  This is the command name either with or without a
+ * remove(!)  operator.  This function simply parses the glob and
+ * determines whether the command corresponds to a trigger removal or
+ * a trigger addition.
+ *
+ * Return: true if this is a remove command, false otherwise
+ */
+bool event_trigger_check_remove(char *glob)
+{
+	return (glob && glob[0] == '!') ? true : false;
+}
+
+/**
+ * event_trigger_empty_param - check whether the param is empty
+ * @param: The trigger param string
+ *
+ * The event trigger callback implementations pass in 'param' as a
+ * parameter.  This corresponds to the string following the command
+ * name minus the command name.  This function can be called by a
+ * callback implementation for any command that requires a param; a
+ * callback that doesn't require a param can ignore it.
+ *
+ * Return: true if this is an empty param, false otherwise
+ */
+bool event_trigger_empty_param(char *param)
+{
+	return !param;
+}
+
+/**
+ * event_trigger_separate_filter - separate an event trigger from a filter
+ * @trigger: outparam, will be filled with a pointer to the trigger
+ * @param: The param string, will contain the filter after this is called
+ * @param_required: Specifies whether or not the param string is required
+ *
+ * Given a param string of the form '[trigger] [if filter]', this
+ * function separates the filter from the trigger and returns the
+ * trigger in *trigger and the filter in *param.  Either the *trigger
+ * or the *param may be set to NULL by this function - if not set to
+ * NULL, they will contain strings corresponding to the trigger and
+ * filter.
+ *
+ * There are two cases that need to be handled with respect to the
+ * passed-in param: either the param is required, or it is not
+ * required.  If @param_required is set, and there's no param, it will
+ * return -EINVAL.  If @param_required is not set and there's a param
+ * that starts with a number, that corresponds to the case of a
+ * trigger with :n (n = number of times the trigger should fire) and
+ * the parsing continues normally; otherwise the function just returns
+ * and assumes param just contains a filter and there's nothing else
+ * to do.
+ *
+ * Return: 0 on success, errno otherwise
+ */
+int event_trigger_separate_filter(char **trigger, char **param, bool param_required)
+{
+	int ret = 0;
+
+	*trigger = NULL;
+
+	if (!*param) {
+		if (param_required)
+			ret = -EINVAL;
+		goto out;
+	}
+
+	/* the only legal optional param is :n */
+	if (!param_required && *param && !isdigit(*param[0]))
+		goto out;
+
+	/* separate the trigger from the filter (trigger [if filter]) */
+	*trigger = strsep(param, " \t");
+	if (!*trigger) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (*param) {
+		*param = skip_spaces(*param);
+		if (!**param)
+			*param = NULL;
+	}
+out:
+	return ret;
+}
+
+/**
+ * event_trigger_alloc - allocate and init event_trigger_data for a trigger
+ * @cmd_ops: The event_command operations for the trigger
+ * @cmd: The cmd string
+ * @param: The param string
+ * @private_data: User data to associate with the event trigger
+ *
+ * Allocate an event_trigger_data instance and initialize it.  The
+ * @cmd_ops are used along with the @cmd and @param to get the
+ * trigger_ops to assign to the event_trigger_data.  @private_data can
+ * also be passed in and associated with the event_trigger_data.
+ *
+ * Use event_trigger_free() to free an event_trigger_data object.
+ *
+ * Return: The trigger_data object success, NULL otherwise
+ */
+struct event_trigger_data *event_trigger_alloc(struct event_command *cmd_ops,
+					       char *cmd,
+					       char *param,
+					       void *private_data)
+{
+	struct event_trigger_data *trigger_data;
+	struct event_trigger_ops *trigger_ops;
+
+	trigger_ops = cmd_ops->get_trigger_ops(cmd, param);
+
+	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+	if (!trigger_data)
+		return NULL;
+
+	trigger_data->count = -1;
+	trigger_data->ops = trigger_ops;
+	trigger_data->cmd_ops = cmd_ops;
+	trigger_data->private_data = private_data;
+
+	INIT_LIST_HEAD(&trigger_data->list);
+	INIT_LIST_HEAD(&trigger_data->named_list);
+	RCU_INIT_POINTER(trigger_data->filter, NULL);
+
+	return trigger_data;
+}
+
+/**
+ * event_trigger_parse_num - parse and return the number param for a trigger
+ * @trigger: The trigger string
+ * @trigger_data: The trigger_data for the trigger
+ *
+ * Parse the :n (n = number of times the trigger should fire) param
+ * and set the count variable in the trigger_data to the parsed count.
+ *
+ * Return: 0 on success, errno otherwise
+ */
+int event_trigger_parse_num(char *trigger,
+			    struct event_trigger_data *trigger_data)
+{
+	char *number;
+	int ret = 0;
+
+	if (trigger) {
+		number = strsep(&trigger, ":");
+
+		if (!strlen(number))
+			return -EINVAL;
+
+		/*
+		 * We use the callback data field (which is a pointer)
+		 * as our counter.
+		 */
+		ret = kstrtoul(number, 0, &trigger_data->count);
+	}
+
+	return ret;
+}
+
+/**
+ * event_trigger_set_filter - set an event trigger's filter
+ * @cmd_ops: The event_command operations for the trigger
+ * @file: The event file for the trigger's event
+ * @param: The string containing the filter
+ * @trigger_data: The trigger_data for the trigger
+ *
+ * Set the filter for the trigger.  If the filter is NULL, just return
+ * without error.
+ *
+ * Return: 0 on success, errno otherwise
+ */
+int event_trigger_set_filter(struct event_command *cmd_ops,
+			     struct trace_event_file *file,
+			     char *param,
+			     struct event_trigger_data *trigger_data)
+{
+	if (param && cmd_ops->set_filter)
+		return cmd_ops->set_filter(param, trigger_data, file);
+
+	return 0;
+}
+
+/**
+ * event_trigger_reset_filter - reset an event trigger's filter
+ * @cmd_ops: The event_command operations for the trigger
+ * @trigger_data: The trigger_data for the trigger
+ *
+ * Reset the filter for the trigger to no filter.
+ */
+void event_trigger_reset_filter(struct event_command *cmd_ops,
+				struct event_trigger_data *trigger_data)
+{
+	if (cmd_ops->set_filter)
+		cmd_ops->set_filter(NULL, trigger_data, NULL);
+}
+
+/**
+ * event_trigger_register - register an event trigger
+ * @cmd_ops: The event_command operations for the trigger
+ * @file: The event file for the trigger's event
+ * @glob: The trigger command string, with optional remove(!) operator
+ * @cmd: The cmd string
+ * @param: The param string
+ * @trigger_data: The trigger_data for the trigger
+ * @n_registered: optional outparam, the number of triggers registered
+ *
+ * Register and event trigger.  The @cmd_ops are used along with the
+ * @cmd and @param to get the trigger_ops to pass to the
+ * cmd_ops->reg() function which actually does the registration. The
+ * cmd_ops->reg() function returns the number of triggers registered,
+ * which is assigned to n_registered, if n_registered is non-NULL.
+ *
+ * Return: 0 on success, errno otherwise
+ */
+int event_trigger_register(struct event_command *cmd_ops,
+			   struct trace_event_file *file,
+			   char *glob,
+			   char *cmd,
+			   char *param,
+			   struct event_trigger_data *trigger_data,
+			   int *n_registered)
+{
+	struct event_trigger_ops *trigger_ops;
+	int ret;
+
+	if (n_registered)
+		*n_registered = 0;
+
+	trigger_ops = cmd_ops->get_trigger_ops(cmd, param);
+
+	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
+	/*
+	 * The above returns on success the # of functions enabled,
+	 * but if it didn't find any functions it returns zero.
+	 * Consider no functions a failure too.
+	 */
+	if (!ret) {
+		cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
+		ret = -ENOENT;
+	} else if (ret > 0) {
+		if (n_registered)
+			*n_registered = ret;
+		/* Just return zero, not the number of enabled functions */
+		ret = 0;
+	}
+
+	return ret;
+}
+
+/*
+ * End event trigger parsing helper functions.
+ */
+
 /**
  * event_trigger_callback - Generic event_command @func implementation
  * @cmd_ops: The command ops, used for trigger registration
-- 
2.17.1

[PATCH v3 2/2] tracing: Have existing event_command implementations use helpers

[Tom Zanussi]

Simplify the existing event_command implementations by making use of
the helper functions previously introduced.

Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
 kernel/trace/trace_events_hist.c    |  51 +++------
 kernel/trace/trace_events_trigger.c | 159 +++++++---------------------
 2 files changed, 55 insertions(+), 155 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 8ff572a31fd3..b7dbbafbe8c7 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6128,10 +6128,11 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 	struct hist_trigger_attrs *attrs;
 	struct event_trigger_ops *trigger_ops;
 	struct hist_trigger_data *hist_data;
+	char *trigger, *p, *start;
 	struct synth_event *se;
 	const char *se_name;
-	bool remove = false;
-	char *trigger, *p, *start;
+	int n_registered;
+	bool remove;
 	int ret = 0;
 
 	lockdep_assert_held(&event_mutex);
@@ -6141,11 +6142,10 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 		last_cmd_set(file, param);
 	}
 
-	if (!param)
-		return -EINVAL;
+	remove = event_trigger_check_remove(glob);
 
-	if (glob[0] == '!')
-		remove = true;
+	if (event_trigger_empty_param(param))
+		return -EINVAL;
 
 	/*
 	 * separate the trigger from the filter (k:v [if filter])
@@ -6202,29 +6202,15 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 		return PTR_ERR(hist_data);
 	}
 
-	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
-
-	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+	trigger_data = event_trigger_alloc(cmd_ops, cmd, trigger, hist_data);
 	if (!trigger_data) {
 		ret = -ENOMEM;
 		goto out_free;
 	}
 
-	trigger_data->count = -1;
-	trigger_data->ops = trigger_ops;
-	trigger_data->cmd_ops = cmd_ops;
-
-	INIT_LIST_HEAD(&trigger_data->list);
-	RCU_INIT_POINTER(trigger_data->filter, NULL);
-
-	trigger_data->private_data = hist_data;
-
-	/* if param is non-empty, it's supposed to be a filter */
-	if (param && cmd_ops->set_filter) {
-		ret = cmd_ops->set_filter(param, trigger_data, file);
-		if (ret < 0)
-			goto out_free;
-	}
+	ret = event_trigger_set_filter(cmd_ops, file, param, trigger_data);
+	if (ret < 0)
+		goto out_free;
 
 	if (remove) {
 		if (!have_hist_trigger_match(trigger_data, file))
@@ -6244,18 +6230,14 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 		goto out_free;
 	}
 
-	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
-	/*
-	 * The above returns on success the # of triggers registered,
-	 * but if it didn't register any it returns zero.  Consider no
-	 * triggers registered a failure too.
-	 */
-	if (!ret) {
+	ret = event_trigger_register(cmd_ops, file, glob, cmd, trigger, trigger_data, &n_registered);
+	if (ret)
+		goto out_free;
+	if ((ret == 0) && (n_registered == 0)) {
 		if (!(attrs->pause || attrs->cont || attrs->clear))
 			ret = -ENOENT;
 		goto out_free;
-	} else if (ret < 0)
-		goto out_free;
+	}
 
 	if (get_named_trigger_data(trigger_data))
 		goto enable;
@@ -6289,8 +6271,7 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
  out_unreg:
 	cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
  out_free:
-	if (cmd_ops->set_filter)
-		cmd_ops->set_filter(NULL, trigger_data, NULL);
+	event_trigger_reset_filter(cmd_ops, trigger_data);
 
 	remove_hist_vars(hist_data);
 
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index a873f4e8be04..1d1716d5bee2 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -931,89 +931,47 @@ event_trigger_callback(struct event_command *cmd_ops,
 	struct event_trigger_data *trigger_data;
 	struct event_trigger_ops *trigger_ops;
 	char *trigger = NULL;
-	char *number;
+	bool remove;
 	int ret;
 
-	/* separate the trigger from the filter (t:n [if filter]) */
-	if (param && isdigit(param[0])) {
-		trigger = strsep(&param, " \t");
-		if (param) {
-			param = skip_spaces(param);
-			if (!*param)
-				param = NULL;
-		}
-	}
+	remove = event_trigger_check_remove(glob);
 
-	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
+	ret = event_trigger_separate_filter(&trigger, &param, false);
+	if (ret)
+		return ret;
 
 	ret = -ENOMEM;
-	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+	trigger_data = event_trigger_alloc(cmd_ops, cmd, trigger, file);
 	if (!trigger_data)
 		goto out;
 
-	trigger_data->count = -1;
-	trigger_data->ops = trigger_ops;
-	trigger_data->cmd_ops = cmd_ops;
-	trigger_data->private_data = file;
-	INIT_LIST_HEAD(&trigger_data->list);
-	INIT_LIST_HEAD(&trigger_data->named_list);
-
-	if (glob[0] == '!') {
+	if (remove) {
 		cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
-		kfree(trigger_data);
 		ret = 0;
 		goto out;
 	}
 
-	if (trigger) {
-		number = strsep(&trigger, ":");
-
-		ret = -EINVAL;
-		if (!strlen(number))
-			goto out_free;
-
-		/*
-		 * We use the callback data field (which is a pointer)
-		 * as our counter.
-		 */
-		ret = kstrtoul(number, 0, &trigger_data->count);
-		if (ret)
-			goto out_free;
-	}
-
-	if (!param) /* if param is non-empty, it's supposed to be a filter */
-		goto out_reg;
-
-	if (!cmd_ops->set_filter)
-		goto out_reg;
+	ret = event_trigger_parse_num(trigger, trigger_data);
+	if (ret)
+		goto out_free;
 
-	ret = cmd_ops->set_filter(param, trigger_data, file);
+	ret = event_trigger_set_filter(cmd_ops, file, param, trigger_data);
 	if (ret < 0)
 		goto out_free;
 
- out_reg:
 	/* Up the trigger_data count to make sure reg doesn't free it on failure */
 	event_trigger_init(trigger_ops, trigger_data);
-	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
-	/*
-	 * The above returns on success the # of functions enabled,
-	 * but if it didn't find any functions it returns zero.
-	 * Consider no functions a failure too.
-	 */
-	if (!ret) {
-		cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
-		ret = -ENOENT;
-	} else if (ret > 0)
-		ret = 0;
+
+	ret = event_trigger_register(cmd_ops, file, glob, cmd, trigger, trigger_data, NULL);
+	if (ret)
+		goto out_free;
 
 	/* Down the counter of trigger_data or free it if not used anymore */
 	event_trigger_free(trigger_ops, trigger_data);
  out:
 	return ret;
-
  out_free:
-	if (cmd_ops->set_filter)
-		cmd_ops->set_filter(NULL, trigger_data, NULL);
+	event_trigger_reset_filter(cmd_ops, trigger_data);
 	kfree(trigger_data);
 	goto out;
 }
@@ -1675,26 +1633,21 @@ int event_enable_trigger_func(struct event_command *cmd_ops,
 	struct event_trigger_data *trigger_data;
 	struct event_trigger_ops *trigger_ops;
 	struct trace_array *tr = file->tr;
+	bool enable, remove;
 	const char *system;
 	const char *event;
 	bool hist = false;
 	char *trigger;
-	char *number;
-	bool enable;
 	int ret;
 
-	if (!param)
-		return -EINVAL;
+	remove = event_trigger_check_remove(glob);
 
-	/* separate the trigger from the filter (s:e:n [if filter]) */
-	trigger = strsep(&param, " \t");
-	if (!trigger)
+	if (event_trigger_empty_param(param))
 		return -EINVAL;
-	if (param) {
-		param = skip_spaces(param);
-		if (!*param)
-			param = NULL;
-	}
+
+	ret = event_trigger_separate_filter(&trigger, &param, true);
+	if (ret)
+		return ret;
 
 	system = strsep(&trigger, ":");
 	if (!trigger)
@@ -1719,28 +1672,23 @@ int event_enable_trigger_func(struct event_command *cmd_ops,
 	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
 
 	ret = -ENOMEM;
-	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
-	if (!trigger_data)
-		goto out;
-
 	enable_data = kzalloc(sizeof(*enable_data), GFP_KERNEL);
 	if (!enable_data) {
 		kfree(trigger_data);
 		goto out;
 	}
 
-	trigger_data->count = -1;
-	trigger_data->ops = trigger_ops;
-	trigger_data->cmd_ops = cmd_ops;
-	INIT_LIST_HEAD(&trigger_data->list);
-	RCU_INIT_POINTER(trigger_data->filter, NULL);
-
 	enable_data->hist = hist;
 	enable_data->enable = enable;
 	enable_data->file = event_enable_file;
-	trigger_data->private_data = enable_data;
 
-	if (glob[0] == '!') {
+	trigger_data = event_trigger_alloc(cmd_ops, cmd, trigger, enable_data);
+	if (!trigger_data) {
+		kfree(enable_data);
+		goto out;
+	}
+
+	if (remove) {
 		cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
 		kfree(trigger_data);
 		kfree(enable_data);
@@ -1751,33 +1699,14 @@ int event_enable_trigger_func(struct event_command *cmd_ops,
 	/* Up the trigger_data count to make sure nothing frees it on failure */
 	event_trigger_init(trigger_ops, trigger_data);
 
-	if (trigger) {
-		number = strsep(&trigger, ":");
-
-		ret = -EINVAL;
-		if (!strlen(number))
-			goto out_free;
-
-		/*
-		 * We use the callback data field (which is a pointer)
-		 * as our counter.
-		 */
-		ret = kstrtoul(number, 0, &trigger_data->count);
-		if (ret)
-			goto out_free;
-	}
-
-	if (!param) /* if param is non-empty, it's supposed to be a filter */
-		goto out_reg;
-
-	if (!cmd_ops->set_filter)
-		goto out_reg;
+	ret = event_trigger_parse_num(trigger, trigger_data);
+	if (ret)
+		goto out_free;
 
-	ret = cmd_ops->set_filter(param, trigger_data, file);
+	ret = event_trigger_set_filter(cmd_ops, file, param, trigger_data);
 	if (ret < 0)
 		goto out_free;
 
- out_reg:
 	/* Don't let event modules unload while probe registered */
 	ret = trace_event_try_get_ref(event_enable_file->event_call);
 	if (!ret) {
@@ -1788,30 +1717,20 @@ int event_enable_trigger_func(struct event_command *cmd_ops,
 	ret = trace_event_enable_disable(event_enable_file, 1, 1);
 	if (ret < 0)
 		goto out_put;
-	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
-	/*
-	 * The above returns on success the # of functions enabled,
-	 * but if it didn't find any functions it returns zero.
-	 * Consider no functions a failure too.
-	 */
-	if (!ret) {
-		ret = -ENOENT;
-		goto out_disable;
-	} else if (ret < 0)
+
+	ret = event_trigger_register(cmd_ops, file, glob, cmd, trigger, trigger_data, NULL);
+	if (ret)
 		goto out_disable;
-	/* Just return zero, not the number of enabled functions */
-	ret = 0;
+
 	event_trigger_free(trigger_ops, trigger_data);
  out:
 	return ret;
-
  out_disable:
 	trace_event_enable_disable(event_enable_file, 0, 1);
  out_put:
 	trace_event_put_ref(event_enable_file->event_call);
  out_free:
-	if (cmd_ops->set_filter)
-		cmd_ops->set_filter(NULL, trigger_data, NULL);
+	event_trigger_reset_filter(cmd_ops, trigger_data);
 	event_trigger_free(trigger_ops, trigger_data);
 	kfree(enable_data);
 	goto out;
-- 
2.17.1

Re: [PATCH v3 2/2] tracing: Have existing event_command implementations use helpers

[Steven Rostedt]

On Tue, 23 Nov 2021 14:53:54 -0600
Tom Zanussi <zanussi@kernel.org> wrote:


> index a873f4e8be04..1d1716d5bee2 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -931,89 +931,47 @@ event_trigger_callback(struct event_command *cmd_ops,
>  	struct event_trigger_data *trigger_data;
>  	struct event_trigger_ops *trigger_ops;
>  	char *trigger = NULL;
> -	char *number;
> +	bool remove;
>  	int ret;
>  
> -	/* separate the trigger from the filter (t:n [if filter]) */
> -	if (param && isdigit(param[0])) {
> -		trigger = strsep(&param, " \t");
> -		if (param) {
> -			param = skip_spaces(param);
> -			if (!*param)
> -				param = NULL;
> -		}
> -	}
> +	remove = event_trigger_check_remove(glob);
>  
> -	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
> +	ret = event_trigger_separate_filter(&trigger, &param, false);
> +	if (ret)
> +		return ret;
>  
>  	ret = -ENOMEM;
> -	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> +	trigger_data = event_trigger_alloc(cmd_ops, cmd, trigger, file);
>  	if (!trigger_data)
>  		goto out;
>  
> -	trigger_data->count = -1;
> -	trigger_data->ops = trigger_ops;
> -	trigger_data->cmd_ops = cmd_ops;
> -	trigger_data->private_data = file;
> -	INIT_LIST_HEAD(&trigger_data->list);
> -	INIT_LIST_HEAD(&trigger_data->named_list);
> -
> -	if (glob[0] == '!') {
> +	if (remove) {
>  		cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
> -		kfree(trigger_data);

How is trigger_data freed on error here?

-- Steve

>  		ret = 0;
>  		goto out;
>  	}
>  
> -	if (trigger) {
> -		number = strsep(&trigger, ":");
> -
> -		ret = -EINVAL;
> -		if (!strlen(number))
> -			goto out_free;
> -
> -		/*
> -		 * We use the callback data field (which is a pointer)
> -		 * as our counter.
> -		 */
> -		ret = kstrtoul(number, 0, &trigger_data->count);
> -		if (ret)
> -			goto out_free;
> -	}
> -
> -	if (!param) /* if param is non-empty, it's supposed to be a filter */
> -		goto out_reg;
> -
> -	if (!cmd_ops->set_filter)
> -		goto out_reg;
> +	ret = event_trigger_parse_num(trigger, trigger_data);
> +	if (ret)
> +		goto out_free;
>  
> -	ret = cmd_ops->set_filter(param, trigger_data, file);
> +	ret = event_trigger_set_filter(cmd_ops, file, param, trigger_data);
>  	if (ret < 0)
>  		goto out_free;
>  
> - out_reg:
>  	/* Up the trigger_data count to make sure reg doesn't free it on failure */
>  	event_trigger_init(trigger_ops, trigger_data);
> -	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> -	/*
> -	 * The above returns on success the # of functions enabled,
> -	 * but if it didn't find any functions it returns zero.
> -	 * Consider no functions a failure too.
> -	 */
> -	if (!ret) {
> -		cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
> -		ret = -ENOENT;
> -	} else if (ret > 0)
> -		ret = 0;
> +
> +	ret = event_trigger_register(cmd_ops, file, glob, cmd, trigger, trigger_data, NULL);
> +	if (ret)
> +		goto out_free;
>  
>  	/* Down the counter of trigger_data or free it if not used anymore */
>  	event_trigger_free(trigger_ops, trigger_data);
>   out:
>  	return ret;
> -
>   out_free:
> -	if (cmd_ops->set_filter)
> -		cmd_ops->set_filter(NULL, trigger_data, NULL);
> +	event_trigger_reset_filter(cmd_ops, trigger_data);
>  	kfree(trigger_data);
>  	goto out;
>  }
> 

Re: [PATCH v3 2/2] tracing: Have existing event_command implementations use helpers

[Tom Zanussi]

Hi Steve,

On Tue, 2021-11-30 at 16:31 -0500, Steven Rostedt wrote:
> On Tue, 23 Nov 2021 14:53:54 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> 
> > index a873f4e8be04..1d1716d5bee2 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -931,89 +931,47 @@ event_trigger_callback(struct event_command
> > *cmd_ops,
> >  	struct event_trigger_data *trigger_data;
> >  	struct event_trigger_ops *trigger_ops;
> >  	char *trigger = NULL;
> > -	char *number;
> > +	bool remove;
> >  	int ret;
> >  
> > -	/* separate the trigger from the filter (t:n [if filter]) */
> > -	if (param && isdigit(param[0])) {
> > -		trigger = strsep(&param, " \t");
> > -		if (param) {
> > -			param = skip_spaces(param);
> > -			if (!*param)
> > -				param = NULL;
> > -		}
> > -	}
> > +	remove = event_trigger_check_remove(glob);
> >  
> > -	trigger_ops = cmd_ops->get_trigger_ops(cmd, trigger);
> > +	ret = event_trigger_separate_filter(&trigger, &param, false);
> > +	if (ret)
> > +		return ret;
> >  
> >  	ret = -ENOMEM;
> > -	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> > +	trigger_data = event_trigger_alloc(cmd_ops, cmd, trigger,
> > file);
> >  	if (!trigger_data)
> >  		goto out;
> >  
> > -	trigger_data->count = -1;
> > -	trigger_data->ops = trigger_ops;
> > -	trigger_data->cmd_ops = cmd_ops;
> > -	trigger_data->private_data = file;
> > -	INIT_LIST_HEAD(&trigger_data->list);
> > -	INIT_LIST_HEAD(&trigger_data->named_list);
> > -
> > -	if (glob[0] == '!') {
> > +	if (remove) {
> >  		cmd_ops->unreg(glob+1, trigger_ops, trigger_data,
> > file);
> > -		kfree(trigger_data);
> 
> How is trigger_data freed on error here?

You're right, that kfree() shouldn't have been removed, will add it
back.

Thanks for picking that up,

Tom

> 
> -- Steve
> 
> >  		ret = 0;
> >  		goto out;
> >  	}
> >  
> > -	if (trigger) {
> > -		number = strsep(&trigger, ":");
> > -
> > -		ret = -EINVAL;
> > -		if (!strlen(number))
> > -			goto out_free;
> > -
> > -		/*
> > -		 * We use the callback data field (which is a pointer)
> > -		 * as our counter.
> > -		 */
> > -		ret = kstrtoul(number, 0, &trigger_data->count);
> > -		if (ret)
> > -			goto out_free;
> > -	}
> > -
> > -	if (!param) /* if param is non-empty, it's supposed to be a
> > filter */
> > -		goto out_reg;
> > -
> > -	if (!cmd_ops->set_filter)
> > -		goto out_reg;
> > +	ret = event_trigger_parse_num(trigger, trigger_data);
> > +	if (ret)
> > +		goto out_free;
> >  
> > -	ret = cmd_ops->set_filter(param, trigger_data, file);
> > +	ret = event_trigger_set_filter(cmd_ops, file, param,
> > trigger_data);
> >  	if (ret < 0)
> >  		goto out_free;
> >  
> > - out_reg:
> >  	/* Up the trigger_data count to make sure reg doesn't free it
> > on failure */
> >  	event_trigger_init(trigger_ops, trigger_data);
> > -	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> > -	/*
> > -	 * The above returns on success the # of functions enabled,
> > -	 * but if it didn't find any functions it returns zero.
> > -	 * Consider no functions a failure too.
> > -	 */
> > -	if (!ret) {
> > -		cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
> > -		ret = -ENOENT;
> > -	} else if (ret > 0)
> > -		ret = 0;
> > +
> > +	ret = event_trigger_register(cmd_ops, file, glob, cmd, trigger,
> > trigger_data, NULL);
> > +	if (ret)
> > +		goto out_free;
> >  
> >  	/* Down the counter of trigger_data or free it if not used
> > anymore */
> >  	event_trigger_free(trigger_ops, trigger_data);
> >   out:
> >  	return ret;
> > -
> >   out_free:
> > -	if (cmd_ops->set_filter)
> > -		cmd_ops->set_filter(NULL, trigger_data, NULL);
> > +	event_trigger_reset_filter(cmd_ops, trigger_data);
> >  	kfree(trigger_data);
> >  	goto out;
> >  }
> > 

Re: [PATCH v3 1/2] tracing: Add helper functions to simplify event_command callback handling

[Steven Rostedt]

On Tue, 23 Nov 2021 14:53:53 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> The event_command.func() callback is responsible for parsing and
> registering triggers.  The existing command implementions for this
> callback duplicate a lot of the same code, so to clean up and
> consolidate those implementations, introduce a handful of helper
> functions for implementors to use.
> 
> This also makes it easier for new commands to be implemented and
> allows them to focus more on the customizations they provide rather
> than obscuring and complicating it with boilerplate code.
> 
> Signed-off-by: Tom Zanussi <zanussi@kernel.org>

Thanks for the update Tom!

> ---
>  kernel/trace/trace.h                |  23 +++
>  kernel/trace/trace_events_trigger.c | 286 ++++++++++++++++++++++++++++
>  2 files changed, 309 insertions(+)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 6b60ab9475ed..66cc32aca2e8 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1594,6 +1594,29 @@ get_named_trigger_data(struct event_trigger_data *data);
>  extern int register_event_command(struct event_command *cmd);
>  extern int unregister_event_command(struct event_command *cmd);
>  extern int register_trigger_hist_enable_disable_cmds(void);
> +extern bool event_trigger_check_remove(char *cmd);
> +extern bool event_trigger_empty_param(char *param);
> +extern int event_trigger_separate_filter(char **trigger, char **param, bool param_required);
> +extern struct event_trigger_data *
> +event_trigger_alloc(struct event_command *cmd_ops,
> +		    char *cmd,
> +		    char *param,
> +		    void *private_data);
> +extern int event_trigger_parse_num(char *trigger,
> +				   struct event_trigger_data *trigger_data);
> +extern int event_trigger_set_filter(struct event_command *cmd_ops,
> +				    struct trace_event_file *file,
> +				    char *param,
> +				    struct event_trigger_data *trigger_data);
> +extern void event_trigger_reset_filter(struct event_command *cmd_ops,
> +				       struct event_trigger_data *trigger_data);
> +extern int event_trigger_register(struct event_command *cmd_ops,
> +				  struct trace_event_file *file,
> +				  char *glob,
> +				  char *cmd,
> +				  char *trigger,
> +				  struct event_trigger_data *trigger_data,
> +				  int *n_registered);
>  
>  /**
>   * struct event_trigger_ops - callbacks for trace event triggers
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 3d5c07239a2a..a873f4e8be04 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -621,6 +621,292 @@ static void unregister_trigger(char *glob, struct event_trigger_ops *ops,
>  		data->ops->free(data->ops, data);
>  }
>  
> +/*
> + * Event trigger parsing helper functions.
> + *
> + * These functions help make it easier to write an event trigger
> + * parsing function i.e. the struct event_command.func() callback
> + * function responsible for parsing and registering a trigger command
> + * written to the 'trigger' file.
> + *
> + * There are a few different categories of event trigger covered by
> + * these helpers:
> + *
> + *  - triggers that don't require a parameter e.g. traceon
> + *  - triggers that do require a parameter e.g. enable_event and hist
> + *  - triggers that though they may not require a param may support an
> + *    optional 'n' param (n = number of times the trigger should fire)
> + *    e.g.: traceon:5 or enable_event:sys:event:n
> + *  - triggers that do not support an 'n' param e.g. hist
> + *
> + * These functions can be used or ignored as necessary - it all
> + * depends on the complexity of the trigger, and the granularity of
> + * the functions supported reflects the fact that some implementations
> + * may need to customize certain aspects of their implementations and
> + * won't need certain functions.  For instance, the hist trigger
> + * implementation doesn't use event_trigger_separate_filter() because
> + * it has special requirements for handling the filter.
> + */
> +
> +/**
> + * event_trigger_check_remove - check whether an event trigger specifies remove
> + * @glob: The trigger command string, with optional remove(!) operator
> + *
> + * The event trigger callback implementations pass in 'glob' as a
> + * parameter.  This is the command name either with or without a
> + * remove(!)  operator.  This function simply parses the glob and
> + * determines whether the command corresponds to a trigger removal or
> + * a trigger addition.
> + *
> + * Return: true if this is a remove command, false otherwise
> + */
> +bool event_trigger_check_remove(char *glob)

			const char *glob ?

> +{
> +	return (glob && glob[0] == '!') ? true : false;
> +}
> +
> +/**
> + * event_trigger_empty_param - check whether the param is empty
> + * @param: The trigger param string
> + *
> + * The event trigger callback implementations pass in 'param' as a
> + * parameter.  This corresponds to the string following the command
> + * name minus the command name.  This function can be called by a
> + * callback implementation for any command that requires a param; a
> + * callback that doesn't require a param can ignore it.
> + *
> + * Return: true if this is an empty param, false otherwise
> + */
> +bool event_trigger_empty_param(char *param)

			const char *param ?
> +{
> +	return !param;
> +}
> +
> +/**
> + * event_trigger_separate_filter - separate an event trigger from a filter
> + * @trigger: outparam, will be filled with a pointer to the trigger
> + * @param: The param string, will contain the filter after this is called
> + * @param_required: Specifies whether or not the param string is required
> + *

Let's not make this more confusing then it has to be. Let's pass in the
input parameter as a separate value. Because I'm totally confused by what
is an input and what is an output.

	(char *param, char **trigger, char **filter, ..

?


> + * Given a param string of the form '[trigger] [if filter]', this
> + * function separates the filter from the trigger and returns the
> + * trigger in *trigger and the filter in *param.  Either the *trigger
> + * or the *param may be set to NULL by this function - if not set to
> + * NULL, they will contain strings corresponding to the trigger and
> + * filter.
> + *
> + * There are two cases that need to be handled with respect to the
> + * passed-in param: either the param is required, or it is not
> + * required.  If @param_required is set, and there's no param, it will
> + * return -EINVAL.  If @param_required is not set and there's a param
> + * that starts with a number, that corresponds to the case of a
> + * trigger with :n (n = number of times the trigger should fire) and
> + * the parsing continues normally; otherwise the function just returns
> + * and assumes param just contains a filter and there's nothing else
> + * to do.

I'm not sure how the number is parsed here.

Really, getting rid of having two different ops that are attached to each
other should be cleaned up first. At least change their internal names.

We have: struct event_command that has

	int			(*func)(struct event_command *cmd_ops,
					struct trace_event_file *file,
					char *glob, char *cmd, char *params);

And

	struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char *param);

Where it returns event_tigger_ops that has:

	void			(*func)(struct event_trigger_data *data,
					struct trace_buffer *buffer, void *rec,
					struct ring_buffer_event *rbe);

So when I see:  ops->func() I have no clue what func that is!

Let's first rename one of them, and that way it is easier to know what is
going on. There's a lot of magic here that is hard to follow.

> + *
> + * Return: 0 on success, errno otherwise
> + */
> +int event_trigger_separate_filter(char **trigger, char **param, bool param_required)
> +{
> +	int ret = 0;
> +
> +	*trigger = NULL;
> +
> +	if (!*param) {
> +		if (param_required)
> +			ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	/* the only legal optional param is :n */
> +	if (!param_required && *param && !isdigit(*param[0]))
> +		goto out;

How does *param[0] end up pointing to a number? If it was:

 trigger:n if filter

?

The event_trigger_parse_num() is called after this.

> +
> +	/* separate the trigger from the filter (trigger [if filter]) */
> +	*trigger = strsep(param, " \t");
> +	if (!*trigger) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (*param) {
> +		*param = skip_spaces(*param);
> +		if (!**param)
> +			*param = NULL;
> +	}
> +out:
> +	return ret;
> +}
> +
> +/**
> + * event_trigger_alloc - allocate and init event_trigger_data for a trigger
> + * @cmd_ops: The event_command operations for the trigger
> + * @cmd: The cmd string
> + * @param: The param string
> + * @private_data: User data to associate with the event trigger
> + *
> + * Allocate an event_trigger_data instance and initialize it.  The
> + * @cmd_ops are used along with the @cmd and @param to get the
> + * trigger_ops to assign to the event_trigger_data.  @private_data can
> + * also be passed in and associated with the event_trigger_data.
> + *
> + * Use event_trigger_free() to free an event_trigger_data object.
> + *
> + * Return: The trigger_data object success, NULL otherwise
> + */
> +struct event_trigger_data *event_trigger_alloc(struct event_command *cmd_ops,
> +					       char *cmd,
> +					       char *param,
> +					       void *private_data)
> +{
> +	struct event_trigger_data *trigger_data;
> +	struct event_trigger_ops *trigger_ops;
> +
> +	trigger_ops = cmd_ops->get_trigger_ops(cmd, param);
> +
> +	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> +	if (!trigger_data)
> +		return NULL;
> +
> +	trigger_data->count = -1;
> +	trigger_data->ops = trigger_ops;
> +	trigger_data->cmd_ops = cmd_ops;
> +	trigger_data->private_data = private_data;
> +
> +	INIT_LIST_HEAD(&trigger_data->list);
> +	INIT_LIST_HEAD(&trigger_data->named_list);
> +	RCU_INIT_POINTER(trigger_data->filter, NULL);
> +
> +	return trigger_data;
> +}
> +
> +/**
> + * event_trigger_parse_num - parse and return the number param for a trigger
> + * @trigger: The trigger string
> + * @trigger_data: The trigger_data for the trigger
> + *
> + * Parse the :n (n = number of times the trigger should fire) param
> + * and set the count variable in the trigger_data to the parsed count.
> + *
> + * Return: 0 on success, errno otherwise
> + */
> +int event_trigger_parse_num(char *trigger,
> +			    struct event_trigger_data *trigger_data)
> +{
> +	char *number;
> +	int ret = 0;
> +
> +	if (trigger) {
> +		number = strsep(&trigger, ":");
> +
> +		if (!strlen(number))
> +			return -EINVAL;
> +
> +		/*
> +		 * We use the callback data field (which is a pointer)
> +		 * as our counter.
> +		 */
> +		ret = kstrtoul(number, 0, &trigger_data->count);
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * event_trigger_set_filter - set an event trigger's filter
> + * @cmd_ops: The event_command operations for the trigger
> + * @file: The event file for the trigger's event
> + * @param: The string containing the filter
> + * @trigger_data: The trigger_data for the trigger
> + *
> + * Set the filter for the trigger.  If the filter is NULL, just return
> + * without error.
> + *
> + * Return: 0 on success, errno otherwise
> + */
> +int event_trigger_set_filter(struct event_command *cmd_ops,
> +			     struct trace_event_file *file,
> +			     char *param,
> +			     struct event_trigger_data *trigger_data)
> +{
> +	if (param && cmd_ops->set_filter)
> +		return cmd_ops->set_filter(param, trigger_data, file);
> +
> +	return 0;
> +}
> +
> +/**
> + * event_trigger_reset_filter - reset an event trigger's filter
> + * @cmd_ops: The event_command operations for the trigger
> + * @trigger_data: The trigger_data for the trigger
> + *
> + * Reset the filter for the trigger to no filter.
> + */
> +void event_trigger_reset_filter(struct event_command *cmd_ops,
> +				struct event_trigger_data *trigger_data)
> +{
> +	if (cmd_ops->set_filter)
> +		cmd_ops->set_filter(NULL, trigger_data, NULL);
> +}
> +
> +/**
> + * event_trigger_register - register an event trigger
> + * @cmd_ops: The event_command operations for the trigger
> + * @file: The event file for the trigger's event
> + * @glob: The trigger command string, with optional remove(!) operator
> + * @cmd: The cmd string
> + * @param: The param string
> + * @trigger_data: The trigger_data for the trigger
> + * @n_registered: optional outparam, the number of triggers registered
> + *
> + * Register and event trigger.  The @cmd_ops are used along with the

    "Register an event trigger"

-- Steve

> + * @cmd and @param to get the trigger_ops to pass to the
> + * cmd_ops->reg() function which actually does the registration. The
> + * cmd_ops->reg() function returns the number of triggers registered,
> + * which is assigned to n_registered, if n_registered is non-NULL.
> + *
> + * Return: 0 on success, errno otherwise
> + */
> +int event_trigger_register(struct event_command *cmd_ops,
> +			   struct trace_event_file *file,
> +			   char *glob,
> +			   char *cmd,
> +			   char *param,
> +			   struct event_trigger_data *trigger_data,
> +			   int *n_registered)
> +{
> +	struct event_trigger_ops *trigger_ops;
> +	int ret;
> +
> +	if (n_registered)
> +		*n_registered = 0;
> +
> +	trigger_ops = cmd_ops->get_trigger_ops(cmd, param);
> +
> +	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> +	/*
> +	 * The above returns on success the # of functions enabled,
> +	 * but if it didn't find any functions it returns zero.
> +	 * Consider no functions a failure too.
> +	 */
> +	if (!ret) {
> +		cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
> +		ret = -ENOENT;
> +	} else if (ret > 0) {
> +		if (n_registered)
> +			*n_registered = ret;
> +		/* Just return zero, not the number of enabled functions */
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * End event trigger parsing helper functions.
> + */
> +
>  /**
>   * event_trigger_callback - Generic event_command @func implementation
>   * @cmd_ops: The command ops, used for trigger registration

Re: [PATCH v3 1/2] tracing: Add helper functions to simplify event_command callback handling

[Tom Zanussi]

Hi Steve,

On Tue, 2021-11-30 at 16:46 -0500, Steven Rostedt wrote:
> On Tue, 23 Nov 2021 14:53:53 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > The event_command.func() callback is responsible for parsing and
> > registering triggers.  The existing command implementions for this
> > callback duplicate a lot of the same code, so to clean up and
> > consolidate those implementations, introduce a handful of helper
> > functions for implementors to use.
> > 
> > This also makes it easier for new commands to be implemented and
> > allows them to focus more on the customizations they provide rather
> > than obscuring and complicating it with boilerplate code.
> > 
> > Signed-off-by: Tom Zanussi <zanussi@kernel.org>
> 
> Thanks for the update Tom!
> 
> > ---
> >  kernel/trace/trace.h                |  23 +++
> >  kernel/trace/trace_events_trigger.c | 286
> > ++++++++++++++++++++++++++++
> >  2 files changed, 309 insertions(+)
> > 
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 6b60ab9475ed..66cc32aca2e8 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -1594,6 +1594,29 @@ get_named_trigger_data(struct
> > event_trigger_data *data);
> >  extern int register_event_command(struct event_command *cmd);
> >  extern int unregister_event_command(struct event_command *cmd);
> >  extern int register_trigger_hist_enable_disable_cmds(void);
> > +extern bool event_trigger_check_remove(char *cmd);
> > +extern bool event_trigger_empty_param(char *param);
> > +extern int event_trigger_separate_filter(char **trigger, char
> > **param, bool param_required);
> > +extern struct event_trigger_data *
> > +event_trigger_alloc(struct event_command *cmd_ops,
> > +		    char *cmd,
> > +		    char *param,
> > +		    void *private_data);
> > +extern int event_trigger_parse_num(char *trigger,
> > +				   struct event_trigger_data
> > *trigger_data);
> > +extern int event_trigger_set_filter(struct event_command *cmd_ops,
> > +				    struct trace_event_file *file,
> > +				    char *param,
> > +				    struct event_trigger_data
> > *trigger_data);
> > +extern void event_trigger_reset_filter(struct event_command
> > *cmd_ops,
> > +				       struct event_trigger_data
> > *trigger_data);
> > +extern int event_trigger_register(struct event_command *cmd_ops,
> > +				  struct trace_event_file *file,
> > +				  char *glob,
> > +				  char *cmd,
> > +				  char *trigger,
> > +				  struct event_trigger_data
> > *trigger_data,
> > +				  int *n_registered);
> >  
> >  /**
> >   * struct event_trigger_ops - callbacks for trace event triggers
> > diff --git a/kernel/trace/trace_events_trigger.c
> > b/kernel/trace/trace_events_trigger.c
> > index 3d5c07239a2a..a873f4e8be04 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -621,6 +621,292 @@ static void unregister_trigger(char *glob,
> > struct event_trigger_ops *ops,
> >  		data->ops->free(data->ops, data);
> >  }
> >  
> > +/*
> > + * Event trigger parsing helper functions.
> > + *
> > + * These functions help make it easier to write an event trigger
> > + * parsing function i.e. the struct event_command.func() callback
> > + * function responsible for parsing and registering a trigger
> > command
> > + * written to the 'trigger' file.
> > + *
> > + * There are a few different categories of event trigger covered
> > by
> > + * these helpers:
> > + *
> > + *  - triggers that don't require a parameter e.g. traceon
> > + *  - triggers that do require a parameter e.g. enable_event and
> > hist
> > + *  - triggers that though they may not require a param may
> > support an
> > + *    optional 'n' param (n = number of times the trigger should
> > fire)
> > + *    e.g.: traceon:5 or enable_event:sys:event:n
> > + *  - triggers that do not support an 'n' param e.g. hist
> > + *
> > + * These functions can be used or ignored as necessary - it all
> > + * depends on the complexity of the trigger, and the granularity
> > of
> > + * the functions supported reflects the fact that some
> > implementations
> > + * may need to customize certain aspects of their implementations
> > and
> > + * won't need certain functions.  For instance, the hist trigger
> > + * implementation doesn't use event_trigger_separate_filter()
> > because
> > + * it has special requirements for handling the filter.
> > + */
> > +
> > +/**
> > + * event_trigger_check_remove - check whether an event trigger
> > specifies remove
> > + * @glob: The trigger command string, with optional remove(!)
> > operator
> > + *
> > + * The event trigger callback implementations pass in 'glob' as a
> > + * parameter.  This is the command name either with or without a
> > + * remove(!)  operator.  This function simply parses the glob and
> > + * determines whether the command corresponds to a trigger removal
> > or
> > + * a trigger addition.
> > + *
> > + * Return: true if this is a remove command, false otherwise
> > + */
> > +bool event_trigger_check_remove(char *glob)
> 
> 			const char *glob ?

Yep, will change.

> 
> > +{
> > +	return (glob && glob[0] == '!') ? true : false;
> > +}
> > +
> > +/**
> > + * event_trigger_empty_param - check whether the param is empty
> > + * @param: The trigger param string
> > + *
> > + * The event trigger callback implementations pass in 'param' as a
> > + * parameter.  This corresponds to the string following the
> > command
> > + * name minus the command name.  This function can be called by a
> > + * callback implementation for any command that requires a param;
> > a
> > + * callback that doesn't require a param can ignore it.
> > + *
> > + * Return: true if this is an empty param, false otherwise
> > + */
> > +bool event_trigger_empty_param(char *param)
> 
> 			const char *param ?

Same.

> > +{
> > +	return !param;
> > +}
> > +
> > +/**
> > + * event_trigger_separate_filter - separate an event trigger from
> > a filter
> > + * @trigger: outparam, will be filled with a pointer to the
> > trigger
> > + * @param: The param string, will contain the filter after this is
> > called
> > + * @param_required: Specifies whether or not the param string is
> > required
> > + *
> 
> Let's not make this more confusing then it has to be. Let's pass in
> the
> input parameter as a separate value. Because I'm totally confused by
> what
> is an input and what is an output.
> 
> 	(char *param, char **trigger, char **filter, ..
> 
> ?
> 

Yes, this probably makes more sense, to highlight what the results are.

> 
> > + * Given a param string of the form '[trigger] [if filter]', this
> > + * function separates the filter from the trigger and returns the
> > + * trigger in *trigger and the filter in *param.  Either the
> > *trigger
> > + * or the *param may be set to NULL by this function - if not set
> > to
> > + * NULL, they will contain strings corresponding to the trigger
> > and
> > + * filter.
> > + *
> > + * There are two cases that need to be handled with respect to the
> > + * passed-in param: either the param is required, or it is not
> > + * required.  If @param_required is set, and there's no param, it
> > will
> > + * return -EINVAL.  If @param_required is not set and there's a
> > param
> > + * that starts with a number, that corresponds to the case of a
> > + * trigger with :n (n = number of times the trigger should fire)
> > and
> > + * the parsing continues normally; otherwise the function just
> > returns
> > + * and assumes param just contains a filter and there's nothing
> > else
> > + * to do.
> 
> I'm not sure how the number is parsed here.
> 
> Really, getting rid of having two different ops that are attached to
> each
> other should be cleaned up first. At least change their internal
> names.
> 
> We have: struct event_command that has
> 
> 	int			(*func)(struct event_command
> *cmd_ops,
> 					struct trace_event_file *file,
> 					char *glob, char *cmd, char
> *params);
> 
> And
> 
> 	struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char
> *param);
> 
> Where it returns event_tigger_ops that has:
> 
> 	void			(*func)(struct event_trigger_data
> *data,
> 					struct trace_buffer *buffer,
> void *rec,
> 					struct ring_buffer_event *rbe);
> 
> So when I see:  ops->func() I have no clue what func that is!
> 
> Let's first rename one of them, and that way it is easier to know
> what is
> going on. There's a lot of magic here that is hard to follow.
> 

Yeah, definitely too many generic-sounding funcs() around that don't
help at all in understanding what they actually do.  Will think of
better names.

> > + *
> > + * Return: 0 on success, errno otherwise
> > + */
> > +int event_trigger_separate_filter(char **trigger, char **param,
> > bool param_required)
> > +{
> > +	int ret = 0;
> > +
> > +	*trigger = NULL;
> > +
> > +	if (!*param) {
> > +		if (param_required)
> > +			ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	/* the only legal optional param is :n */
> > +	if (!param_required && *param && !isdigit(*param[0]))
> > +		goto out;
> 
> How does *param[0] end up pointing to a number? If it was:
> 
>  trigger:n if filter
> 
> ?
> 

Well, the param being passed around actually doesn't contain the
trigger - it's what remains after the 'trigger:' has been removed.  So
in the case of trigger:n, param starts with 'n', so this works as
usual.

> The event_trigger_parse_num() is called after this.
> 
> > +
> > +	/* separate the trigger from the filter (trigger [if filter])
> > */
> > +	*trigger = strsep(param, " \t");
> > +	if (!*trigger) {
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	if (*param) {
> > +		*param = skip_spaces(*param);
> > +		if (!**param)
> > +			*param = NULL;
> > +	}
> > +out:
> > +	return ret;
> > +}
> > +
> > +/**
> > + * event_trigger_alloc - allocate and init event_trigger_data for
> > a trigger
> > + * @cmd_ops: The event_command operations for the trigger
> > + * @cmd: The cmd string
> > + * @param: The param string
> > + * @private_data: User data to associate with the event trigger
> > + *
> > + * Allocate an event_trigger_data instance and initialize it.  The
> > + * @cmd_ops are used along with the @cmd and @param to get the
> > + * trigger_ops to assign to the event_trigger_data.  @private_data
> > can
> > + * also be passed in and associated with the event_trigger_data.
> > + *
> > + * Use event_trigger_free() to free an event_trigger_data object.
> > + *
> > + * Return: The trigger_data object success, NULL otherwise
> > + */
> > +struct event_trigger_data *event_trigger_alloc(struct
> > event_command *cmd_ops,
> > +					       char *cmd,
> > +					       char *param,
> > +					       void *private_data)
> > +{
> > +	struct event_trigger_data *trigger_data;
> > +	struct event_trigger_ops *trigger_ops;
> > +
> > +	trigger_ops = cmd_ops->get_trigger_ops(cmd, param);
> > +
> > +	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> > +	if (!trigger_data)
> > +		return NULL;
> > +
> > +	trigger_data->count = -1;
> > +	trigger_data->ops = trigger_ops;
> > +	trigger_data->cmd_ops = cmd_ops;
> > +	trigger_data->private_data = private_data;
> > +
> > +	INIT_LIST_HEAD(&trigger_data->list);
> > +	INIT_LIST_HEAD(&trigger_data->named_list);
> > +	RCU_INIT_POINTER(trigger_data->filter, NULL);
> > +
> > +	return trigger_data;
> > +}
> > +
> > +/**
> > + * event_trigger_parse_num - parse and return the number param for
> > a trigger
> > + * @trigger: The trigger string
> > + * @trigger_data: The trigger_data for the trigger
> > + *
> > + * Parse the :n (n = number of times the trigger should fire)
> > param
> > + * and set the count variable in the trigger_data to the parsed
> > count.
> > + *
> > + * Return: 0 on success, errno otherwise
> > + */
> > +int event_trigger_parse_num(char *trigger,
> > +			    struct event_trigger_data *trigger_data)
> > +{
> > +	char *number;
> > +	int ret = 0;
> > +
> > +	if (trigger) {
> > +		number = strsep(&trigger, ":");
> > +
> > +		if (!strlen(number))
> > +			return -EINVAL;
> > +
> > +		/*
> > +		 * We use the callback data field (which is a pointer)
> > +		 * as our counter.
> > +		 */
> > +		ret = kstrtoul(number, 0, &trigger_data->count);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * event_trigger_set_filter - set an event trigger's filter
> > + * @cmd_ops: The event_command operations for the trigger
> > + * @file: The event file for the trigger's event
> > + * @param: The string containing the filter
> > + * @trigger_data: The trigger_data for the trigger
> > + *
> > + * Set the filter for the trigger.  If the filter is NULL, just
> > return
> > + * without error.
> > + *
> > + * Return: 0 on success, errno otherwise
> > + */
> > +int event_trigger_set_filter(struct event_command *cmd_ops,
> > +			     struct trace_event_file *file,
> > +			     char *param,
> > +			     struct event_trigger_data *trigger_data)
> > +{
> > +	if (param && cmd_ops->set_filter)
> > +		return cmd_ops->set_filter(param, trigger_data, file);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * event_trigger_reset_filter - reset an event trigger's filter
> > + * @cmd_ops: The event_command operations for the trigger
> > + * @trigger_data: The trigger_data for the trigger
> > + *
> > + * Reset the filter for the trigger to no filter.
> > + */
> > +void event_trigger_reset_filter(struct event_command *cmd_ops,
> > +				struct event_trigger_data
> > *trigger_data)
> > +{
> > +	if (cmd_ops->set_filter)
> > +		cmd_ops->set_filter(NULL, trigger_data, NULL);
> > +}
> > +
> > +/**
> > + * event_trigger_register - register an event trigger
> > + * @cmd_ops: The event_command operations for the trigger
> > + * @file: The event file for the trigger's event
> > + * @glob: The trigger command string, with optional remove(!)
> > operator
> > + * @cmd: The cmd string
> > + * @param: The param string
> > + * @trigger_data: The trigger_data for the trigger
> > + * @n_registered: optional outparam, the number of triggers
> > registered
> > + *
> > + * Register and event trigger.  The @cmd_ops are used along with
> > the
> 
>     "Register an event trigger"

Doh!  Yeah, thanks for picking that up.

I'll create a new version with all these changes, thanks for your
suggestions!

Tom


> 
> -- Steve
> 
> > + * @cmd and @param to get the trigger_ops to pass to the
> > + * cmd_ops->reg() function which actually does the registration.
> > The
> > + * cmd_ops->reg() function returns the number of triggers
> > registered,
> > + * which is assigned to n_registered, if n_registered is non-NULL.
> > + *
> > + * Return: 0 on success, errno otherwise
> > + */
> > +int event_trigger_register(struct event_command *cmd_ops,
> > +			   struct trace_event_file *file,
> > +			   char *glob,
> > +			   char *cmd,
> > +			   char *param,
> > +			   struct event_trigger_data *trigger_data,
> > +			   int *n_registered)
> > +{
> > +	struct event_trigger_ops *trigger_ops;
> > +	int ret;
> > +
> > +	if (n_registered)
> > +		*n_registered = 0;
> > +
> > +	trigger_ops = cmd_ops->get_trigger_ops(cmd, param);
> > +
> > +	ret = cmd_ops->reg(glob, trigger_ops, trigger_data, file);
> > +	/*
> > +	 * The above returns on success the # of functions enabled,
> > +	 * but if it didn't find any functions it returns zero.
> > +	 * Consider no functions a failure too.
> > +	 */
> > +	if (!ret) {
> > +		cmd_ops->unreg(glob, trigger_ops, trigger_data, file);
> > +		ret = -ENOENT;
> > +	} else if (ret > 0) {
> > +		if (n_registered)
> > +			*n_registered = ret;
> > +		/* Just return zero, not the number of enabled
> > functions */
> > +		ret = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * End event trigger parsing helper functions.
> > + */
> > +
> >  /**
> >   * event_trigger_callback - Generic event_command @func
> > implementation
> >   * @cmd_ops: The command ops, used for trigger registration
> 
>