Hi Tom,

On Fri, Sep 22, 2017 at 02:59:52PM -0500, Tom Zanussi wrote:

Add support for saving the value of a current event's event field by
assigning it to a variable that can be read by a subsequent event.

The basic syntax for saving a variable is to simply prefix a unique
variable name not corresponding to any keyword along with an '=' sign
to any event field.

Both keys and values can be saved and retrieved in this way:

    # echo 'hist:keys=next_pid:vals=$ts0:ts0=common_timestamp ...

The 'common_timestamp' needs a '$' prefix, right?

    # echo 'hist:timer_pid=common_pid:key=$timer_pid ...'

If a variable isn't a key variable or prefixed with 'vals=', the
associated event field will be saved in a variable but won't be summed
as a value:

    # echo 'hist:keys=next_pid:ts1=common_timestamp:...

Ditto.

Multiple variables can be assigned at the same time:

    # echo 'hist:keys=pid:vals=$ts0,$b,field2:ts0=common_timestamp,b=field1 ...

Multiple (or single) variables can also be assigned at the same time
using separate assignments:

    # echo 'hist:keys=pid:vals=$ts0:ts0=common_timestamp:b=field1:c=field2 ...

Same here..

So you separated the variable definition and it should not be a part
of key or value field, right?

Variables set as above can be used by being referenced from another
event, as described in a subsequent patch.

That means, a variable should be unique (in a tracing instance at least).

And then I'm not sure what _VAR_ONLY flag is for.  IIUC it's used to
identify pure variable definition from a definition in value fields.
But it's not possible anymore, no?

+};
+
+struct var_defs {
+	unsigned int	n_vars;
+	char		*name[TRACING_MAP_VARS_MAX];
+	char		*expr[TRACING_MAP_VARS_MAX];
 };

[SNIP]

+static int create_var_field(struct hist_trigger_data *hist_data,
+			    unsigned int val_idx,
+			    struct trace_event_file *file,
+			    char *var_name, char *expr_str)
+{
+	unsigned long flags = 0;
+
+	if (WARN_ON(val_idx >= TRACING_MAP_VALS_MAX + TRACING_MAP_VARS_MAX))
+		return -EINVAL;
+
+	if (find_var(file, var_name) && !hist_data->remove) {

Is this for the uniqueness check?  But I think it only checks
variables in the current event (file).

+		return -EINVAL;
+	}
+
+	flags |= HIST_FIELD_FL_VAR;
+	hist_data->n_vars++;
+	if (hist_data->n_vars > TRACING_MAP_VARS_MAX) {
+		return -EINVAL;
+	}
+
+	flags |= HIST_FIELD_FL_VAR_ONLY;
+
+	return __create_val_field(hist_data, val_idx, file, var_name, expr_str, flags);
+}
+

[SNIP]

+static int parse_var_defs(struct hist_trigger_data *hist_data)
+{
+	char *s, *str, *var_name, *field_str;
+	unsigned int i, j, n_vars = 0;
+	int ret = 0;
+
+	for (i = 0; i < hist_data->attrs->n_assignments; i++) {
+		str = hist_data->attrs->assignment_str[i];
+		for (j = 0; j < TRACING_MAP_VARS_MAX; j++) {
+			field_str = strsep(&str, ",");
+			if (!field_str)
+				break;
+
+			var_name = strsep(&field_str, "=");
+			if (!var_name || !field_str) {
+				ret = -EINVAL;
+				goto free;
+			}
+
+			s = kstrdup(var_name, GFP_KERNEL);
+			if (!s) {
+				ret = -ENOMEM;
+				goto free;
+			}
+			hist_data->attrs->var_defs.name[n_vars] = s;
+
+			s = kstrdup(field_str, GFP_KERNEL);
+			if (!s) {
+				ret = -ENOMEM;
+				goto free;

It seems that it might leak the copy of var_name here..

Thanks,
Namhyung

+			}
+			hist_data->attrs->var_defs.expr[n_vars++] = s;
+
+			hist_data->attrs->var_defs.n_vars = n_vars;
+
+			if (n_vars == TRACING_MAP_VARS_MAX)
+				goto free;
+		}
+	}
+
+	return ret;
+ free:
+	free_var_defs(hist_data);
+
+	return ret;
+}
+