[PATCH v2 00/11] libtraceeval: Even more updates!

[PATCH v2 00/11] libtraceeval: Even more updates!

[Steven Rostedt]

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

More updates to libtraceeval

- As the results and stats are often needed in the iterator loops, add helper
  functions to quickly retrieve them. The iterator has a handle on the current
  entry, no need to do a key lookup to find it.

- The iterator can be used to search for items to remove or update. Add a
  helper function to remove the current iterator item. This will also update
  the iterator so that other functions called are still safe, even if the removed
  entry is the current one.

- Convert traceeval_data into a proper structure with a "type" field and a
  union data portion. This will allow for checking the type of data being
  passed into the functions to make sure they match their corresponding types.

- Add checks to make sure that the data matches the types.

- Make traceeval_insert() and traceeval_query() into macros and add a
  traceeval_query_size() and traceeval_insert_size() that take the size of the
  list of data being passed into it. The macros work when the arrays are defined.

- Add checks to make sure the array sizes passed in match the sizes internally.

- Add back the STATS flag, and only do STATS on values that are numeric and
  have this set. This should speed things up, and also allow for optimizations
  in the future.

Changes since v1: https://lore.kernel.org/linux-trace-devel/20230817222422.118568-1-rostedt@goodmis.org/

- Use of ARRAY_SIZE() macro to make some other macro logic simpler.

- Fix TRACEEVAL_FL_STAT (was a duplicate of 3 not 4) (Ross Zwisler)

- Add traceeval_remove_size() to check traceeval_remove()

- Add traceeval_query_size() to check traceeval_query()

- Add traceeval_stat_size() to check traceeval_stat()

- Add traceeval_iterator_results_release()

- Use TRACEEVAL_SET_NUMBER() on keys in task-eval.c (Ross Zwisler)

- Use is_stat_type() in copy_traceeval_data() (Ross Zwisler)

- Add type test to update_entry() (Ross Zwisler)

- Folded checking patch into size check patches (Ross Zwisler)

- Free entry on traceeval_remove() and trace_iterator_remove (Ross Zwisler)

- Fix added tab of return -1 (Ross Zwisler)

- Fix comment of traceeval_iterator_query() (Ross Zwisler)

- Fix comment of traceeval_iterator_remove() (Ross Zwisler)


Steven Rostedt (Google) (11):
  libtraceeval: Add check for updates to know to recreate iter array
  libtraceeval: Add traceeval_iterator_query()
  libtraceeval: Add traceeval_iterator_results_release()
  libtraceeval: Add traceeval_iterator_stat()
  libtraceeval: Add traceeval_iterator_remove()
  libtraceeval histogram: Add type to traceeval_data and make it a
    structure
  libtraceveal: Add type checks to traceeval_data vals and keys
  libtraceeval: Add size checks to insert and query functions
  libtraceeval: Make traceeval_remove() check size of keys array
  libtraceeval: Make traceeval_stat() check size of keys array
  libtraceeval: Only do stats on values marked with the STAT flag

 include/traceeval-hist.h | 118 +++++++++----
 samples/task-eval.c      | 148 +++++++----------
 src/eval-local.h         |   6 +-
 src/histograms.c         | 349 +++++++++++++++++++++++++++++++--------
 4 files changed, 429 insertions(+), 192 deletions(-)

-- 
2.40.1

[PATCH v2 01/11] libtraceeval: Add check for updates to know to recreate iter array

[Steven Rostedt]

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

When an iterator is created, it creates an array of pointers to point to
all the elements in the traceeval. This is used to index through the
entities in an nice order. But if an event is added or removed from the
traceeval, the size and count of this array will be off in the iterator.

Add an "update_counter" that gets incremented every time an item is added
or removed (doesn't need to keep track of updates to existing entries). If
the counter is different from the last time the iterator created the sort
array, it will need to delete and recreate the list again before it can do
a sort.

Note: It is safe to use the iterator to remove items, so a removal (or
even insert) should not affect the traceeval_iterator_next(). But it
should be explained in the man pages (soon to be written) that doing so
must be done with care. And maybe a helper function should be used
instead!

Reviewed-by: Ross Zwisler <zwisler@google.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 src/eval-local.h |  2 ++
 src/histograms.c | 91 ++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 74 insertions(+), 19 deletions(-)

diff --git a/src/eval-local.h b/src/eval-local.h
index 5c3918f17cc1..73f52efdf0da 100644
--- a/src/eval-local.h
+++ b/src/eval-local.h
@@ -68,6 +68,7 @@ struct traceeval {
 	struct hash_table		*hist;
 	size_t				nr_key_types;
 	size_t				nr_val_types;
+	size_t				update_counter;
 };
 
 struct traceeval_iterator {
@@ -75,6 +76,7 @@ struct traceeval_iterator {
 	struct entry			**entries;
 	struct traceeval_type		**sort;
 	bool				*direction;
+	size_t				update_counter;
 	size_t				nr_entries;
 	size_t				nr_sort;
 	size_t				next;
diff --git a/src/histograms.c b/src/histograms.c
index fdc9f0c2fbce..9bf20e4d6d26 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -757,6 +757,8 @@ static int create_entry(struct traceeval *teval,
 	entry->keys = new_keys;
 	entry->vals = new_vals;
 
+	teval->update_counter++;
+
 	return 0;
 
 fail:
@@ -960,6 +962,9 @@ int traceeval_remove(struct traceeval *teval,
 		return check;
 
 	hash_remove(hist, &entry->hash);
+
+	teval->update_counter++;
+
 	return 1;
 }
 
@@ -981,6 +986,37 @@ void traceeval_iterator_put(struct traceeval_iterator *iter)
 	free(iter);
 }
 
+static int create_iter_array(struct traceeval_iterator *iter)
+{
+	struct traceeval *teval = iter->teval;
+	struct hash_table *hist = teval->hist;
+	struct hash_iter *hiter;
+	struct hash_item *item;
+	int i;
+
+	iter->nr_entries = hash_nr_items(hist);
+	iter->entries = calloc(iter->nr_entries, sizeof(*iter->entries));
+	if (!iter->entries)
+		return -1;
+
+	for (i = 0, hiter = hash_iter_start(hist); (item = hash_iter_next(hiter)); i++) {
+		struct entry *entry = container_of(item, struct entry, hash);
+
+		iter->entries[i] = entry;
+	}
+
+	/* Loop must match entries */
+	if (i != iter->nr_entries) {
+		free(iter->entries);
+		iter->entries = NULL;
+		return -1;
+	}
+
+	iter->update_counter = teval->update_counter;
+
+	return 0;
+}
+
 /**
  * traceeval_iterator_get - get a handle to iterate over a given traceeval
  * @teval: The traceeval handle to iterate over
@@ -997,33 +1033,19 @@ void traceeval_iterator_put(struct traceeval_iterator *iter)
 struct traceeval_iterator *traceeval_iterator_get(struct traceeval *teval)
 {
 	struct traceeval_iterator *iter;
-	struct hash_table *hist = teval->hist;
-	struct hash_iter *hiter;
-	struct hash_item *item;
-	int i;
+	int ret;
 
 	iter = calloc(1, sizeof(*iter));
 	if (!iter)
 		return NULL;
 
 	iter->teval = teval;
-	iter->nr_entries = hash_nr_items(hist);
-	iter->entries = calloc(iter->nr_entries, sizeof(*iter->entries));
-	if (!iter->entries) {
-		free(iter);
-		return NULL;
-	}
-
-	for (i = 0, hiter = hash_iter_start(hist); (item = hash_iter_next(hiter)); i++) {
-		struct entry *entry = container_of(item, struct entry, hash);
 
-		iter->entries[i] = entry;
-	}
+	ret = create_iter_array(iter);
 
-	/* Loop must match entries */
-	if (i != iter->nr_entries) {
-		traceeval_iterator_put(iter);
-		return NULL;
+	if (ret < 0) {
+		free(iter);
+		iter = NULL;
 	}
 
 	return iter;
@@ -1168,6 +1190,31 @@ static int iter_cmp(const void *A, const void *B, void *data)
 	return 0;
 }
 
+static int check_update(struct traceeval_iterator *iter)
+{
+	struct entry **entries;
+	size_t nr_entries;
+	int ret;
+
+	/* Was something added or removed from the teval? */
+	if (iter->teval->update_counter == iter->update_counter)
+		return 0;
+
+	entries = iter->entries;
+	nr_entries = iter->nr_entries;
+
+	/* Something changed, need to recreate the array */
+	ret = create_iter_array(iter);
+	if (ret < 0) {
+		iter->entries = entries;
+		iter->nr_entries = nr_entries;
+		return -1;
+	}
+	free(entries);
+
+	return 0;
+}
+
 static int sort_iter(struct traceeval_iterator *iter)
 {
 	int i;
@@ -1178,6 +1225,9 @@ static int sort_iter(struct traceeval_iterator *iter)
 			return -1;
 	}
 
+	if (check_update(iter) < 0)
+		return -1;
+
 	qsort_r(iter->entries, iter->nr_entries, sizeof(*iter->entries),
 		iter_cmp, iter);
 
@@ -1214,6 +1264,9 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
 		.data = data
 	};
 
+	if (check_update(iter) < 0)
+		return -1;
+
 	qsort_r(iter->entries, iter->nr_entries, sizeof(*iter->entries),
 		iter_custom_cmp, &cust_data);
 
-- 
2.40.1

[PATCH v2 02/11] libtraceeval: Add traceeval_iterator_query()

[Steven Rostedt]

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

Add a query to the iterator that will return the values of the last entry
that matches the keys returned by the last call of traceeval_iterator_next().

Reviewed-by: Ross Zwisler <zwisler@google.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  2 ++
 samples/task-eval.c      |  2 +-
 src/histograms.c         | 30 ++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 0cee9bcbeb83..837a74f61a66 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -186,5 +186,7 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
 				   traceeval_cmp_fn sort_fn, void *data);
 int traceeval_iterator_next(struct traceeval_iterator *iter,
 			    const union traceeval_data **keys);
+int traceeval_iterator_query(struct traceeval_iterator *iter,
+			     const union traceeval_data **results);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/samples/task-eval.c b/samples/task-eval.c
index 66d0c40dc0c8..18e07f44e11e 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -836,7 +836,7 @@ static void display_processes(struct traceeval *teval,
 		struct process_data *pdata = NULL;
 		const char *comm = keys[0].cstring;
 
-		ret = traceeval_query(teval_data, keys, &results);
+		ret = traceeval_iterator_query(iter, &results);
 		if (ret < 0)
 			pdie("Could not query iterator");
 		if (ret < 1)
diff --git a/src/histograms.c b/src/histograms.c
index 9bf20e4d6d26..06613a8933ec 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -1306,3 +1306,33 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
 	*keys = entry->keys;
 	return 1;
 }
+
+/**
+ * traceeval_iterator_query - return the results from the current entry in the iterator
+ * @iter: The iterator to retrieve the entry results from
+ * @results: The returned results of the last entry (if exists)
+ *
+ * This returns the @results of the values from the last instance of
+ * traceeval_iterator_next(). It is equivalent of calling:
+ *
+ * traceeval_query() with the keys returned by traceeval_iterator_next().
+ *
+ * Except that it will always quickly return the current entry, whereas the
+ * traceeval_query() will reset the cached next_entry and do a full
+ * lookup again.
+ *
+ * Returns 1 if another entry is returned, or 0 if not (or negative on error)
+ */
+int traceeval_iterator_query(struct traceeval_iterator *iter,
+			     const union traceeval_data **results)
+{
+	struct entry *entry;
+
+	if (iter->next < 1 || iter->next > iter->nr_entries)
+		return 0;
+
+	entry = iter->entries[iter->next - 1];
+	*results = entry->vals;
+
+	return 1;
+}
-- 
2.40.1

[PATCH v2 03/11] libtraceeval: Add traceeval_iterator_results_release()

[Steven Rostedt]

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

Add the function that can release the results from the
traceeval_iterator_qeury() function.

Currently, the user must use traceeval_results_release() for the results
returned by traceeval_iterator_query(), but that requires the caller to
also have access to the traceeval where the iterator came from. There
could be many iterators and many traceevals, the user should not need to
worry about which one goes with which.

This new traceeval_iterator_results_release() will take care of that for
them.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  2 ++
 src/histograms.c         | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 837a74f61a66..80450d7ae7f9 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -188,5 +188,7 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
 			    const union traceeval_data **keys);
 int traceeval_iterator_query(struct traceeval_iterator *iter,
 			     const union traceeval_data **results);
+void traceeval_iterator_results_release(struct traceeval_iterator *iter,
+					const union traceeval_data *results);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/src/histograms.c b/src/histograms.c
index 06613a8933ec..50dc8b689637 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -1336,3 +1336,26 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
 
 	return 1;
 }
