[PATCH] libtracefs: Add "precedence" to tracefs_synth_add_start/end_filter()

[PATCH] libtracefs: Add "precedence" to tracefs_synth_add_start/end_filter()

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

While implementing the sqlhist, I found that the
tracefs_synth_add_start/end_filter() was not sufficient in adding the
possible filters that the kernel accepts. That is, we could not implement:

  A && (B || C) && D

as it only allowed appending operators, and the kernel sets && as a higher
precedence than ||. That is, if we try to do the above with just:

  A && B || C && D

The kernel will interpret it as:

  (A && B) || (C && D)

To fix this, add a "precedence" that will allow the user to add
"parenthesis" to the logic:

  #define AND 0
  #define OR 1

 1. tracefs_synth_add_start( A, 0, AND);
 2. tracefs_synth_add_start( B, 1, AND);
 3. tracefs_synth_add_start( C, 1, OR);
 4. tracefs_synth_add_start( D, 0, AND);

Will produce the A && (B || C) && D

Because 1, will add A (AND is ignored) and save precedence '0'
        2, will add B, but precedence is 1, so 1 '(' is added
        3, will add C, with same precedence as step 2.
        4, will add D, but since the precedence dropped, it adds
            a ')' before the "&&"

The "neg" is given a precedence too, to state where to add the "!"

(note, the original code had "-" for negative, which was wrong)

To produce: (A || !(B && !C)) && D

   This time with (compare, neg, precedence, do_or)

 1. tracefs_synth_add_start( A, 0, 1, AND);
 2. tracefs_synth_add_start( B, 2, 2, OR);
 3. tracefs_synth_add_start( C, 1, 2, AND);
 4. tracefs_synth_add_start( D, 0, 0, AND);

Also, rename the parameter in tracefs from "or" to "or_conj" as "or" is a
keyword in C++ as reported by Yordan Karadzhov:

  Link: https://lore.kernel.org/linux-trace-devel/20210727135030.25914-1-y.karadz@gmail.com/

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/tracefs.h  |   8 +-
 src/tracefs-hist.c | 206 +++++++++++++++++++++++++--------------------
 2 files changed, 117 insertions(+), 97 deletions(-)

