[PATCH] libtraceeval: Use name for type look up for traceeval_stat

[PATCH] libtraceeval: Use name for type look up for traceeval_stat

[Steven Rostedt]

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

The functions that retrieve the traceeval_stat currently requires passing
in the struct traceeval_type. But this is fragile, as it may not be set up
properly. It was originally done that way as a "fast" way to look up
stats, but in reality it will likely just cause more issues with wrong
types passed in. That decision was based more on defining policy which the
library should not do.

Instead, just pass in the string name of the value type to find and
retrieve the traceeval_data value from the traceeval that way.

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

diff --git a/include/traceeval.h b/include/traceeval.h
index 48c28dfc2f99..32996ba17e5f 100644
--- a/include/traceeval.h
+++ b/include/traceeval.h
@@ -219,7 +219,7 @@ size_t traceeval_count(struct traceeval *teval);
 struct traceeval_stat *traceeval_stat_size(struct traceeval *teval,
 					   const struct traceeval_data *keys,
 					   size_t nr_keys,
-					   struct traceeval_type *type);
+					   const char *val_name);
 
 unsigned long long traceeval_stat_max(struct traceeval_stat *stat);
 unsigned long long traceeval_stat_min(struct traceeval_stat *stat);
@@ -239,7 +239,7 @@ int traceeval_iterator_query(struct traceeval_iterator *iter,
 void traceeval_iterator_results_release(struct traceeval_iterator *iter,
 					const struct traceeval_data *results);
 struct traceeval_stat *traceeval_iterator_stat(struct traceeval_iterator *iter,
-					       struct traceeval_type *type);
+					       const char *val_name);
 int traceeval_iterator_remove(struct traceeval_iterator *iter);
 
 #endif /* __LIBTRACEEVAL_HIST_H__ */
diff --git a/src/histograms.c b/src/histograms.c
index 3ba7687af9bb..2be95e6a8d1f 100644
--- a/src/histograms.c
+++ b/src/histograms.c
@@ -849,17 +849,36 @@ static int update_entry(struct traceeval *teval, struct entry *entry,
 	return -1;
 }
 
+static struct traceeval_type *find_val_type(struct traceeval *teval, const char *name)
+{
+	struct traceeval_type *type;
+	int i;
+
+	for (i = 0; i < teval->nr_val_types; i++) {
+		type = &teval->val_types[i];
+
+		if (strcmp(type->name, name) == 0)
+			return type;
+	}
+	return NULL;
+}
+
 struct traceeval_stat *traceeval_stat_size(struct traceeval *teval,
 					   const struct traceeval_data *keys,
 					   size_t nr_keys,
-					   struct traceeval_type *type)
+					   const char *val_name)
 {
+	struct traceeval_type *type;
 	struct entry *entry;
 	int ret;
 
 	if (teval->nr_key_types != nr_keys)
 		return NULL;
 
+	type = find_val_type(teval, val_name);
+	if (!type)
+		return NULL;
+
 	if (!is_stat_type(type))
 		return NULL;
 
@@ -1116,12 +1135,9 @@ static struct traceeval_type *find_sort_type(struct traceeval *teval,
 	int i;
 
 	/* Check values first, and then keys */
-	for (i = 0; i < teval->nr_val_types; i++) {
-		type = &teval->val_types[i];
-
-		if (strcmp(type->name, name) == 0)
-			return type;
-	}
+	type = find_val_type(teval, name);
+	if (type)
+		return type;
 
 	for (i = 0; i < teval->nr_key_types; i++) {
 		type = &teval->key_types[i];
@@ -1426,16 +1442,21 @@ void traceeval_iterator_results_release(struct traceeval_iterator *iter,
 /**
  * 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
+ * @val_name: The name of the value 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)
+					       const char *val_name)
 {
+	struct traceeval_type *type;
 	struct entry *entry;
 
+	type = find_val_type(iter->teval, val_name);
+	if (!type)
+		return NULL;
+
 	if (!is_stat_type(type))
 		return NULL;
 
-- 
2.40.1

Re: [PATCH] libtraceeval: Use name for type look up for traceeval_stat

[Ross Zwisler]

On Tue, Oct 03, 2023 at 06:12:37PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The functions that retrieve the traceeval_stat currently requires passing
> in the struct traceeval_type. But this is fragile, as it may not be set up
> properly. It was originally done that way as a "fast" way to look up
> stats, but in reality it will likely just cause more issues with wrong
> types passed in. That decision was based more on defining policy which the
> library should not do.
> 
> Instead, just pass in the string name of the value type to find and
> retrieve the traceeval_data value from the traceeval that way.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  include/traceeval.h |  4 ++--
>  src/histograms.c    | 39 ++++++++++++++++++++++++++++++---------
>  2 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/include/traceeval.h b/include/traceeval.h
> index 48c28dfc2f99..32996ba17e5f 100644
> --- a/include/traceeval.h
> +++ b/include/traceeval.h
> @@ -219,7 +219,7 @@ size_t traceeval_count(struct traceeval *teval);
>  struct traceeval_stat *traceeval_stat_size(struct traceeval *teval,
>  					   const struct traceeval_data *keys,
>  					   size_t nr_keys,
> -					   struct traceeval_type *type);
> +					   const char *val_name);

Probably need to also s/type/val_name/g for the traceeval_stat() macro so they
stay in sync, and need to update the calls to traceeval_iterator_stat() and
traceeval_stat() in samples/task-eval.c so they continue to work.

Re: [PATCH] libtraceeval: Use name for type look up for traceeval_stat

[Steven Rostedt]

On Wed, 4 Oct 2023 08:56:54 -0600
Ross Zwisler <zwisler@google.com> wrote:


> Probably need to also s/type/val_name/g for the traceeval_stat() macro so they
> stay in sync, and need to update the calls to traceeval_iterator_stat() and

Makes sense.

> traceeval_stat() in samples/task-eval.c so they continue to work.

Ah, I had that as a separate patch (never sent it). But it makes more sense
to just fold it into this one.

-- Steve