Re: [PATCH 3/5] histograms: traceeval release

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 28 Jul 2023 15:04:38 -0400
Stevie Alvarez <stevie.6strings@gmail.com> wrote:

> From: "Stevie Alvarez (Google)" <stevie.6strings@gmail.com>
> 
> traceeval_release() deconstructs a given struct traceeval instance. It
> frees any data allocated to the heap within the union traceeval_data
> arrays of entries to the histogram, and the names allocated for the
> struct traceeval_type key-value definitions.
> 
> Signed-off-by: Stevie Alvarez (Google) <stevie.6strings@gmail.com>
> ---
>  src/histograms.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/src/histograms.c b/src/histograms.c
> index 13830e4..f46a0e0 100644
> --- a/src/histograms.c
> +++ b/src/histograms.c
> @@ -209,10 +209,93 @@ fail_eval_init_unalloced:
>  	return NULL;
>  }
>  
> -// TODO
> -void traceeval_release(struct traceeval *teval)
> +/**
> + * Deallocate array of traceeval_type's, which must be terminated by
> + * TRACEEVAL_TYPE_NONE.
> + */
> +static void type_release(struct traceeval_type *defs)

So if we keep track of the number of defs, we should just pass in the
size. And ignore the NONE.

>  {
> +	size_t i = 0;
> +
> +	if (!defs)
> +		return;
> +
> +	for_each_key(i, defs) {
> +		if (defs[i].name)
> +			free(defs[i].name);
> +	}
> +
> +	free(defs);
> +}
> +
> +/**
> + * Deallocate any specified dynamic data in @data.
> + */
> +static void clean_data(union traceeval_data *data, struct traceeval_type *def) +{
> +	size_t i = 0;
> +
> +	if (!data || !def)
> +		return;
> +
> +	for_each_key(i, def) {
> +		switch (def[i].type) {
> +		case TRACEEVAL_TYPE_STRING:
> +			if (data[i].string)
> +				free(data[i].string);
> +			break;
> +		case TRACEEVAL_TYPE_DYNAMIC:
> +			def[i].dyn_release(data[i].dyn_data, &def[i]);

Note, it should be OK if the dynamic event does not have a release
function. This should be:

			if (def[i].dyn_release)
				def[i].dyn_release(data[i].dyn_data, &def[i]);

> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
>  
> +/**
> + * Deallocate all possible data stored within the entry.

Use the word "Free" and not "Deallocate". This goes for all other
places.

-- Steve

> + */
> +static void clean_entry(struct entry *entry, struct traceeval *teval)
> +{
> +	if (!entry)
> +		return;
> +
> +	// deallocate dynamic traceeval_data
> +	clean_data(entry->keys, teval->def_keys);
> +	clean_data(entry->vals, teval->def_vals);
> +	free(entry->keys);
> +	free(entry->vals);
> +}
> +
> +/**
> + * Deallocate the hist_table allocated to a traceeval instance.
> + */
> +static void hist_table_release(struct traceeval *teval)
> +{
> +	struct hist_table *hist = teval->hist;
> +
> +	if (!hist)
> +		return;
> +
> +	for (size_t i = 0; i < hist->nr_entries; i++) {
> +		clean_entry(&hist->map[i], teval);
> +	}
> +	free(hist->map);
> +	free(hist);
> +}
> +
> +/**
> + * Deallocate a traceeval instance.
> + */
> +void traceeval_release(struct traceeval *teval)
> +{
> +	if (teval) {
> +		hist_table_release(teval);
> +		type_release(teval->def_keys);
> +		type_release(teval->def_vals);
> +		free(teval);
> +	}
>  }
>  
>  // TODO