diff --git a/include/tracefs.h b/include/tracefs.h
index 9cfd257..25d99f8 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -354,13 +354,13 @@ int tracefs_synth_add_end_field(struct tracefs_synth *synth,
 int tracefs_synth_add_start_filter(struct tracefs_synth *synth,
 				   const char *field,
 				   enum tracefs_synth_compare compare,
-				   const char *val,
-				   bool neg, bool or);
+				   const char *val, unsigned int neg,
+				   unsigned int precedence, bool or_conj);
 int tracefs_synth_add_end_filter(struct tracefs_synth *synth,
 				 const char *field,
 				 enum tracefs_synth_compare compare,
-				 const char *val,
-				 bool neg, bool or);
+				 const char *val, unsigned int neg,
+				 unsigned int precedence, bool or_conj);
 int tracefs_synth_create(struct tracefs_instance *instance,
 			 struct tracefs_synth *synth);
 int tracefs_synth_destroy(struct tracefs_instance *instance,
diff --git a/src/tracefs-hist.c b/src/tracefs-hist.c
index 8b90787..6313487 100644
--- a/src/tracefs-hist.c
+++ b/src/tracefs-hist.c
@@ -559,7 +559,8 @@ struct tracefs_synth {
 	char			**end_vars;
 	char			*start_filter;
 	char			*end_filter;
-
+	int			start_precedence;
+	int			end_precedence;
 	int			arg_cnt;
 };
 
@@ -1166,100 +1167,112 @@ int tracefs_synth_add_end_field(struct tracefs_synth *synth,
 static int add_synth_filter(char **filter, const char *field,
 			    enum tracefs_synth_compare compare,
 			    const char *val, bool is_string,
-			    bool neg, bool or)
+			    int neg, int precedence, bool or)
 {
-	const char *minus = "";
-	const char *op;
-	char *str = NULL;
-	int ret;
-
-	switch (compare) {
-	case TRACEFS_COMPARE_EQ:
-		op = "==";
-		break;
+	struct trace_seq seq;
+	int i;
 
-	case TRACEFS_COMPARE_NQ:
-		op = "!=";
-		break;
+	trace_seq_init(&seq);
 
-	case TRACEFS_COMPARE_GR:
-		op = ">";
-		if (is_string)
-			goto inval;
-		break;
+	if (*filter) {
+		trace_seq_puts(&seq, *filter);
+
+		if (precedence < 0) {
+			for (i = 0; i > precedence; i--)
+				trace_seq_putc(&seq, ')');
+			/* neg adds new parenthesis. */
+			if (neg)
+				precedence = 1;
+		}
 
-	case TRACEFS_COMPARE_GE:
-		op = ">=";
-		if (is_string)
-			goto inval;
-		break;
+		if (or)
+			trace_seq_puts(&seq, "||");
+		else
+			trace_seq_puts(&seq, "&&");
+	}
 
-	case TRACEFS_COMPARE_LT:
-		op = "<";
-		if (is_string)
-			goto inval;
-		break;
+	for (i = 0; i < precedence; i++) {
+		if (i + 1 == neg)
+			trace_seq_putc(&seq, '!');
+		trace_seq_putc(&seq, '(');
+	}
 
-	case TRACEFS_COMPARE_LE:
-		op = "<=";
-		if (is_string)
-			goto inval;
-		break;
+	trace_seq_puts(&seq, field);
 
+	switch (compare) {
+	case TRACEFS_COMPARE_EQ: trace_seq_puts(&seq, " == "); break;
+	case TRACEFS_COMPARE_NQ: trace_seq_puts(&seq, " != "); break;
 	case TRACEFS_COMPARE_RE:
-		op = "~";
 		if (!is_string)
 			goto inval;
+		trace_seq_putc(&seq, '~');
 		break;
-
-	case TRACEFS_COMPARE_AND:
-		op = "&";
+	default:
 		if (is_string)
 			goto inval;
-		break;
 	}
 
-	if (neg)
-		minus = "-";
+	switch (compare) {
+	case TRACEFS_COMPARE_GR: trace_seq_puts(&seq, " > "); break;
+	case TRACEFS_COMPARE_GE: trace_seq_puts(&seq, " >= "); break;
+	case TRACEFS_COMPARE_LT: trace_seq_puts(&seq, " < "); break;
+	case TRACEFS_COMPARE_LE: trace_seq_puts(&seq, " <= "); break;
+	case TRACEFS_COMPARE_AND: trace_seq_puts(&seq, " & "); break;
+	default: break;
+	}
 
-	if (is_string && val[0] != '"')
-		ret = asprintf(&str, "%s(%s %s \"%s\")",
-			       minus, field, op, val);
-	else
-		ret = asprintf(&str, "%s(%s %s %s)",
-			       minus, field, op, val);
+	trace_seq_puts(&seq, val);
 
-	if (ret < 0)
+	trace_seq_terminate(&seq);
+
+	if (seq.state != TRACE_SEQ__GOOD) {
+		errno = ENOMEM;
+		trace_seq_destroy(&seq);
 		return -1;
+	}
 
-	if (*filter) {
-		char *new;
-		char *conjunction = or ? "||" : "&&";
+	free(*filter);
 
-		ret = asprintf(&new, "%s %s %s", *filter,
-			       conjunction, str);
-		free(str);
-		if (ret < 0)
-			return -1;
-		free(*filter);
-		*filter = new;
-	} else {
-		*filter = str;
-	}
+	*filter = strdup(seq.buffer);
+	trace_seq_destroy(&seq);
 
-	return 0;
+	return *filter != NULL;
 inval:
 	errno = -EINVAL;
 	return -1;
 }
 
+static void update_precedence(int *saved, int *precedence, int *neg)
+{
+	int cur = *saved;
+
+	*saved = *precedence;
+
+	/*
+	 * neg only makes sense at precedence or between
+	 * cur and precedence.
+	 */
+	if (*neg && (*neg < cur || *neg > *precedence))
+		*neg = 1;
+	else if (*neg)
+		*neg -= cur;
+
+	*precedence -= cur;
+
+	/* A negative adds parenthesis around the compare */
+	if (*neg && *precedence <= 0)
+		(*precedence)++;
+}
+
 int tracefs_synth_add_start_filter(struct tracefs_synth *synth,
 				   const char *field,
 				   enum tracefs_synth_compare compare,
-				   const char *val,
-				   bool neg, bool or)
+				   const char *val, unsigned int uneg,
+				   unsigned int uprecedence, bool or)
 {
 	const struct tep_format_field *start_field;
+	int precedence = uprecedence;
+	int neg = uneg;
 	bool is_string;
 
 	if (!field || !val)
@@ -1274,9 +1287,11 @@ int tracefs_synth_add_start_filter(struct tracefs_synth *synth,
 	if (!is_string && (start_field->flags & TEP_FIELD_IS_ARRAY))
 		goto inval;
 
+	update_precedence(&synth->start_precedence, &precedence, &neg);
+
 	return add_synth_filter(&synth->start_filter,
 				field, compare, val, is_string,
-				neg, or);
+				neg, precedence, or);
 inval:
 	errno = -EINVAL;
 	return -1;
@@ -1285,10 +1300,12 @@ inval:
 int tracefs_synth_add_end_filter(struct tracefs_synth *synth,
 				 const char *field,
 				 enum tracefs_synth_compare compare,
-				 const char *val,
-				 bool neg, bool or)
+				 const char *val, unsigned int uneg,
+				 unsigned int uprecedence, bool or)
 {
 	const struct tep_format_field *end_field;
+	int precedence = uprecedence;
+	int neg = uneg;
 	bool is_string;
 
 	if (!field || !val)
@@ -1303,9 +1320,11 @@ int tracefs_synth_add_end_filter(struct tracefs_synth *synth,
 	if (!is_string && (end_field->flags & TEP_FIELD_IS_ARRAY))
 		goto inval;
 
+	update_precedence(&synth->end_precedence, &precedence, &neg);
+
 	return add_synth_filter(&synth->end_filter,
 				field, compare, val, is_string,
-				neg, or);
+				neg, precedence, or);
 inval:
 	errno = -EINVAL;
 	return -1;
@@ -1414,6 +1433,20 @@ static char *create_end_hist(struct tracefs_synth *synth)
 	return append_string(end_hist, NULL, ")");
 }
 
+static char *append_filter(char *hist, char *filter, int precedence)
+{
+	int i;
+
+	if (!filter)
+		return hist;
+
+	hist = append_string(hist, NULL, " if ");
+	hist = append_string(hist, NULL, filter);
+	for (i = 0; i < precedence; i++)
+		hist = append_string(hist, NULL, ")");
+	return hist;
+}
+
 /**
  * tracefs_synth_create - creates the synthetic event on the system
  * @instance: The instance to modify the start and end events
@@ -1451,18 +1484,14 @@ int tracefs_synth_create(struct tracefs_instance *instance,
 		goto free_synthetic;
 
 	start_hist = create_hist(synth->start_keys, synth->start_vars);
-	if (synth->start_filter) {
-		start_hist = append_string(start_hist, NULL, " if ");
-		start_hist = append_string(start_hist, NULL, synth->start_filter);
-	}
+	start_hist = append_filter(start_hist, synth->start_filter,
+				   synth->start_precedence);
 	if (!start_hist)
 		goto remove_synthetic;
 
 	end_hist = create_end_hist(synth);
-	if (synth->end_filter) {
-		end_hist = append_string(end_hist, NULL, " if ");
-		end_hist = append_string(end_hist, NULL, synth->end_filter);
-	}
+	end_hist = append_filter(end_hist, synth->end_filter,
+				   synth->end_precedence);
 	if (!end_hist)
 		goto remove_synthetic;
 
@@ -1528,20 +1557,16 @@ int tracefs_synth_destroy(struct tracefs_instance *instance,
 	tracefs_event_disable(instance, "synthetic", synth->name);
 
 	hist = create_end_hist(synth);
-	if (synth->end_filter) {
-		hist = append_string(hist, NULL, " if ");
-		hist = append_string(hist, NULL, synth->end_filter);
-	}
+	hist = append_filter(hist, synth->end_filter,
+			     synth->end_precedence);
 	if (!hist)
 		return -1;
 	ret = remove_hist(instance, synth->end_event, hist);
 	free(hist);
 
 	hist = create_hist(synth->start_keys, synth->start_vars);
-	if (synth->start_filter) {
-		hist = append_string(hist, NULL, " if ");
-		hist = append_string(hist, NULL, synth->start_filter);
-	}
+	hist = append_filter(hist, synth->start_filter,
+			     synth->start_precedence);
 	if (!hist)
 		return -1;
 
@@ -1599,10 +1624,8 @@ int tracefs_synth_show(struct trace_seq *seq,
 	path = tracefs_instance_get_dir(instance);
 
 	hist = create_hist(synth->start_keys, synth->start_vars);
-	if (synth->start_filter) {
-		hist = append_string(hist, NULL, " if ");
-		hist = append_string(hist, NULL, synth->start_filter);
-	}
+	hist = append_filter(hist, synth->start_filter,
+			     synth->start_precedence);
 	if (!hist)
 		goto out_free;
 
@@ -1611,11 +1634,8 @@ int tracefs_synth_show(struct trace_seq *seq,
 			 synth->start_event->name);
 	free(hist);
 	hist = create_end_hist(synth);
-
-	if (synth->end_filter) {
-		hist = append_string(hist, NULL, " if ");
-		hist = append_string(hist, NULL, synth->end_filter);
-	}
+	hist = append_filter(hist, synth->end_filter,
+			     synth->end_precedence);
 	if (!hist)
 		goto out_free;
 
-- 
2.31.1