Hi Namhyung,

On Fri, 2018-08-10 at 15:58 +0900, Namhyung Kim wrote:

Hi Tom,

On Thu, Aug 09, 2018 at 09:34:11AM -0500, Tom Zanussi wrote:

quoted

From: Tom Zanussi <redacted>

The hist trigger action code currently implements two essentially
hard-coded pairs of 'actions' - onmax(), which tracks a variable
and
saves some event fields when a max is hit, and onmatch(), which is
hard-coded to generate a synthetic event.

These hardcoded pairs (track max/save fields and detect
match/generate
synthetic event) should really be decoupled into separate
components
that can then be arbitrarily combined.  The first component of each
pair (track max/detect match) is called a 'handler' in the new
code,
while the second component (save fields/generate synthetic event)
is
called an 'action' in this scheme.

This change refactors the action code to reflect this split by
adding
two handlers, HANDLER_ONMATCH and HANDLER_ONMAX, along with two
actions, ACTION_SAVE and ACTION_TRACE.

The new code combines them to produce the existing ONMATCH/TRACE
and
ONMAX/SAVE functionality, but doesn't implement the other
combinations
now possible.  Future patches will expand these to further useful
cases, such as ONMAX/TRACE, as well as add additional handlers and
actions such as ONCHANGE and SNAPSHOT.

Signed-off-by: Tom Zanussi <redacted>
---

[SNIP]

quoted

@@ -3421,10 +3419,69 @@ static int parse_action_params(char

*params, struct action_data *data)
 	return ret;
 }
 
-static struct action_data *onmax_parse(char *str)
+static int action_parse(char *str, struct action_data *data,
+			enum handler_id handler)
+{
+	char *action_name;
+	int ret = 0;
+
+	strsep(&str, ".");
+	if (!str) {
+		hist_err("action parsing: No action found", "");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	action_name = strsep(&str, "(");
+	if (!action_name || !str) {
+		hist_err("action parsing: No action found", "");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (strncmp(action_name, "save", strlen("save")) == 0) {
+		char *params = strsep(&str, ")");
+
+		if (!params) {
+			hist_err("action parsing: No params found
for %s", "save");
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = parse_action_params(params, data);
+		if (ret)
+			goto out;
+
+		if (handler == HANDLER_ONMAX)
+			data->fn = onmax_save;
+
+		data->action = ACTION_SAVE;
+	} else {
+		char *params = strsep(&str, ")");
+
+		if (params) {
+			ret = parse_action_params(params, data);
+			if (ret)
+				goto out;
+		}
+
+		data->fn = action_trace;
+		data->action = ACTION_TRACE;

This will set action name as (synthetic) event name, right?  I think

Right.

it's natural to have "trace" for it with this change.  Maybe it's
time
to change the syntax like below?

  onmatch(SYS.EVENT).trace(EVENT, FIELD, ...)

Of course it should support old syntax too..

Yeah, I can easily add this variant while keeping the original syntax. 
Will do that in v4.

Thanks,

Tom