+
+/*
+ * traceeval_iterator_results_release - release the results return by traceeval_iterator_query()
+ * @iter: The iterator descriptor used in traceeval_iterator_query()
+ * @results: The results returned by traceeval_iterator_query()
+ *
+ * The @results returned by traceeval_iterator_query() is owned by @teval,
+ * that is attached to the iterator and how it manages it is implementation
+ * specific. The caller should not worry about it. When the caller of
+ * traceeval_iterator_query() is done with the @results, it must call
+ * traceeval_iterator_results_release() (or traceeval_results_release() if it
+ * has the handle of the teval used to get the iterator) on it to allow traceeval
+ * to clean up its references.
+ */
+void traceeval_iterator_results_release(struct traceeval_iterator *iter,
+					const union traceeval_data *results)
+{
+	if (!iter || !results) {
+		if (!iter)
+			print_err("Results to be freed without accompanied iterator!");
+		return;
+	}
+}
-- 
2.40.1

[PATCH v2 04/11] libtraceeval: Add traceeval_iterator_stat()

[Steven Rostedt]

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

Add traceeval_iterator_stat() that will return a stat of a value of the
current entry from a traceeval_iterator_next() loop.

Reviewed-by: Ross Zwisler <zwisler@google.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  2 ++
 samples/task-eval.c      |  6 ++---
 src/histograms.c         | 47 ++++++++++++++++++++++++++++++++--------
 3 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 80450d7ae7f9..d6c7097a66c8 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -190,5 +190,7 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
 			     const union traceeval_data **results);
 void traceeval_iterator_results_release(struct traceeval_iterator *iter,
 					const union traceeval_data *results);
+struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
+					       struct traceeval_type *type);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/samples/task-eval.c b/samples/task-eval.c
index 18e07f44e11e..bde167d1219b 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -706,7 +706,7 @@ static void display_cpus(struct traceeval *teval)
 		int state = keys[1].number;
 		int cpu = keys[0].number;
 
-		stat = traceeval_stat(teval, keys, &delta_vals[0]);
+		stat = traceeval_iterator_stat(iter, &delta_vals[0]);
 		if (!stat)
 			continue; // die?
 
@@ -773,7 +773,7 @@ static void display_threads(struct traceeval *teval)
 		int state = keys[1].number;
 		int tid = keys[0].number;
 
-		stat = traceeval_stat(teval, keys, &delta_vals[0]);
+		stat = traceeval_iterator_stat(iter, &delta_vals[0]);
 		if (!stat)
 			continue; // die?
 
@@ -875,7 +875,7 @@ static void display(struct task_data *tdata)
 	while (traceeval_iterator_next(iter, &keys) > 0) {
 		int state = keys[1].number;
 
-		stat = traceeval_stat(teval, keys, &delta_vals[0]);
+		stat = traceeval_iterator_stat(iter, &delta_vals[0]);
 		if (!stat)
 			continue;
 
diff --git a/src/histograms.c b/src/histograms.c
index 50dc8b689637..1dd32157f1cb 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -812,16 +812,11 @@ static int update_entry(struct traceeval *teval, struct entry *entry,
 	return -1;
 }
 
-struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const union traceeval_data *keys,
-				      struct traceeval_type *type)
+static bool is_stat_type(struct traceeval_type *type)
 {
-	struct entry *entry;
-	int ret;
-
 	/* Only value numbers have stats */
 	if (!(type->flags & TRACEEVAL_FL_VALUE))
-		return NULL;
+		return false;
 
 	switch (type->type) {
 	case TRACEEVAL_TYPE_NUMBER:
@@ -829,10 +824,21 @@ struct traceeval_stat *traceeval_stat(struct traceeval *teval,
 	case TRACEEVAL_TYPE_NUMBER_32:
 	case TRACEEVAL_TYPE_NUMBER_16:
 	case TRACEEVAL_TYPE_NUMBER_8:
-		break;
+		return true;
 	default:
-		return NULL;
+		return false;
 	}
+}
+
+struct traceeval_stat *traceeval_stat(struct traceeval *teval,
+				      const union traceeval_data *keys,
+				      struct traceeval_type *type)
+{
+	struct entry *entry;
+	int ret;
+
+	if (!is_stat_type(type))
+		return NULL;
 
 	ret = get_entry(teval, keys, &entry);
 	if (ret <= 0)
@@ -1359,3 +1365,26 @@ void traceeval_iterator_results_release(struct traceeval_iterator *iter,
 		return;
 	}
 }
+
+/**
+ * traceeval_iterator_stat - return the stats from the last iterator entry
+ * @iter: The iterator to retrieve the stats from
+ * @type: The value type to get the stat from
+ *
+ * Returns the stats of the @type for the current iterator entry on success,
+ * or NULL if not found or an error occurred.
+ */
+struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
+					       struct traceeval_type *type)
+{
+	struct entry *entry;
+
+	if (!is_stat_type(type))
+		return NULL;
+
+	if (iter->next < 1 || iter->next > iter->nr_entries)
+		return NULL;
+
+	entry = iter->entries[iter->next - 1];
+	return &entry->val_stats[type->index];
+}
-- 
2.40.1

[PATCH v2 05/11] libtraceeval: Add traceeval_iterator_remove()

[Steven Rostedt]

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

Add an API traceeval_iterator_remove() that is safe to call in the
traceeval_iterator_next() loop. Currently, traceeval_remove() can also be
called "safely", but that may change in the future.

The main difference between traceeval_remove() and traceeval_iterator_remove()
is that that traceeval_iterator_remove() will NULL out the entry in the
sort array, and use this in the other iterator functions. If the entry is
NULL, it will not be returned.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  1 +
 src/histograms.c         | 49 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index d6c7097a66c8..e619d52ea0d0 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -192,5 +192,6 @@ void traceeval_iterator_results_release(struct traceeval_iterator *iter,
 					const union traceeval_data *results);
 struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
 					       struct traceeval_type *type);
+int traceeval_iterator_remove(struct traceeval_iterator *iter);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/src/histograms.c b/src/histograms.c
index 1dd32157f1cb..0cc3e0bd732b 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -968,6 +968,7 @@ int traceeval_remove(struct traceeval *teval,
 		return check;
 
 	hash_remove(hist, &entry->hash);
+	free_entry(teval, entry);
 
 	teval->update_counter++;
 
@@ -1305,10 +1306,13 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
 		iter->next = 0;
 	}
 
-	if (iter->next >= iter->nr_entries)
-		return 0;
+	do {
+		if (iter->next >= iter->nr_entries)
+			return 0;
+
+		entry = iter->entries[iter->next++];
+	} while (!entry);
 
-	entry = iter->entries[iter->next++];
 	*keys = entry->keys;
 	return 1;
 }
@@ -1338,6 +1342,9 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
 		return 0;
 
 	entry = iter->entries[iter->next - 1];
+	if (!entry)
+		return 0;
+
 	*results = entry->vals;
 
 	return 1;
@@ -1386,5 +1393,39 @@ struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
 		return NULL;
 
 	entry = iter->entries[iter->next - 1];
-	return &entry->val_stats[type->index];
+	return entry ? &entry->val_stats[type->index] : NULL;
+}
+
+/**
+ * traceeval_iterator_remove - remove the current iterator entry
+ * @iter: The iterator to remove the entry from
+ *
+ * This will remove the current entry from the histogram.
+ * This is useful if the current entry should be removed. It will not
+ * affect the traceeval_iterator_next().
+ *
+ * Returns 1 if it successfully removed the entry, 0 if for some reason
+ * there was no "current entry" (called before traceeval_iterator_next()).
+ */
+int traceeval_iterator_remove(struct traceeval_iterator *iter)
+{
+	struct traceeval *teval = iter->teval;
+	struct hash_table *hist = teval->hist;
+	struct entry *entry;
+
+	if (iter->next < 1 || iter->next > iter->nr_entries)
+		return 0;
+
+	entry = iter->entries[iter->next - 1];
+	if (!entry)
+		return 0;
+
+	hash_remove(hist, &entry->hash);
+	free_entry(teval, entry);
+
+	/* The entry no longer exists */
+	iter->entries[iter->next - 1] = NULL;
+	teval->update_counter++;
+
+	return 1;
 }
-- 
2.40.1

[PATCH v2 06/11] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Steven Rostedt]

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

Having a straight union for passing in the data that must match the types
is dangerous and prone for buggy code. If some data doesn't match its
type, there's nothing to catch it.

Instead of having a union traceeval_data of each type, have it be a
structure with a "type" field follow by a union, where the type defines
what kind of data it is.

Add helper macros to make it easy to define the data when using it.

Reviewed-by: Ross Zwisler <zwisler@google.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  90 ++++++++++++++++---------
 samples/task-eval.c      | 139 ++++++++++++++++-----------------------
 src/eval-local.h         |   4 +-
 src/histograms.c         |  70 ++++++++++----------
 4 files changed, 153 insertions(+), 150 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index e619d52ea0d0..78cfac5982ee 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -52,45 +52,75 @@ struct traceeval_dynamic {
  * Trace data entry for a traceeval histogram
  * Constitutes keys and values.
  */
-union traceeval_data {
-	struct traceeval_dynamic	dyn_data;
-	char				*string;
-	const char			*cstring;
-	void				*pointer;
-	unsigned long			number;
-	unsigned long long		number_64;
-	unsigned int			number_32;
-	unsigned short			number_16;
-	unsigned char			number_8;
+struct traceeval_data {
+	enum traceeval_data_type		type;
+	union {
+		struct traceeval_dynamic	dyn_data;
+		char				*string;
+		const char			*cstring;
+		void				*pointer;
+		unsigned long			number;
+		unsigned long long		number_64;
+		unsigned int			number_32;
+		unsigned short			number_16;
+		unsigned char			number_8;
+	};
 };
 
+#define __TRACEEVAL_DATA(data_type, member, data)			\
+	{  .type = TRACEEVAL_TYPE_##data_type, .member = (data) }
+
+#define DEFINE_TRACEEVAL_NUMBER(data)	   __TRACEEVAL_DATA(NUMBER, number, data)
+#define DEFINE_TRACEEVAL_NUMBER_8(data)	   __TRACEEVAL_DATA(NUMBER_8, number_8, data)
+#define DEFINE_TRACEEVAL_NUMBER_16(data)   __TRACEEVAL_DATA(NUMBER_16, number_16, data)
+#define DEFINE_TRACEEVAL_NUMBER_32(data)   __TRACEEVAL_DATA(NUMBER_32, number_32, data)
+#define DEFINE_TRACEEVAL_NUMBER_64(data)   __TRACEEVAL_DATA(NUMBER_64, number_64, data)
+#define DEFINE_TRACEEVAL_STRING(data)	   __TRACEEVAL_DATA(STRING, string, data)
+#define DEFINE_TRACEEVAL_CSTRING(data)	   __TRACEEVAL_DATA(STRING, cstring, data)
+#define DEFINE_TRACEEVAL_POINTER(data)	   __TRACEEVAL_DATA(POINTER, pointer, data)
+
+#define __TRACEEVAL_SET(data, data_type, member, val)		\
+	do {							\
+		(data).type = TRACEEVAL_TYPE_##data_type;	\
+		(data).member = (val);				\
+	} while (0)
+
+#define TRACEEVAL_SET_NUMBER(data, val)	     __TRACEEVAL_SET(data, NUMBER, number, val)
+#define TRACEEVAL_SET_NUMBER_8(data, val)    __TRACEEVAL_SET(data, NUMBER_8, number_8, val)
+#define TRACEEVAL_SET_NUMBER_16(data, val)   __TRACEEVAL_SET(data, NUMBER_16, number_16, val)
+#define TRACEEVAL_SET_NUMBER_32(data, val)   __TRACEEVAL_SET(data, NUMBER_32, number_32, val)
+#define TRACEEVAL_SET_NUMBER_64(data, val)   __TRACEEVAL_SET(data, NUMBER_64, number_64, val)
+#define TRACEEVAL_SET_STRING(data, val)	     __TRACEEVAL_SET(data, STRING, string, val)
+#define TRACEEVAL_SET_CSTRING(data, val)     __TRACEEVAL_SET(data, STRING, cstring, val)
+#define TRACEEVAL_SET_POINTER(data, val)     __TRACEEVAL_SET(data, POINTER, pointer, val)
+
 struct traceeval_type;
 struct traceeval;
 
 /* release function callback on traceeval_data */
 typedef void (*traceeval_data_release_fn)(const struct traceeval_type *type,
-					  union traceeval_data *data);
+					  struct traceeval_data *data);
 
 /* compare function callback to compare traceeval_data */
 typedef int (*traceeval_data_cmp_fn)(struct traceeval *teval,
 				     const struct traceeval_type *type,
-				     const union traceeval_data *A,
-				     const union traceeval_data *B);
+				     const struct traceeval_data *A,
+				     const struct traceeval_data *B);
 
 /* make a unique value */
 typedef int (*traceeval_data_hash_fn)(struct traceeval *teval,
 				      const struct traceeval_type *type,
-				      const union traceeval_data *data);
+				      const struct traceeval_data *data);
 
 typedef int (*traceeval_data_copy_fn)(const struct traceeval_type *type,
-				      union traceeval_data *dst,
-				      const union traceeval_data *src);
+				      struct traceeval_data *dst,
+				      const struct traceeval_data *src);
 
 typedef int (*traceeval_cmp_fn)(struct traceeval *teval,
-				const union traceeval_data *Akeys,
-				const union traceeval_data *Avals,
-				const union traceeval_data *Bkeys,
-				const union traceeval_data *Bvals,
+				const struct traceeval_data *Akeys,
+				const struct traceeval_data *Avals,
+				const struct traceeval_data *Bkeys,
+				const struct traceeval_data *Bvals,
 				void *data);
 
 /*
@@ -157,20 +187,20 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
 void traceeval_release(struct traceeval *teval);
 
 int traceeval_insert(struct traceeval *teval,
-		     const union traceeval_data *keys,
-		     const union traceeval_data *vals);
+		     const struct traceeval_data *keys,
+		     const struct traceeval_data *vals);
 
 int traceeval_remove(struct traceeval *teval,
-		     const union traceeval_data *keys);
+		     const struct traceeval_data *keys);
 
-int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
-		    const union traceeval_data **results);
+int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
+		    const struct traceeval_data **results);
 
 void traceeval_results_release(struct traceeval *teval,
-			       const union traceeval_data *results);
+			       const struct traceeval_data *results);
 
 struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const union traceeval_data *keys,
+				      const struct traceeval_data *keys,
 				      struct traceeval_type *type);
 
 unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
@@ -185,11 +215,11 @@ int traceeval_iterator_sort(struct traceeval_iterator *iter, const char *sort_fi
 int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
 				   traceeval_cmp_fn sort_fn, void *data);
 int traceeval_iterator_next(struct traceeval_iterator *iter,
-			    const union traceeval_data **keys);
+			    const struct traceeval_data **keys);
 int traceeval_iterator_query(struct traceeval_iterator *iter,
-			     const union traceeval_data **results);
+			     const struct traceeval_data **results);
 void traceeval_iterator_results_release(struct traceeval_iterator *iter,
-					const union traceeval_data *results);
+					const struct traceeval_data *results);
 struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
 					       struct traceeval_type *type);
 int traceeval_iterator_remove(struct traceeval_iterator *iter);
diff --git a/samples/task-eval.c b/samples/task-eval.c
index bde167d1219b..e62bfddafdd3 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -190,21 +190,15 @@ static void update_process(struct task_data *tdata, const char *comm,
 			   enum sched_state state, enum command cmd,
 			   unsigned long long ts)
 {
-	union traceeval_data keys[] = {
-		{
-			.cstring	= comm,
-		},
-		{
-			.number		= state,
-		},
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm	),
+		DEFINE_TRACEEVAL_NUMBER(	state	),
 	};
-	union traceeval_data vals[] = {
-		{
-			.number_64	= ts,
-		},
+	struct traceeval_data vals[] = {
+		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
 	};
-	union traceeval_data new_vals[1] = { };
-	const union traceeval_data *results;
+	struct traceeval_data new_vals[1] = { };
+	const struct traceeval_data *results;
 	int ret;
 
 	switch (cmd) {
@@ -222,14 +216,15 @@ static void update_process(struct task_data *tdata, const char *comm,
 		if (!results[0].number_64)
 			break;
 
-		new_vals[0].number_64 = ts - results[0].number_64;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
 
 		ret = traceeval_insert(tdata->teval_processes.stop, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not stop process");
 
 		/* Reset the start */
