On Tue, Aug 15, 2023 at 07:38:52PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@xxxxxxxxxxx>
>
> 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.
>
> Note, most of this patch was just a find and replace of "union" to
> "struct", and then the added helper macros, and the updates to the sample
> code. Nothing else was modified to get it to work.
>
> Note, there isn't a "dynamic" helper code, as I still do not know of a use
> case for it.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
> ---
>
> Ross, as we discussed on our 1:1, I think this is the better way to go. It
> labels the data with their types, and I even think the helper macros look
> better (just by looking at the sample code).
>
> include/traceeval-hist.h | 71 +++++++++++++++++++++---------------
> samples/task-eval.c | 78 ++++++++++++++++++----------------------
> src/eval-local.h | 4 +--
> src/histograms.c | 66 +++++++++++++++++-----------------
> 4 files changed, 112 insertions(+), 107 deletions(-)
>
> diff --git a/include/traceeval-hist.h b/include/traceeval-hist.h
> index 8e5a6451f399..d0bee7c02aa0 100644
> --- a/include/traceeval-hist.h
> +++ b/include/traceeval-hist.h
> @@ -52,45 +52,60 @@ 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)
> +
> 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 *copy,
> - const union traceeval_data *origin);
> + struct traceeval_data *copy,
> + const struct traceeval_data *origin);
>
> 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 +172,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,6 +200,6 @@ 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);
>
> #endif /* __LIBTRACEEVAL_HIST_H__ */
> diff --git a/samples/task-eval.c b/samples/task-eval.c
> index e0ef2d0bcb30..c2950b2d64b0 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) {
> @@ -251,15 +245,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;
>
> @@ -274,7 +264,7 @@ 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[] = {
> + struct traceeval_data keys[] = {
> {
> .cstring = comm,
> },
Is there a reason you stop using the macros to define your
traceeval_data array elements? Or a better question: Are these struct
traceeval_data instances exempt to needing a type field? And if so, why?
-- Stevie
> @@ -282,8 +272,8 @@ void set_process_data(struct task_data *tdata, const char *comm, void *data)
> .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);
> @@ -303,8 +293,8 @@ 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[] = {
> + const struct traceeval_data *results;
> + struct traceeval_data keys[] = {
> {
> .number = cpu,
> },
> @@ -312,12 +302,12 @@ static void update_cpu(struct teval_pair *teval_pair, int cpu,
> .number = state,
> }
> };
> - union traceeval_data vals[] = {
> + struct traceeval_data vals[] = {
> {
> .number_64 = ts,
> },
> };
> - union traceeval_data new_vals[1] = { };
> + struct traceeval_data new_vals[1] = { };
> int ret;
>
> switch (cmd) {
> @@ -375,8 +365,8 @@ 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[] = {
> + const struct traceeval_data *results;
> + struct traceeval_data keys[] = {
> {
> .number = tid,
> },
> @@ -384,12 +374,12 @@ static void update_thread(struct process_data *pdata, int tid,
> .number = state,
> }
> };
> - union traceeval_data vals[] = {
> + struct traceeval_data vals[] = {
> {
> .number_64 = ts,
> },
> };
> - union traceeval_data new_vals[1] = { };
> + struct traceeval_data new_vals[1] = { };
> int ret;
>
> switch (cmd) {
> @@ -634,16 +624,16 @@ 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[] = {
> + struct traceeval_data keysA[] = {
> { .cstring = Akeys[0].cstring }, { .number = RUNNING } };
> - union traceeval_data keysB[] = {
> + struct traceeval_data keysB[] = {
> { .cstring = Bkeys[0].cstring }, { .number = RUNNING } };
> struct traceeval_stat *statA;
> struct traceeval_stat *statB;
> @@ -678,7 +668,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;
>
> @@ -750,7 +740,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;
>
> @@ -790,7 +780,7 @@ static void display_process_stats(struct traceeval *teval,
> {
> struct traceeval_stat *stat;
> unsigned long long delta;
> - union traceeval_data keys[] = {
> + struct traceeval_data keys[] = {
> {
> .cstring = comm,
> },
> @@ -814,13 +804,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;
>
> @@ -843,7 +833,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 5c3918f17cc1..848851f7ffc4 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 3f43f7201b65..a7d0643cfbbd 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,
> - const union traceeval_data *orig,
> - union traceeval_data *copy)
> + const struct traceeval_data *orig,
> + struct traceeval_data *copy)
> {
> 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);
> @@ -624,9 +624,9 @@ static void data_release_and_free(size_t size, union traceeval_data **data,
> * Returns 1 on success, -1 on error.
> */
> static int copy_traceeval_data_set(size_t size, struct traceeval_type *type,
> - const union traceeval_data *orig,
> + const struct traceeval_data *orig,
> struct traceeval_stat *stats,
> - union traceeval_data **copy)
> + 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);
> @@ -778,12 +778,12 @@ fail_entry:
> * Return 1 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;
> size_t i;
>
> @@ -806,7 +806,7 @@ fail:
> }
>
> 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;
> @@ -910,8 +910,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;
> @@ -942,7 +942,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;
> @@ -1138,8 +1138,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];
>
> @@ -1225,7 +1225,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;
> --
> 2.40.1
>
|