-		new_vals[0].number_64 = 0;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
+
 		ret = traceeval_insert(tdata->teval_processes.start, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not start CPU");
@@ -253,15 +248,11 @@ static void stop_process(struct task_data *tdata, const char *comm,
 static struct process_data *
 get_process_data(struct task_data *tdata, const char *comm)
 {
-	union traceeval_data keys[] = {
-		{
-			.cstring = comm,
-		},
-		{
-			.number = RUNNING,
-		}
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
 	};
-	const union traceeval_data *results;
+	const struct traceeval_data *results;
 	void *data;
 	int ret;
 
@@ -278,16 +269,12 @@ get_process_data(struct task_data *tdata, const char *comm)
 
 void set_process_data(struct task_data *tdata, const char *comm, void *data)
 {
-	union traceeval_data keys[] = {
-		{
-			.cstring = comm,
-		},
-		{
-			.number = RUNNING,
-		}
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
 	};
-	union traceeval_data new_vals[1] = { };
-	const union traceeval_data *results;
+	struct traceeval_data new_vals[1] = { };
+	const struct traceeval_data *results;
 	int ret;
 
 	ret = traceeval_query(tdata->teval_processes_data, keys, &results);
@@ -296,7 +283,7 @@ void set_process_data(struct task_data *tdata, const char *comm, void *data)
 	if (ret < 0)
 		pdie("Could not query process data");
 
-	new_vals[0].pointer = data;
+	TRACEEVAL_SET_POINTER(new_vals[0], data);
 	ret = traceeval_insert(tdata->teval_processes_data, keys, new_vals);
 	if (ret < 0)
 		pdie("Failed to set process data");
@@ -309,21 +296,15 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
 		       enum sched_state state, enum command cmd,
 		       unsigned long long ts)
 {
-	const union traceeval_data *results;
-	union traceeval_data keys[] = {
-		{
-			.number = cpu,
-		},
-		{
-			.number = state,
-		}
+	const struct traceeval_data *results;
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_NUMBER(	cpu	),
+		DEFINE_TRACEEVAL_NUMBER(	state	),
 	};
-	union traceeval_data vals[] = {
-		{
-			.number_64	= ts,
-		},
+	struct traceeval_data vals[] = {
+		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
 	};
-	union traceeval_data new_vals[1] = { };
+	struct traceeval_data new_vals[1] = { };
 	int ret;
 
 	switch (cmd) {
@@ -350,14 +331,14 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
 		if (!results[0].number_64)
 			break;
 
-		new_vals[0].number_64 = ts - results[0].number_64;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
 
 		ret = traceeval_insert(teval_pair->stop, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not stop CPU");
 
 		/* Reset the start */
-		new_vals[0].number_64 = 0;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
 		ret = traceeval_insert(teval_pair->start, keys, new_vals);
 		if (ret < 0)
 			pdie("Could not start CPU");
@@ -385,21 +366,15 @@ static void update_thread(struct process_data *pdata, int tid,
 			  enum sched_state state, enum command cmd,
 			  unsigned long long ts)
 {
-	const union traceeval_data *results;
-	union traceeval_data keys[] = {
-		{
-			.number = tid,
-		},
-		{
-			.number = state,
-		}
+	const struct traceeval_data *results;
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_NUMBER(	tid	),
+		DEFINE_TRACEEVAL_NUMBER(	state	),
 	};
-	union traceeval_data vals[] = {
-		{
-			.number_64	= ts,
-		},
+	struct traceeval_data vals[] = {
+		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
 	};
-	union traceeval_data new_vals[1] = { };
+	struct traceeval_data new_vals[1] = { };
 	int ret;
 
 	switch (cmd) {
@@ -415,7 +390,7 @@ static void update_thread(struct process_data *pdata, int tid,
 		if (ret == 0)
 			return;
 
-		new_vals[0].number_64 = ts - results[0].number_64;
+		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
 
 		ret = traceeval_insert(pdata->teval_threads.stop, keys, new_vals);
 		traceeval_results_release(pdata->teval_threads.start, results);
@@ -646,17 +621,19 @@ static void print_microseconds(int idx, unsigned long long nsecs)
  *  If RUNNING is equal, then sort by COMM.
  */
 static int compare_pdata(struct traceeval *teval_data,
-				const union traceeval_data *Akeys,
-				const union traceeval_data *Avals,
-				const union traceeval_data *Bkeys,
-				const union traceeval_data *Bvals,
+				const struct traceeval_data *Akeys,
+				const struct traceeval_data *Avals,
+				const struct traceeval_data *Bkeys,
+				const struct traceeval_data *Bvals,
 				void *data)
 {
 	struct traceeval *teval = data; /* The deltas are here */
-	union traceeval_data keysA[] = {
-		{ .cstring = Akeys[0].cstring }, { .number = RUNNING } };
-	union traceeval_data keysB[] = {
-		{ .cstring = Bkeys[0].cstring }, { .number = RUNNING } };
+	struct traceeval_data keysA[] = {
+		DEFINE_TRACEEVAL_CSTRING(	Akeys[0].cstring	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
+	struct traceeval_data keysB[] = {
+		DEFINE_TRACEEVAL_CSTRING(	Bkeys[0].cstring	),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
 	struct traceeval_stat *statA;
 	struct traceeval_stat *statB;
 	unsigned long long totalA = -1;
@@ -690,7 +667,7 @@ static int compare_pdata(struct traceeval *teval_data,
 static void display_cpus(struct traceeval *teval)
 {
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	struct traceeval_stat *stat;
 	int last_cpu = -1;
 
@@ -762,7 +739,7 @@ static void display_state_times(int state, unsigned long long total)
 static void display_threads(struct traceeval *teval)
 {
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	struct traceeval_stat *stat;
 	int last_tid = -1;
 
@@ -802,17 +779,13 @@ static void display_process_stats(struct traceeval *teval,
 {
 	struct traceeval_stat *stat;
 	unsigned long long delta;
-	union traceeval_data keys[] = {
-		{
-			.cstring = comm,
-		},
-		{
-			.number = RUNNING,
-		}
+	struct traceeval_data keys[] = {
+		DEFINE_TRACEEVAL_CSTRING(	comm		),
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
 	};
 
 	for (int i = 0; i < OTHER; i++) {
-		keys[1].number = i;
+		TRACEEVAL_SET_NUMBER_64(keys[1], i);
 
 		delta = 0;
 		stat = traceeval_stat(teval, keys, &delta_vals[0]);
@@ -826,13 +799,13 @@ static void display_processes(struct traceeval *teval,
 			      struct traceeval *teval_data)
 {
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval_data);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	int ret;
 
 	traceeval_iterator_sort_custom(iter, compare_pdata, teval);
 
 	while (traceeval_iterator_next(iter, &keys) > 0) {
-		const union traceeval_data *results;
+		const struct traceeval_data *results;
 		struct process_data *pdata = NULL;
 		const char *comm = keys[0].cstring;
 
@@ -857,7 +830,7 @@ static void display(struct task_data *tdata)
 {
 	struct traceeval *teval = tdata->teval_cpus.stop;
 	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
-	const union traceeval_data *keys;
+	const struct traceeval_data *keys;
 	struct traceeval_stat *stat;
 	unsigned long long total_time = 0;
 	unsigned long long idle_time = 0;
diff --git a/src/eval-local.h b/src/eval-local.h
index 73f52efdf0da..26b3c9b29929 100644
--- a/src/eval-local.h
+++ b/src/eval-local.h
@@ -56,8 +56,8 @@ struct traceeval_stat {
 /* A key-value pair */
 struct entry {
 	struct hash_item	hash;
-	union traceeval_data	*keys;
-	union traceeval_data	*vals;
+	struct traceeval_data	*keys;
+	struct traceeval_data	*vals;
 	struct traceeval_stat	*val_stats;
 };
 
diff --git a/src/histograms.c b/src/histograms.c
index 0cc3e0bd732b..fc29558d75fe 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -37,8 +37,8 @@ static void print_err(const char *fmt, ...)
  */
 static int compare_traceeval_data(struct traceeval *teval,
 				  struct traceeval_type *type,
-				  const union traceeval_data *orig,
-				  const union traceeval_data *copy)
+				  const struct traceeval_data *orig,
+				  const struct traceeval_data *copy)
 {
 	int i;
 
@@ -86,14 +86,14 @@ static int compare_traceeval_data(struct traceeval *teval,
 }
 
 /*
- * Compare arrays of union traceeval_data's with respect to @def.
+ * Compare arrays of struct traceeval_data's with respect to @def.
  *
  * Return 1 if @orig and @copy are the same, 0 if not, and -1 on error.
  */
 static int compare_traceeval_data_set(struct traceeval *teval,
 				      struct traceeval_type *defs,
-				      union traceeval_data *orig,
-				      const union traceeval_data *copy, size_t size)
+				      struct traceeval_data *orig,
+				      const struct traceeval_data *copy, size_t size)
 {
 	int check;
 	size_t i;
@@ -335,7 +335,7 @@ fail:
 /*
  * Frees dynamic data in @data if @type specifies a dynamic data type.
  */
-static void clean_data(union traceeval_data *data, struct traceeval_type *type)
+static void clean_data(struct traceeval_data *data, struct traceeval_type *type)
 {
 		if (type->release)
 			type->release(type, data);
@@ -352,7 +352,7 @@ static void clean_data(union traceeval_data *data, struct traceeval_type *type)
 /*
  * Free any specified dynamic data in @data.
  */
-static void clean_data_set(union traceeval_data *data, struct traceeval_type *defs,
+static void clean_data_set(struct traceeval_data *data, struct traceeval_type *defs,
 		       size_t size)
 {
 	size_t i;
@@ -431,7 +431,7 @@ void traceeval_release(struct traceeval *teval)
 	free(teval);
 }
 
-static unsigned make_hash(struct traceeval *teval, const union traceeval_data *keys,
+static unsigned make_hash(struct traceeval *teval, const struct traceeval_data *keys,
 			  int bits)
 {
 	const struct traceeval_type *types = teval->key_types;
@@ -470,7 +470,7 @@ static unsigned make_hash(struct traceeval *teval, const union traceeval_data *k
  *
  * Returns 1 on success, 0 if no match found, -1 on error.
  */
-static int get_entry(struct traceeval *teval, const union traceeval_data *keys,
+static int get_entry(struct traceeval *teval, const struct traceeval_data *keys,
 		     struct entry **result)
 {
 	struct hash_table *hist = teval->hist;
@@ -506,8 +506,8 @@ static int get_entry(struct traceeval *teval, const union traceeval_data *keys,
  */
 static int copy_traceeval_data(struct traceeval_type *type,
 			       struct traceeval_stat *stat,
-			       union traceeval_data *dst,
-			       const union traceeval_data *src)
+			       struct traceeval_data *dst,
+			       const struct traceeval_data *src)
 {
 	unsigned long long val;
 
@@ -597,7 +597,7 @@ static int copy_traceeval_data(struct traceeval_type *type,
  *
  * Does not call the release() callback if a copy() exists.
  */
-static void data_release(size_t size, union traceeval_data *data,
+static void data_release(size_t size, struct traceeval_data *data,
 			 struct traceeval_type *type)
 {
 	for (size_t i = 0; i < size; i++) {
@@ -610,7 +610,7 @@ static void data_release(size_t size, union traceeval_data *data,
 	}
 }
 
-static void data_release_and_free(size_t size, union traceeval_data **data,
+static void data_release_and_free(size_t size, struct traceeval_data **data,
 				struct traceeval_type *type)
 {
 	data_release(size, *data, type);
@@ -625,8 +625,8 @@ static void data_release_and_free(size_t size, union traceeval_data **data,
  */
 static int dup_traceeval_data_set(size_t size, struct traceeval_type *type,
 				  struct traceeval_stat *stats,
-				  const union traceeval_data *orig,
-				  union traceeval_data **copy)
+				  const struct traceeval_data *orig,
+				  struct traceeval_data **copy)
 {
 	size_t i;
 
@@ -669,8 +669,8 @@ fail:
  *
  * Returns 1 if found, 0 if not found, and -1 on error.
  */
-int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
-		    const union traceeval_data **results)
+int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
+		    const struct traceeval_data **results)
 {
 	struct entry *entry;
 	int check;
@@ -698,7 +698,7 @@ int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
  * allow traceeval to clean up its references.
  */
 void traceeval_results_release(struct traceeval *teval,
-			       const union traceeval_data *results)
+			       const struct traceeval_data *results)
 {
 	if (!teval || !results) {
 		if (!teval)
@@ -708,7 +708,7 @@ void traceeval_results_release(struct traceeval *teval,
 }
 
 static struct entry *create_hist_entry(struct traceeval *teval,
-				       const union traceeval_data *keys)
+				       const struct traceeval_data *keys)
 {
 	struct hash_table *hist = teval->hist;
 	unsigned key = make_hash(teval, keys, hist->bits);
@@ -729,11 +729,11 @@ static struct entry *create_hist_entry(struct traceeval *teval,
  * Returns 0 on success, -1 on error.
  */
 static int create_entry(struct traceeval *teval,
-			const union traceeval_data *keys,
-			const union traceeval_data *vals)
+			const struct traceeval_data *keys,
+			const struct traceeval_data *vals)
 {
-	union traceeval_data *new_keys;
-	union traceeval_data *new_vals;
+	struct traceeval_data *new_keys;
+	struct traceeval_data *new_vals;
 	struct entry *entry;
 
 	entry = create_hist_entry(teval, keys);
@@ -780,12 +780,12 @@ fail_entry:
  * Return 0 on success, -1 on error.
  */
 static int update_entry(struct traceeval *teval, struct entry *entry,
-			const union traceeval_data *vals)
+			const struct traceeval_data *vals)
 {
 	struct traceeval_stat *stats = entry->val_stats;
 	struct traceeval_type *types = teval->val_types;
-	union traceeval_data *copy = entry->vals;
-	union traceeval_data old[teval->nr_val_types];
+	struct traceeval_data *copy = entry->vals;
+	struct traceeval_data old[teval->nr_val_types];
 	size_t size = teval->nr_val_types;
 	ssize_t i;
 
@@ -831,7 +831,7 @@ static bool is_stat_type(struct traceeval_type *type)
 }
 
 struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const union traceeval_data *keys,
+				      const struct traceeval_data *keys,
 				      struct traceeval_type *type)
 {
 	struct entry *entry;
@@ -923,8 +923,8 @@ unsigned long long traceeval_stat_count(struct traceeval_stat *stat)
  * Returns 0 on success, and -1 on error.
  */
 int traceeval_insert(struct traceeval *teval,
-		     const union traceeval_data *keys,
-		     const union traceeval_data *vals)
+		     const struct traceeval_data *keys,
+		     const struct traceeval_data *vals)
 {
 	struct entry *entry;
 	int check;
@@ -955,7 +955,7 @@ int traceeval_insert(struct traceeval *teval,
  *        -1 if there was an error.
  */
 int traceeval_remove(struct traceeval *teval,
-		     const union traceeval_data *keys)
+		     const struct traceeval_data *keys)
 {
 	struct hash_table *hist = teval->hist;
 	struct entry *entry;
@@ -1175,8 +1175,8 @@ static int iter_cmp(const void *A, const void *B, void *data)
 
 	for (int i = 0; i < iter->nr_sort; i++) {
 		struct traceeval_type *type;
-		union traceeval_data *dataA;
-		union traceeval_data *dataB;
+		struct traceeval_data *dataA;
+		struct traceeval_data *dataB;
 
 		type = iter->sort[i];
 
@@ -1294,7 +1294,7 @@ int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
  * Returns 1 if another entry is returned, or 0 if not (or negative on error)
  */
 int traceeval_iterator_next(struct traceeval_iterator *iter,
-			    const union traceeval_data **keys)
+			    const struct traceeval_data **keys)
 {
 	struct entry *entry;
 	int ret;
@@ -1334,7 +1334,7 @@ int traceeval_iterator_next(struct traceeval_iterator *iter,
  * Returns 1 if another entry is returned, or 0 if not (or negative on error)
  */
 int traceeval_iterator_query(struct traceeval_iterator *iter,
-			     const union traceeval_data **results)
+			     const struct traceeval_data **results)
 {
 	struct entry *entry;
 
@@ -1364,7 +1364,7 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
  * to clean up its references.
  */
 void traceeval_iterator_results_release(struct traceeval_iterator *iter,
-					const union traceeval_data *results)
+					const struct traceeval_data *results)
 {
 	if (!iter || !results) {
 		if (!iter)
-- 
2.40.1

[PATCH v2 07/11] libtraceveal: Add type checks to traceeval_data vals and keys

[Steven Rostedt]

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

Now that the traceeval_data has a type, add checks to traceeval_insert()
and traceveal_query() as well as traceeval_stat() to make sure the keys
and vals match the types that were for the traceeval.

Which also caught a bug in task_eval.c display_process_stats().

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 samples/task-eval.c |  2 +-
 src/histograms.c    | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/samples/task-eval.c b/samples/task-eval.c
index e62bfddafdd3..475df190acd2 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -785,7 +785,7 @@ static void display_process_stats(struct traceeval *teval,
 	};
 
 	for (int i = 0; i < OTHER; i++) {
-		TRACEEVAL_SET_NUMBER_64(keys[1], i);
+		TRACEEVAL_SET_NUMBER(keys[1], i);
 
 		delta = 0;
 		stat = traceeval_stat(teval, keys, &delta_vals[0]);
diff --git a/src/histograms.c b/src/histograms.c
index fc29558d75fe..954042da28e4 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -479,10 +479,16 @@ static int get_entry(struct traceeval *teval, const struct traceeval_data *keys,
 	struct hash_item *item;
 	unsigned key;
 	int check = 0;
+	int i;
 
 	if (!teval || !keys)
 		return -1;
 
+	for (i = 0; i < teval->nr_key_types; i++) {
+		if (keys[i].type != teval->key_types[i].type)
+			return -1;
+	}
+
 	key = make_hash(teval, keys, hist->bits);
 
 	for (iter = hash_iter_bucket(hist, key); (item = hash_iter_bucket_next(iter)); ) {
@@ -792,6 +798,11 @@ static int update_entry(struct traceeval *teval, struct entry *entry,
 	if (!size)
 		return 0;
 
+	for (i = 0; i < teval->nr_val_types; i++) {
+		if (vals[i].type != teval->val_types[i].type)
+			return -1;
+	}
+
 	for (i = 0; i < size; i++) {
 		old[i] = copy[i];
 
@@ -928,10 +939,16 @@ int traceeval_insert(struct traceeval *teval,
 {
 	struct entry *entry;
 	int check;
+	int i;
 
 	entry = NULL;
 	check = get_entry(teval, keys, &entry);
 
+	for (i = 0; i < teval->nr_val_types; i++) {
+		if (vals[i].type != teval->val_types[i].type)
+			return -1;
+	}
+
 	if (check == -1)
 		return check;
 
-- 
2.40.1

[PATCH v2 08/11] libtraceeval: Add size checks to insert and query functions

[Steven Rostedt]

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

Currently, there's nothing that checks the size of the keys/values being
passed into traceeval_query() and traceeval_insert(). Make functions that
take the size of those arrays:

   traceeval_query_size()
   traceeval_insert_size()

and convert the traceeval_query() and traceeval_insert() into macros that
calculate the size of the fields to pass in:

  #define TRACEEVAL_ARRAY_SIZE(data)  \
     	(sizeof(data) / sizeof((data)[0]))

  #define traceeval_query(teval, keys, results) \
     traceeval_query_size(teval, TRACEEVAL_ARRAY_SIZE(keys), results)

and this will allow the code to check to make sure the size matches what
is expected (note, that currently is not done, but can be added).

If the keys or vals is not an array, but instead a pointer, if one of the
macros is used, the compiler will complain with:

 warning: division sizeof (const struct traceeval_data *) / sizeof (const struct traceeval_data)
          does not compute the number of array elements [-Wsizeof-pointer-div]
   20 | #define TRACEEVAL_ARRAY_SIZE(data)      (sizeof(data) / sizeof((data)[0]))
      |                                                       ^
 include/traceeval-hist.h:206:43: note: in expansion of macro TRACEEVAL_ARRAY_SIZE
  206 |         traceeval_query_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys), results)
      |                                           ^~~~~~~~~~~~~~~~~~~~

Then the user would need to call the "_size()" function directly
(as is done in the sample code, where the keys received by an iterator is
 used for a query)

Now that there are size functions for traceeval_insert() and
traceveal_query(), add checks to make sure that the size being passed in
is actually the size expected.

Reviewed-by: Ross Zwisler <zwisler@google.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h | 19 ++++++++++++++-----
 src/histograms.c         | 16 +++++++++++-----
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 78cfac5982ee..7f48bb92cc96 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -17,6 +17,8 @@
 /* Field name/descriptor for number of hits */
 #define TRACEEVAL_VAL_HITS ((const char *)(-1UL))
 
+#define TRACEEVAL_ARRAY_SIZE(data)	(sizeof(data) / sizeof((data)[0]))
+
 /* Data type distinguishers */
 enum traceeval_data_type {
 	TRACEEVAL_TYPE_NONE,
@@ -186,15 +188,22 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
 
 void traceeval_release(struct traceeval *teval);
 
-int traceeval_insert(struct traceeval *teval,
-		     const struct traceeval_data *keys,
-		     const struct traceeval_data *vals);
+int traceeval_insert_size(struct traceeval *teval,
+			  const struct traceeval_data *keys, size_t nr_keys,
+			  const struct traceeval_data *vals, size_t nr_vals);
+
+#define traceeval_insert(teval, keys, vals)				\
+	traceeval_insert_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys), \
+			      vals, TRACEEVAL_ARRAY_SIZE(vals))
 
 int traceeval_remove(struct traceeval *teval,
 		     const struct traceeval_data *keys);
 
-int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
-		    const struct traceeval_data **results);
+int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *keys,
+			 size_t nr_keys, const struct traceeval_data **results);
+
+#define traceeval_query(teval, keys, results)				\
+	traceeval_query_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys), results)
 
 void traceeval_results_release(struct traceeval *teval,
 			       const struct traceeval_data *results);
diff --git a/src/histograms.c b/src/histograms.c
index 954042da28e4..ab8a560fe14d 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -675,8 +675,8 @@ fail:
  *
  * Returns 1 if found, 0 if not found, and -1 on error.
  */
-int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
-		    const struct traceeval_data **results)
+int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *keys,
+			 size_t nr_keys, const struct traceeval_data **results)
 {
 	struct entry *entry;
 	int check;
@@ -684,6 +684,9 @@ int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
 	if (!teval || !keys || !results)
 		return -1;
 
+	if (nr_keys != teval->nr_key_types)
+		return -1;
+
 	/* find key and copy its corresponding value pair */
 	if ((check = get_entry(teval, keys, &entry)) < 1)
 		return check;
@@ -933,14 +936,17 @@ unsigned long long traceeval_stat_count(struct traceeval_stat *stat)
  *
  * Returns 0 on success, and -1 on error.
  */
-int traceeval_insert(struct traceeval *teval,
-		     const struct traceeval_data *keys,
-		     const struct traceeval_data *vals)
+int traceeval_insert_size(struct traceeval *teval,
+			  const struct traceeval_data *keys, size_t nr_keys,
+			  const struct traceeval_data *vals, size_t nr_vals)
 {
 	struct entry *entry;
 	int check;
 	int i;
 
+	if (nr_keys != teval->nr_key_types || nr_vals != teval->nr_val_types)
+		return -1;
+
 	entry = NULL;
 	check = get_entry(teval, keys, &entry);
 
-- 
2.40.1

[PATCH v2 09/11] libtraceeval: Make traceeval_remove() check size of keys array

[Steven Rostedt]

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

Make traceeval_remove() into a macro to pass in the size of the keys
array, and rename the function to traceeval_remove_size() that now takes
the size of the keys array.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h | 7 +++++--
 src/histograms.c         | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 7f48bb92cc96..804a0aaa631d 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -196,8 +196,11 @@ int traceeval_insert_size(struct traceeval *teval,
 	traceeval_insert_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys), \
 			      vals, TRACEEVAL_ARRAY_SIZE(vals))
 
-int traceeval_remove(struct traceeval *teval,
-		     const struct traceeval_data *keys);
+int traceeval_remove_size(struct traceeval *teval,
+			  const struct traceeval_data *keys, size_t nr_keys);
+
+#define traceeval_remove(teval, keys)					\
+	traceeval_remove_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys))
 
 int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *keys,
 			 size_t nr_keys, const struct traceeval_data **results);
diff --git a/src/histograms.c b/src/histograms.c
index ab8a560fe14d..28cf0d4ed225 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -977,13 +977,16 @@ int traceeval_insert_size(struct traceeval *teval,
  *         0 if it did not find an time matching @keys
  *        -1 if there was an error.
  */
-int traceeval_remove(struct traceeval *teval,
-		     const struct traceeval_data *keys)
+int traceeval_remove_size(struct traceeval *teval,
+			  const struct traceeval_data *keys, size_t nr_keys)
 {
 	struct hash_table *hist = teval->hist;
 	struct entry *entry;
 	int check;
 
+	if (teval->nr_key_types != nr_keys)
+		return -1;
+
 	entry = NULL;
 	check = get_entry(teval, keys, &entry);
 
-- 
2.40.1

[PATCH v2 10/11] libtraceeval: Make traceeval_stat() check size of keys array

[Steven Rostedt]

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

Make traceeval_stat() into a macro to pass in the size of the keys
array, and rename the function to traceeval_stat_size() that now takes
the size of the keys array.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h | 10 +++++++---
 src/histograms.c         | 10 +++++++---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index 804a0aaa631d..a1b498108fb4 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -211,9 +211,13 @@ int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *k
 void traceeval_results_release(struct traceeval *teval,
 			       const struct traceeval_data *results);
 
-struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const struct traceeval_data *keys,
-				      struct traceeval_type *type);
+#define traceeval_stat(teval, keys, type)				\
+	traceeval_stat_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys), type)
+
+struct traceeval_stat *traceeval_stat_size(struct traceeval *teval,
+					   const struct traceeval_data *keys,
+					   size_t nr_keys,
+					   struct traceeval_type *type);
 
 unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
 unsigned long long traceeval_stat_min(struct traceeval_stat *stat);
diff --git a/src/histograms.c b/src/histograms.c
index 28cf0d4ed225..4d534d127cff 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -844,13 +844,17 @@ static bool is_stat_type(struct traceeval_type *type)
 	}
 }
 
-struct traceeval_stat *traceeval_stat(struct traceeval *teval,
-				      const struct traceeval_data *keys,
-				      struct traceeval_type *type)
+struct traceeval_stat *traceeval_stat_size(struct traceeval *teval,
+					   const struct traceeval_data *keys,
+					   size_t nr_keys,
+					   struct traceeval_type *type)
 {
 	struct entry *entry;
 	int ret;
 
+	if (teval->nr_key_types != nr_keys)
+		return NULL;
+
 	if (!is_stat_type(type))
 		return NULL;
 
-- 
2.40.1

[PATCH v2 11/11] libtraceeval: Only do stats on values marked with the STAT flag

[Steven Rostedt]

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

Add TRACEEVAL_FL_STAT to perform stats on the value, otherwise ignore it.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/traceeval-hist.h |  1 +
 samples/task-eval.c      |  1 +
 src/histograms.c         | 39 ++++++++++++++++++++-------------------
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
index a1b498108fb4..618e67593dc0 100644
--- a/include/traceeval-hist.h
+++ b/include/traceeval-hist.h
@@ -38,6 +38,7 @@ enum traceeval_flags {
 	TRACEEVAL_FL_VALUE		= (1 << 1),
 	TRACEEVAL_FL_SIGNED		= (1 << 2),
 	TRACEEVAL_FL_TIMESTAMP		= (1 << 3),
+	TRACEEVAL_FL_STAT		= (1 << 4),
 };
 
 /*
diff --git a/samples/task-eval.c b/samples/task-eval.c
index 475df190acd2..6b01b8d076f2 100644
--- a/samples/task-eval.c
+++ b/samples/task-eval.c
@@ -147,6 +147,7 @@ static struct traceeval_type delta_vals[] = {
 	{
 		.type	= TRACEEVAL_TYPE_NUMBER_64,
 		.name	= "delta",
+		.flags = TRACEEVAL_FL_STAT,
 	},
 	{
 		.type	= TRACEEVAL_TYPE_NONE,
diff --git a/src/histograms.c b/src/histograms.c
index 4d534d127cff..0a3ab84d288e 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -505,6 +505,25 @@ static int get_entry(struct traceeval *teval, const struct traceeval_data *keys,
 	return check;
 }
 
+static bool is_stat_type(struct traceeval_type *type)
+{
+	/* Only value numbers have stats */
+	if (!(type->flags & TRACEEVAL_FL_VALUE) ||
+	    !(type->flags & TRACEEVAL_FL_STAT))
+		return false;
+
+	switch (type->type) {
+	case TRACEEVAL_TYPE_NUMBER:
+	case TRACEEVAL_TYPE_NUMBER_64:
+	case TRACEEVAL_TYPE_NUMBER_32:
+	case TRACEEVAL_TYPE_NUMBER_16:
+	case TRACEEVAL_TYPE_NUMBER_8:
+		return true;
+	default:
+		return false;
+	}
+}
+
 /*
  * Copy @src to @dst with respect to @type.
  *
@@ -571,7 +590,7 @@ static int copy_traceeval_data(struct traceeval_type *type,
 		return 0;
 	}
 
-	if (!stat)
+	if (!stat || !is_stat_type(type))
 		return 0;
 
 	if (!stat->count++) {
@@ -826,24 +845,6 @@ static int update_entry(struct traceeval *teval, struct entry *entry,
 	return -1;
 }
 
-static bool is_stat_type(struct traceeval_type *type)
-{
-	/* Only value numbers have stats */
-	if (!(type->flags & TRACEEVAL_FL_VALUE))
-		return false;
-
-	switch (type->type) {
-	case TRACEEVAL_TYPE_NUMBER:
-	case TRACEEVAL_TYPE_NUMBER_64:
-	case TRACEEVAL_TYPE_NUMBER_32:
-	case TRACEEVAL_TYPE_NUMBER_16:
-	case TRACEEVAL_TYPE_NUMBER_8:
-		return true;
-	default:
-		return false;
-	}
-}
-
 struct traceeval_stat *traceeval_stat_size(struct traceeval *teval,
 					   const struct traceeval_data *keys,
 					   size_t nr_keys,
-- 
2.40.1

Re: [PATCH v2 03/11] libtraceeval: Add traceeval_iterator_results_release()

[Ross Zwisler]

On Wed, Sep 27, 2023 at 08:33:06AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add the function that can release the results from the
> traceeval_iterator_qeury() function.
> 
> Currently, the user must use traceeval_results_release() for the results
> returned by traceeval_iterator_query(), but that requires the caller to
> also have access to the traceeval where the iterator came from. There
> could be many iterators and many traceevals, the user should not need to
> worry about which one goes with which.
> 
> This new traceeval_iterator_results_release() will take care of that for
> them.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Reviewed-by: Ross Zwisler <zwisler@google.com>

Re: [PATCH v2 05/11] libtraceeval: Add traceeval_iterator_remove()

[Ross Zwisler]

On Wed, Sep 27, 2023 at 08:33:08AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add an API traceeval_iterator_remove() that is safe to call in the
> traceeval_iterator_next() loop. Currently, traceeval_remove() can also be
> called "safely", but that may change in the future.
> 
> The main difference between traceeval_remove() and traceeval_iterator_remove()
> is that that traceeval_iterator_remove() will NULL out the entry in the
> sort array, and use this in the other iterator functions. If the entry is
> NULL, it will not be returned.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Reviewed-by: Ross Zwisler <zwisler@google.com>

Re: [PATCH v2 06/11] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Ross Zwisler]

On Wed, Sep 27, 2023 at 08:33:09AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Having a straight union for passing in the data that must match the types
> is dangerous and prone for buggy code. If some data doesn't match its
> type, there's nothing to catch it.
> 
> Instead of having a union traceeval_data of each type, have it be a
> structure with a "type" field follow by a union, where the type defines
> what kind of data it is.
> 
> Add helper macros to make it easy to define the data when using it.
> 
> Reviewed-by: Ross Zwisler <zwisler@google.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h |  90 ++++++++++++++++---------
>  samples/task-eval.c      | 139 ++++++++++++++++-----------------------
>  src/eval-local.h         |   4 +-
>  src/histograms.c         |  70 ++++++++++----------
>  4 files changed, 153 insertions(+), 150 deletions(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index e619d52ea0d0..78cfac5982ee 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -52,45 +52,75 @@ struct traceeval_dynamic {
>   * Trace data entry for a traceeval histogram
>   * Constitutes keys and values.
>   */
> -union traceeval_data {
> -	struct traceeval_dynamic	dyn_data;
> -	char				*string;
> -	const char			*cstring;
> -	void				*pointer;
> -	unsigned long			number;
> -	unsigned long long		number_64;
> -	unsigned int			number_32;
> -	unsigned short			number_16;
> -	unsigned char			number_8;
> +struct traceeval_data {
> +	enum traceeval_data_type		type;
> +	union {
> +		struct traceeval_dynamic	dyn_data;
> +		char				*string;
> +		const char			*cstring;
> +		void				*pointer;
> +		unsigned long			number;
> +		unsigned long long		number_64;
> +		unsigned int			number_32;
> +		unsigned short			number_16;
> +		unsigned char			number_8;
> +	};
>  };
>  
> +#define __TRACEEVAL_DATA(data_type, member, data)			\
> +	{  .type = TRACEEVAL_TYPE_##data_type, .member = (data) }
> +
> +#define DEFINE_TRACEEVAL_NUMBER(data)	   __TRACEEVAL_DATA(NUMBER, number, data)
> +#define DEFINE_TRACEEVAL_NUMBER_8(data)	   __TRACEEVAL_DATA(NUMBER_8, number_8, data)
> +#define DEFINE_TRACEEVAL_NUMBER_16(data)   __TRACEEVAL_DATA(NUMBER_16, number_16, data)
> +#define DEFINE_TRACEEVAL_NUMBER_32(data)   __TRACEEVAL_DATA(NUMBER_32, number_32, data)
> +#define DEFINE_TRACEEVAL_NUMBER_64(data)   __TRACEEVAL_DATA(NUMBER_64, number_64, data)
> +#define DEFINE_TRACEEVAL_STRING(data)	   __TRACEEVAL_DATA(STRING, string, data)
> +#define DEFINE_TRACEEVAL_CSTRING(data)	   __TRACEEVAL_DATA(STRING, cstring, data)
> +#define DEFINE_TRACEEVAL_POINTER(data)	   __TRACEEVAL_DATA(POINTER, pointer, data)
> +
> +#define __TRACEEVAL_SET(data, data_type, member, val)		\
> +	do {							\
> +		(data).type = TRACEEVAL_TYPE_##data_type;	\
> +		(data).member = (val);				\
> +	} while (0)
> +
> +#define TRACEEVAL_SET_NUMBER(data, val)	     __TRACEEVAL_SET(data, NUMBER, number, val)
> +#define TRACEEVAL_SET_NUMBER_8(data, val)    __TRACEEVAL_SET(data, NUMBER_8, number_8, val)
> +#define TRACEEVAL_SET_NUMBER_16(data, val)   __TRACEEVAL_SET(data, NUMBER_16, number_16, val)
> +#define TRACEEVAL_SET_NUMBER_32(data, val)   __TRACEEVAL_SET(data, NUMBER_32, number_32, val)
> +#define TRACEEVAL_SET_NUMBER_64(data, val)   __TRACEEVAL_SET(data, NUMBER_64, number_64, val)
> +#define TRACEEVAL_SET_STRING(data, val)	     __TRACEEVAL_SET(data, STRING, string, val)
> +#define TRACEEVAL_SET_CSTRING(data, val)     __TRACEEVAL_SET(data, STRING, cstring, val)
> +#define TRACEEVAL_SET_POINTER(data, val)     __TRACEEVAL_SET(data, POINTER, pointer, val)
> +
>  struct traceeval_type;
>  struct traceeval;
>  
>  /* release function callback on traceeval_data */
>  typedef void (*traceeval_data_release_fn)(const struct traceeval_type *type,
> -					  union traceeval_data *data);
> +					  struct traceeval_data *data);
>  
>  /* compare function callback to compare traceeval_data */
>  typedef int (*traceeval_data_cmp_fn)(struct traceeval *teval,
>  				     const struct traceeval_type *type,
> -				     const union traceeval_data *A,
> -				     const union traceeval_data *B);
> +				     const struct traceeval_data *A,
> +				     const struct traceeval_data *B);
>  
>  /* make a unique value */
>  typedef int (*traceeval_data_hash_fn)(struct traceeval *teval,
>  				      const struct traceeval_type *type,
> -				      const union traceeval_data *data);
> +				      const struct traceeval_data *data);
>  
>  typedef int (*traceeval_data_copy_fn)(const struct traceeval_type *type,
> -				      union traceeval_data *dst,
> -				      const union traceeval_data *src);
> +				      struct traceeval_data *dst,
> +				      const struct traceeval_data *src);
>  
>  typedef int (*traceeval_cmp_fn)(struct traceeval *teval,
> -				const union traceeval_data *Akeys,
> -				const union traceeval_data *Avals,
> -				const union traceeval_data *Bkeys,
> -				const union traceeval_data *Bvals,
> +				const struct traceeval_data *Akeys,
> +				const struct traceeval_data *Avals,
> +				const struct traceeval_data *Bkeys,
> +				const struct traceeval_data *Bvals,
>  				void *data);
>  
>  /*
> @@ -157,20 +187,20 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
>  void traceeval_release(struct traceeval *teval);
>  
>  int traceeval_insert(struct traceeval *teval,
> -		     const union traceeval_data *keys,
> -		     const union traceeval_data *vals);
> +		     const struct traceeval_data *keys,
> +		     const struct traceeval_data *vals);
>  
>  int traceeval_remove(struct traceeval *teval,
> -		     const union traceeval_data *keys);
> +		     const struct traceeval_data *keys);
>  
> -int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
> -		    const union traceeval_data **results);
> +int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
> +		    const struct traceeval_data **results);
>  
>  void traceeval_results_release(struct traceeval *teval,
> -			       const union traceeval_data *results);
> +			       const struct traceeval_data *results);
>  
>  struct traceeval_stat *traceeval_stat(struct traceeval *teval,
> -				      const union traceeval_data *keys,
> +				      const struct traceeval_data *keys,
>  				      struct traceeval_type *type);
>  
>  unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
> @@ -185,11 +215,11 @@ int traceeval_iterator_sort(struct traceeval_iterator *iter, const char *sort_fi
>  int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
>  				   traceeval_cmp_fn sort_fn, void *data);
>  int traceeval_iterator_next(struct traceeval_iterator *iter,
> -			    const union traceeval_data **keys);
> +			    const struct traceeval_data **keys);
>  int traceeval_iterator_query(struct traceeval_iterator *iter,
> -			     const union traceeval_data **results);
> +			     const struct traceeval_data **results);
>  void traceeval_iterator_results_release(struct traceeval_iterator *iter,
> -					const union traceeval_data *results);
> +					const struct traceeval_data *results);
>  struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
>  					       struct traceeval_type *type);
>  int traceeval_iterator_remove(struct traceeval_iterator *iter);
> diff --git a/samples/task-eval.c b/samples/task-eval.c
> index bde167d1219b..e62bfddafdd3 100644
> --- a/samples/task-eval.c
> +++ b/samples/task-eval.c
> @@ -190,21 +190,15 @@ static void update_process(struct task_data *tdata, const char *comm,
>  			   enum sched_state state, enum command cmd,
>  			   unsigned long long ts)
>  {
> -	union traceeval_data keys[] = {
> -		{
> -			.cstring	= comm,
> -		},
> -		{
> -			.number		= state,
> -		},
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	comm	),
> +		DEFINE_TRACEEVAL_NUMBER(	state	),
>  	};
> -	union traceeval_data vals[] = {
> -		{
> -			.number_64	= ts,
> -		},
> +	struct traceeval_data vals[] = {
> +		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
>  	};
> -	union traceeval_data new_vals[1] = { };
> -	const union traceeval_data *results;
> +	struct traceeval_data new_vals[1] = { };
> +	const struct traceeval_data *results;
>  	int ret;
>  
>  	switch (cmd) {
> @@ -222,14 +216,15 @@ static void update_process(struct task_data *tdata, const char *comm,
>  		if (!results[0].number_64)
>  			break;
>  
> -		new_vals[0].number_64 = ts - results[0].number_64;
> +		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
>  
>  		ret = traceeval_insert(tdata->teval_processes.stop, keys, new_vals);
>  		if (ret < 0)
>  			pdie("Could not stop process");
>  
>  		/* Reset the start */
> -		new_vals[0].number_64 = 0;
> +		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
> +
>  		ret = traceeval_insert(tdata->teval_processes.start, keys, new_vals);
>  		if (ret < 0)
>  			pdie("Could not start CPU");
> @@ -253,15 +248,11 @@ static void stop_process(struct task_data *tdata, const char *comm,
>  static struct process_data *
>  get_process_data(struct task_data *tdata, const char *comm)
>  {
> -	union traceeval_data keys[] = {
> -		{
> -			.cstring = comm,
> -		},
> -		{
> -			.number = RUNNING,
> -		}
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	comm	),
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
>  	};
> -	const union traceeval_data *results;
> +	const struct traceeval_data *results;
>  	void *data;
>  	int ret;
>  
> @@ -278,16 +269,12 @@ get_process_data(struct task_data *tdata, const char *comm)
>  
>  void set_process_data(struct task_data *tdata, const char *comm, void *data)
>  {
> -	union traceeval_data keys[] = {
> -		{
> -			.cstring = comm,
> -		},
> -		{
> -			.number = RUNNING,
> -		}
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	comm	),
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
>  	};
> -	union traceeval_data new_vals[1] = { };
> -	const union traceeval_data *results;
> +	struct traceeval_data new_vals[1] = { };
> +	const struct traceeval_data *results;
>  	int ret;
>  
>  	ret = traceeval_query(tdata->teval_processes_data, keys, &results);
> @@ -296,7 +283,7 @@ void set_process_data(struct task_data *tdata, const char *comm, void *data)
>  	if (ret < 0)
>  		pdie("Could not query process data");
>  
> -	new_vals[0].pointer = data;
> +	TRACEEVAL_SET_POINTER(new_vals[0], data);
>  	ret = traceeval_insert(tdata->teval_processes_data, keys, new_vals);
>  	if (ret < 0)
>  		pdie("Failed to set process data");
> @@ -309,21 +296,15 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
>  		       enum sched_state state, enum command cmd,
>  		       unsigned long long ts)
>  {
> -	const union traceeval_data *results;
> -	union traceeval_data keys[] = {
> -		{
> -			.number = cpu,
> -		},
> -		{
> -			.number = state,
> -		}
> +	const struct traceeval_data *results;
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_NUMBER(	cpu	),
> +		DEFINE_TRACEEVAL_NUMBER(	state	),
>  	};
> -	union traceeval_data vals[] = {
> -		{
> -			.number_64	= ts,
> -		},
> +	struct traceeval_data vals[] = {
> +		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
>  	};
> -	union traceeval_data new_vals[1] = { };
> +	struct traceeval_data new_vals[1] = { };
>  	int ret;
>  
>  	switch (cmd) {
> @@ -350,14 +331,14 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
>  		if (!results[0].number_64)
>  			break;
>  
> -		new_vals[0].number_64 = ts - results[0].number_64;
> +		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
>  
>  		ret = traceeval_insert(teval_pair->stop, keys, new_vals);
>  		if (ret < 0)
>  			pdie("Could not stop CPU");
>  
>  		/* Reset the start */
> -		new_vals[0].number_64 = 0;
> +		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
>  		ret = traceeval_insert(teval_pair->start, keys, new_vals);
>  		if (ret < 0)
>  			pdie("Could not start CPU");
> @@ -385,21 +366,15 @@ static void update_thread(struct process_data *pdata, int tid,
>  			  enum sched_state state, enum command cmd,
>  			  unsigned long long ts)
>  {
> -	const union traceeval_data *results;
> -	union traceeval_data keys[] = {
> -		{
> -			.number = tid,
> -		},
> -		{
> -			.number = state,
> -		}
> +	const struct traceeval_data *results;
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_NUMBER(	tid	),
> +		DEFINE_TRACEEVAL_NUMBER(	state	),
>  	};
> -	union traceeval_data vals[] = {
> -		{
> -			.number_64	= ts,
> -		},
> +	struct traceeval_data vals[] = {
> +		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
>  	};
> -	union traceeval_data new_vals[1] = { };
> +	struct traceeval_data new_vals[1] = { };
>  	int ret;
>  
>  	switch (cmd) {
> @@ -415,7 +390,7 @@ static void update_thread(struct process_data *pdata, int tid,
>  		if (ret == 0)
>  			return;
>  
> -		new_vals[0].number_64 = ts - results[0].number_64;
> +		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
>  
>  		ret = traceeval_insert(pdata->teval_threads.stop, keys, new_vals);
>  		traceeval_results_release(pdata->teval_threads.start, results);
> @@ -646,17 +621,19 @@ static void print_microseconds(int idx, unsigned long long nsecs)
>   *  If RUNNING is equal, then sort by COMM.
>   */
>  static int compare_pdata(struct traceeval *teval_data,
> -				const union traceeval_data *Akeys,
> -				const union traceeval_data *Avals,
> -				const union traceeval_data *Bkeys,
> -				const union traceeval_data *Bvals,
> +				const struct traceeval_data *Akeys,
> +				const struct traceeval_data *Avals,
> +				const struct traceeval_data *Bkeys,
> +				const struct traceeval_data *Bvals,
>  				void *data)
>  {
>  	struct traceeval *teval = data; /* The deltas are here */
> -	union traceeval_data keysA[] = {
> -		{ .cstring = Akeys[0].cstring }, { .number = RUNNING } };
> -	union traceeval_data keysB[] = {
> -		{ .cstring = Bkeys[0].cstring }, { .number = RUNNING } };
> +	struct traceeval_data keysA[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	Akeys[0].cstring	),
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
> +	struct traceeval_data keysB[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	Bkeys[0].cstring	),
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
>  	struct traceeval_stat *statA;
>  	struct traceeval_stat *statB;
>  	unsigned long long totalA = -1;
> @@ -690,7 +667,7 @@ static int compare_pdata(struct traceeval *teval_data,
>  static void display_cpus(struct traceeval *teval)
>  {
>  	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
> -	const union traceeval_data *keys;
> +	const struct traceeval_data *keys;
>  	struct traceeval_stat *stat;
>  	int last_cpu = -1;
>  
> @@ -762,7 +739,7 @@ static void display_state_times(int state, unsigned long long total)
>  static void display_threads(struct traceeval *teval)
>  {
>  	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
> -	const union traceeval_data *keys;
> +	const struct traceeval_data *keys;
>  	struct traceeval_stat *stat;
>  	int last_tid = -1;
>  
> @@ -802,17 +779,13 @@ static void display_process_stats(struct traceeval *teval,
>  {
>  	struct traceeval_stat *stat;
>  	unsigned long long delta;
> -	union traceeval_data keys[] = {
> -		{
> -			.cstring = comm,
> -		},
> -		{
> -			.number = RUNNING,
> -		}
> +	struct traceeval_data keys[] = {
> +		DEFINE_TRACEEVAL_CSTRING(	comm		),
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
>  	};
>  
>  	for (int i = 0; i < OTHER; i++) {
> -		keys[1].number = i;
> +		TRACEEVAL_SET_NUMBER_64(keys[1], i);

I think this should be 
+		TRACEEVAL_SET_NUMBER(keys[1], i);

to match the 
+		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),

a little up in this function.

Re: [PATCH v2 06/11] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Ross Zwisler]

On Mon, Oct 02, 2023 at 01:53:04PM -0600, Ross Zwisler wrote:
> On Wed, Sep 27, 2023 at 08:33:09AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > Having a straight union for passing in the data that must match the types
> > is dangerous and prone for buggy code. If some data doesn't match its
> > type, there's nothing to catch it.
> > 
> > Instead of having a union traceeval_data of each type, have it be a
> > structure with a "type" field follow by a union, where the type defines
> > what kind of data it is.
> > 
> > Add helper macros to make it easy to define the data when using it.
> > 
> > Reviewed-by: Ross Zwisler <zwisler@google.com>
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > ---
> >  include/traceeval-hist.h |  90 ++++++++++++++++---------
> >  samples/task-eval.c      | 139 ++++++++++++++++-----------------------
> >  src/eval-local.h         |   4 +-
> >  src/histograms.c         |  70 ++++++++++----------
> >  4 files changed, 153 insertions(+), 150 deletions(-)
> > 
> > diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> > index e619d52ea0d0..78cfac5982ee 100644
> > --- a/include/traceeval-hist.h
> > +++ b/include/traceeval-hist.h
> > @@ -52,45 +52,75 @@ struct traceeval_dynamic {
> >   * Trace data entry for a traceeval histogram
> >   * Constitutes keys and values.
> >   */
> > -union traceeval_data {
> > -	struct traceeval_dynamic	dyn_data;
> > -	char				*string;
> > -	const char			*cstring;
> > -	void				*pointer;
> > -	unsigned long			number;
> > -	unsigned long long		number_64;
> > -	unsigned int			number_32;
> > -	unsigned short			number_16;
> > -	unsigned char			number_8;
> > +struct traceeval_data {
> > +	enum traceeval_data_type		type;
> > +	union {
> > +		struct traceeval_dynamic	dyn_data;
> > +		char				*string;
> > +		const char			*cstring;
> > +		void				*pointer;
> > +		unsigned long			number;
> > +		unsigned long long		number_64;
> > +		unsigned int			number_32;
> > +		unsigned short			number_16;
> > +		unsigned char			number_8;
> > +	};
> >  };
> >  
> > +#define __TRACEEVAL_DATA(data_type, member, data)			\
> > +	{  .type = TRACEEVAL_TYPE_##data_type, .member = (data) }
> > +
> > +#define DEFINE_TRACEEVAL_NUMBER(data)	   __TRACEEVAL_DATA(NUMBER, number, data)
> > +#define DEFINE_TRACEEVAL_NUMBER_8(data)	   __TRACEEVAL_DATA(NUMBER_8, number_8, data)
> > +#define DEFINE_TRACEEVAL_NUMBER_16(data)   __TRACEEVAL_DATA(NUMBER_16, number_16, data)
> > +#define DEFINE_TRACEEVAL_NUMBER_32(data)   __TRACEEVAL_DATA(NUMBER_32, number_32, data)
> > +#define DEFINE_TRACEEVAL_NUMBER_64(data)   __TRACEEVAL_DATA(NUMBER_64, number_64, data)
> > +#define DEFINE_TRACEEVAL_STRING(data)	   __TRACEEVAL_DATA(STRING, string, data)
> > +#define DEFINE_TRACEEVAL_CSTRING(data)	   __TRACEEVAL_DATA(STRING, cstring, data)
> > +#define DEFINE_TRACEEVAL_POINTER(data)	   __TRACEEVAL_DATA(POINTER, pointer, data)
> > +
> > +#define __TRACEEVAL_SET(data, data_type, member, val)		\
> > +	do {							\
> > +		(data).type = TRACEEVAL_TYPE_##data_type;	\
> > +		(data).member = (val);				\
> > +	} while (0)
> > +
> > +#define TRACEEVAL_SET_NUMBER(data, val)	     __TRACEEVAL_SET(data, NUMBER, number, val)
> > +#define TRACEEVAL_SET_NUMBER_8(data, val)    __TRACEEVAL_SET(data, NUMBER_8, number_8, val)
> > +#define TRACEEVAL_SET_NUMBER_16(data, val)   __TRACEEVAL_SET(data, NUMBER_16, number_16, val)
> > +#define TRACEEVAL_SET_NUMBER_32(data, val)   __TRACEEVAL_SET(data, NUMBER_32, number_32, val)
> > +#define TRACEEVAL_SET_NUMBER_64(data, val)   __TRACEEVAL_SET(data, NUMBER_64, number_64, val)
> > +#define TRACEEVAL_SET_STRING(data, val)	     __TRACEEVAL_SET(data, STRING, string, val)
> > +#define TRACEEVAL_SET_CSTRING(data, val)     __TRACEEVAL_SET(data, STRING, cstring, val)
> > +#define TRACEEVAL_SET_POINTER(data, val)     __TRACEEVAL_SET(data, POINTER, pointer, val)
> > +
> >  struct traceeval_type;
> >  struct traceeval;
> >  
> >  /* release function callback on traceeval_data */
> >  typedef void (*traceeval_data_release_fn)(const struct traceeval_type *type,
> > -					  union traceeval_data *data);
> > +					  struct traceeval_data *data);
> >  
> >  /* compare function callback to compare traceeval_data */
> >  typedef int (*traceeval_data_cmp_fn)(struct traceeval *teval,
> >  				     const struct traceeval_type *type,
> > -				     const union traceeval_data *A,
> > -				     const union traceeval_data *B);
> > +				     const struct traceeval_data *A,
> > +				     const struct traceeval_data *B);
> >  
> >  /* make a unique value */
> >  typedef int (*traceeval_data_hash_fn)(struct traceeval *teval,
> >  				      const struct traceeval_type *type,
> > -				      const union traceeval_data *data);
> > +				      const struct traceeval_data *data);
> >  
> >  typedef int (*traceeval_data_copy_fn)(const struct traceeval_type *type,
> > -				      union traceeval_data *dst,
> > -				      const union traceeval_data *src);
> > +				      struct traceeval_data *dst,
> > +				      const struct traceeval_data *src);
> >  
> >  typedef int (*traceeval_cmp_fn)(struct traceeval *teval,
> > -				const union traceeval_data *Akeys,
> > -				const union traceeval_data *Avals,
> > -				const union traceeval_data *Bkeys,
> > -				const union traceeval_data *Bvals,
> > +				const struct traceeval_data *Akeys,
> > +				const struct traceeval_data *Avals,
> > +				const struct traceeval_data *Bkeys,
> > +				const struct traceeval_data *Bvals,
> >  				void *data);
> >  
> >  /*
> > @@ -157,20 +187,20 @@ struct traceeval *traceeval_init(struct traceeval_type *keys,
> >  void traceeval_release(struct traceeval *teval);
> >  
> >  int traceeval_insert(struct traceeval *teval,
> > -		     const union traceeval_data *keys,
> > -		     const union traceeval_data *vals);
> > +		     const struct traceeval_data *keys,
> > +		     const struct traceeval_data *vals);
> >  
> >  int traceeval_remove(struct traceeval *teval,
> > -		     const union traceeval_data *keys);
> > +		     const struct traceeval_data *keys);
> >  
> > -int traceeval_query(struct traceeval *teval, const union traceeval_data *keys,
> > -		    const union traceeval_data **results);
> > +int traceeval_query(struct traceeval *teval, const struct traceeval_data *keys,
> > +		    const struct traceeval_data **results);
> >  
> >  void traceeval_results_release(struct traceeval *teval,
> > -			       const union traceeval_data *results);
> > +			       const struct traceeval_data *results);
> >  
> >  struct traceeval_stat *traceeval_stat(struct traceeval *teval,
> > -				      const union traceeval_data *keys,
> > +				      const struct traceeval_data *keys,
> >  				      struct traceeval_type *type);
> >  
> >  unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
> > @@ -185,11 +215,11 @@ int traceeval_iterator_sort(struct traceeval_iterator *iter, const char *sort_fi
> >  int traceeval_iterator_sort_custom(struct traceeval_iterator *iter,
> >  				   traceeval_cmp_fn sort_fn, void *data);
> >  int traceeval_iterator_next(struct traceeval_iterator *iter,
> > -			    const union traceeval_data **keys);
> > +			    const struct traceeval_data **keys);
> >  int traceeval_iterator_query(struct traceeval_iterator *iter,
> > -			     const union traceeval_data **results);
> > +			     const struct traceeval_data **results);
> >  void traceeval_iterator_results_release(struct traceeval_iterator *iter,
> > -					const union traceeval_data *results);
> > +					const struct traceeval_data *results);
> >  struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
> >  					       struct traceeval_type *type);
> >  int traceeval_iterator_remove(struct traceeval_iterator *iter);
> > diff --git a/samples/task-eval.c b/samples/task-eval.c
> > index bde167d1219b..e62bfddafdd3 100644
> > --- a/samples/task-eval.c
> > +++ b/samples/task-eval.c
> > @@ -190,21 +190,15 @@ static void update_process(struct task_data *tdata, const char *comm,
> >  			   enum sched_state state, enum command cmd,
> >  			   unsigned long long ts)
> >  {
> > -	union traceeval_data keys[] = {
> > -		{
> > -			.cstring	= comm,
> > -		},
> > -		{
> > -			.number		= state,
> > -		},
> > +	struct traceeval_data keys[] = {
> > +		DEFINE_TRACEEVAL_CSTRING(	comm	),
> > +		DEFINE_TRACEEVAL_NUMBER(	state	),
> >  	};
> > -	union traceeval_data vals[] = {
> > -		{
> > -			.number_64	= ts,
> > -		},
> > +	struct traceeval_data vals[] = {
> > +		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
> >  	};
> > -	union traceeval_data new_vals[1] = { };
> > -	const union traceeval_data *results;
> > +	struct traceeval_data new_vals[1] = { };
> > +	const struct traceeval_data *results;
> >  	int ret;
> >  
> >  	switch (cmd) {
> > @@ -222,14 +216,15 @@ static void update_process(struct task_data *tdata, const char *comm,
> >  		if (!results[0].number_64)
> >  			break;
> >  
> > -		new_vals[0].number_64 = ts - results[0].number_64;
> > +		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
> >  
> >  		ret = traceeval_insert(tdata->teval_processes.stop, keys, new_vals);
> >  		if (ret < 0)
> >  			pdie("Could not stop process");
> >  
> >  		/* Reset the start */
> > -		new_vals[0].number_64 = 0;
> > +		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
> > +
> >  		ret = traceeval_insert(tdata->teval_processes.start, keys, new_vals);
> >  		if (ret < 0)
> >  			pdie("Could not start CPU");
> > @@ -253,15 +248,11 @@ static void stop_process(struct task_data *tdata, const char *comm,
> >  static struct process_data *
> >  get_process_data(struct task_data *tdata, const char *comm)
> >  {
> > -	union traceeval_data keys[] = {
> > -		{
> > -			.cstring = comm,
> > -		},
> > -		{
> > -			.number = RUNNING,
> > -		}
> > +	struct traceeval_data keys[] = {
> > +		DEFINE_TRACEEVAL_CSTRING(	comm	),
> > +		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
> >  	};
> > -	const union traceeval_data *results;
> > +	const struct traceeval_data *results;
> >  	void *data;
> >  	int ret;
> >  
> > @@ -278,16 +269,12 @@ get_process_data(struct task_data *tdata, const char *comm)
> >  
> >  void set_process_data(struct task_data *tdata, const char *comm, void *data)
> >  {
> > -	union traceeval_data keys[] = {
> > -		{
> > -			.cstring = comm,
> > -		},
> > -		{
> > -			.number = RUNNING,
> > -		}
> > +	struct traceeval_data keys[] = {
> > +		DEFINE_TRACEEVAL_CSTRING(	comm	),
> > +		DEFINE_TRACEEVAL_NUMBER(	RUNNING	),
> >  	};
> > -	union traceeval_data new_vals[1] = { };
> > -	const union traceeval_data *results;
> > +	struct traceeval_data new_vals[1] = { };
> > +	const struct traceeval_data *results;
> >  	int ret;
> >  
> >  	ret = traceeval_query(tdata->teval_processes_data, keys, &results);
> > @@ -296,7 +283,7 @@ void set_process_data(struct task_data *tdata, const char *comm, void *data)
> >  	if (ret < 0)
> >  		pdie("Could not query process data");
> >  
> > -	new_vals[0].pointer = data;
> > +	TRACEEVAL_SET_POINTER(new_vals[0], data);
> >  	ret = traceeval_insert(tdata->teval_processes_data, keys, new_vals);
> >  	if (ret < 0)
> >  		pdie("Failed to set process data");
> > @@ -309,21 +296,15 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
> >  		       enum sched_state state, enum command cmd,
> >  		       unsigned long long ts)
> >  {
> > -	const union traceeval_data *results;
> > -	union traceeval_data keys[] = {
> > -		{
> > -			.number = cpu,
> > -		},
> > -		{
> > -			.number = state,
> > -		}
> > +	const struct traceeval_data *results;
> > +	struct traceeval_data keys[] = {
> > +		DEFINE_TRACEEVAL_NUMBER(	cpu	),
> > +		DEFINE_TRACEEVAL_NUMBER(	state	),
> >  	};
> > -	union traceeval_data vals[] = {
> > -		{
> > -			.number_64	= ts,
> > -		},
> > +	struct traceeval_data vals[] = {
> > +		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
> >  	};
> > -	union traceeval_data new_vals[1] = { };
> > +	struct traceeval_data new_vals[1] = { };
> >  	int ret;
> >  
> >  	switch (cmd) {
> > @@ -350,14 +331,14 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
> >  		if (!results[0].number_64)
> >  			break;
> >  
> > -		new_vals[0].number_64 = ts - results[0].number_64;
> > +		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
> >  
> >  		ret = traceeval_insert(teval_pair->stop, keys, new_vals);
> >  		if (ret < 0)
> >  			pdie("Could not stop CPU");
> >  
> >  		/* Reset the start */
> > -		new_vals[0].number_64 = 0;
> > +		TRACEEVAL_SET_NUMBER_64(new_vals[0], 0);
> >  		ret = traceeval_insert(teval_pair->start, keys, new_vals);
> >  		if (ret < 0)
> >  			pdie("Could not start CPU");
> > @@ -385,21 +366,15 @@ static void update_thread(struct process_data *pdata, int tid,
> >  			  enum sched_state state, enum command cmd,
> >  			  unsigned long long ts)
> >  {
> > -	const union traceeval_data *results;
> > -	union traceeval_data keys[] = {
> > -		{
> > -			.number = tid,
> > -		},
> > -		{
> > -			.number = state,
> > -		}
> > +	const struct traceeval_data *results;
> > +	struct traceeval_data keys[] = {
> > +		DEFINE_TRACEEVAL_NUMBER(	tid	),
> > +		DEFINE_TRACEEVAL_NUMBER(	state	),
> >  	};
> > -	union traceeval_data vals[] = {
> > -		{
> > -			.number_64	= ts,
> > -		},
> > +	struct traceeval_data vals[] = {
> > +		DEFINE_TRACEEVAL_NUMBER_64(	ts	),
> >  	};
> > -	union traceeval_data new_vals[1] = { };
> > +	struct traceeval_data new_vals[1] = { };
> >  	int ret;
> >  
> >  	switch (cmd) {
> > @@ -415,7 +390,7 @@ static void update_thread(struct process_data *pdata, int tid,
> >  		if (ret == 0)
> >  			return;
> >  
> > -		new_vals[0].number_64 = ts - results[0].number_64;
> > +		TRACEEVAL_SET_NUMBER_64(new_vals[0], ts - results[0].number_64);
> >  
> >  		ret = traceeval_insert(pdata->teval_threads.stop, keys, new_vals);
> >  		traceeval_results_release(pdata->teval_threads.start, results);
> > @@ -646,17 +621,19 @@ static void print_microseconds(int idx, unsigned long long nsecs)
> >   *  If RUNNING is equal, then sort by COMM.
> >   */
> >  static int compare_pdata(struct traceeval *teval_data,
> > -				const union traceeval_data *Akeys,
> > -				const union traceeval_data *Avals,
> > -				const union traceeval_data *Bkeys,
> > -				const union traceeval_data *Bvals,
> > +				const struct traceeval_data *Akeys,
> > +				const struct traceeval_data *Avals,
> > +				const struct traceeval_data *Bkeys,
> > +				const struct traceeval_data *Bvals,
> >  				void *data)
> >  {
> >  	struct traceeval *teval = data; /* The deltas are here */
> > -	union traceeval_data keysA[] = {
> > -		{ .cstring = Akeys[0].cstring }, { .number = RUNNING } };
> > -	union traceeval_data keysB[] = {
> > -		{ .cstring = Bkeys[0].cstring }, { .number = RUNNING } };
> > +	struct traceeval_data keysA[] = {
> > +		DEFINE_TRACEEVAL_CSTRING(	Akeys[0].cstring	),
> > +		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
> > +	struct traceeval_data keysB[] = {
> > +		DEFINE_TRACEEVAL_CSTRING(	Bkeys[0].cstring	),
> > +		DEFINE_TRACEEVAL_NUMBER(	RUNNING			), };
> >  	struct traceeval_stat *statA;
> >  	struct traceeval_stat *statB;
> >  	unsigned long long totalA = -1;
> > @@ -690,7 +667,7 @@ static int compare_pdata(struct traceeval *teval_data,
> >  static void display_cpus(struct traceeval *teval)
> >  {
> >  	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
> > -	const union traceeval_data *keys;
> > +	const struct traceeval_data *keys;
> >  	struct traceeval_stat *stat;
> >  	int last_cpu = -1;
> >  
> > @@ -762,7 +739,7 @@ static void display_state_times(int state, unsigned long long total)
> >  static void display_threads(struct traceeval *teval)
> >  {
> >  	struct traceeval_iterator *iter = traceeval_iterator_get(teval);
> > -	const union traceeval_data *keys;
> > +	const struct traceeval_data *keys;
> >  	struct traceeval_stat *stat;
> >  	int last_tid = -1;
> >  
> > @@ -802,17 +779,13 @@ static void display_process_stats(struct traceeval *teval,
> >  {
> >  	struct traceeval_stat *stat;
> >  	unsigned long long delta;
> > -	union traceeval_data keys[] = {
> > -		{
> > -			.cstring = comm,
> > -		},
> > -		{
> > -			.number = RUNNING,
> > -		}
> > +	struct traceeval_data keys[] = {
> > +		DEFINE_TRACEEVAL_CSTRING(	comm		),
> > +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
> >  	};
> >  
> >  	for (int i = 0; i < OTHER; i++) {
> > -		keys[1].number = i;
> > +		TRACEEVAL_SET_NUMBER_64(keys[1], i);
> 
> I think this should be 
> +		TRACEEVAL_SET_NUMBER(keys[1], i);
> 
> to match the 
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
> 
> a little up in this function.

Ah, it looks like you fix this in the next patch.

Re: [PATCH v2 07/11] libtraceveal: Add type checks to traceeval_data vals and keys

[Ross Zwisler]

On Wed, Sep 27, 2023 at 08:33:10AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Now that the traceeval_data has a type, add checks to traceeval_insert()
> and traceveal_query() as well as traceeval_stat() to make sure the keys
> and vals match the types that were for the traceeval.
> 
> Which also caught a bug in task_eval.c display_process_stats().
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Reviewed-by: Ross Zwisler <zwisler@google.com>

Re: [PATCH v2 09/11] libtraceeval: Make traceeval_remove() check size of keys array

[Ross Zwisler]

On Wed, Sep 27, 2023 at 08:33:12AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Make traceeval_remove() into a macro to pass in the size of the keys
> array, and rename the function to traceeval_remove_size() that now takes
> the size of the keys array.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval-hist.h | 7 +++++--
>  src/histograms.c         | 7 +++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 7f48bb92cc96..804a0aaa631d 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -196,8 +196,11 @@ int traceeval_insert_size(struct traceeval *teval,
>  	traceeval_insert_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys), \
>  			      vals, TRACEEVAL_ARRAY_SIZE(vals))
>  
> -int traceeval_remove(struct traceeval *teval,
> -		     const struct traceeval_data *keys);
> +int traceeval_remove_size(struct traceeval *teval,
> +			  const struct traceeval_data *keys, size_t nr_keys);
> +
> +#define traceeval_remove(teval, keys)					\
> +	traceeval_remove_size(teval, keys, TRACEEVAL_ARRAY_SIZE(keys))
>  
>  int traceeval_query_size(struct traceeval *teval, const struct traceeval_data *keys,
>  			 size_t nr_keys, const struct traceeval_data **results);
> diff --git a/src/histograms.c b/src/histograms.c
> index ab8a560fe14d..28cf0d4ed225 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -977,13 +977,16 @@ int traceeval_insert_size(struct traceeval *teval,

I think we need to update the comments above this function to account for the
new name and the additional 'nr_keys' param, but other than that you can add:

Reviewed-by: Ross Zwisler <zwisler@google.com>

>   *         0 if it did not find an time matching @keys
>   *        -1 if there was an error.
>   */
> -int traceeval_remove(struct traceeval *teval,
> -		     const struct traceeval_data *keys)
> +int traceeval_remove_size(struct traceeval *teval,
> +			  const struct traceeval_data *keys, size_t nr_keys)
>  {
>  	struct hash_table *hist = teval->hist;
>  	struct entry *entry;
>  	int check;
>  
> +	if (teval->nr_key_types != nr_keys)
> +		return -1;
> +
>  	entry = NULL;
>  	check = get_entry(teval, keys, &entry);
>  
> -- 
> 2.40.1
> 

Re: [PATCH v2 10/11] libtraceeval: Make traceeval_stat() check size of keys array

[Ross Zwisler]

On Wed, Sep 27, 2023 at 08:33:13AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Make traceeval_stat() into a macro to pass in the size of the keys
> array, and rename the function to traceeval_stat_size() that now takes
> the size of the keys array.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Reviewed-by: Ross Zwisler <zwisler@google.com>

Re: [PATCH v2 11/11] libtraceeval: Only do stats on values marked with the STAT flag

[Ross Zwisler]

On Wed, Sep 27, 2023 at 08:33:14AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Add TRACEEVAL_FL_STAT to perform stats on the value, otherwise ignore it.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Reviewed-by: Ross Zwisler <zwisler@google.com>

Re: [PATCH v2 06/11] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Steven Rostedt]

On Mon, 2 Oct 2023 13:53:04 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > -		}
> > +	struct traceeval_data keys[] = {
> > +		DEFINE_TRACEEVAL_CSTRING(	comm		),
> > +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
> >  	};
> >  
> >  	for (int i = 0; i < OTHER; i++) {
> > -		keys[1].number = i;
> > +		TRACEEVAL_SET_NUMBER_64(keys[1], i);  
> 
> I think this should be 
> +		TRACEEVAL_SET_NUMBER(keys[1], i);
> 
> to match the 
> +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
> 
> a little up in this function.

You're correct, and I already fixed it. Just haven't posted the fix yet.

-- Steve

Re: [PATCH v2 06/11] libtraceeval histogram: Add type to traceeval_data and make it a structure

[Steven Rostedt]

On Mon, 2 Oct 2023 13:54:48 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > to match the 
> > +		DEFINE_TRACEEVAL_NUMBER(	RUNNING		),
> > 
> > a little up in this function.  
> 
> Ah, it looks like you fix this in the next patch.

Now I remember. Yeah, the next patch catches this bug! :-)

-- Steve