diff mbox series

[3/5] histograms: traceeval release

Message ID 20230728190515.23088-3-stevie.6strings@gmail.com (mailing list archive)
State Superseded
Headers show
Series [1/5] histograms: initial histograms interface | expand

Commit Message

Stevie Alvarez July 28, 2023, 7:04 p.m. UTC 
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(-)

Comments

Steven Rostedt July 28, 2023, 10:07 p.m. UTC | #1 
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
Ross Zwisler Aug. 2, 2023, 9:54 p.m. UTC | #2 
On Fri, Jul 28, 2023 at 03:04:38PM -0400, Stevie Alvarez 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)
>  {
> +	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]);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
>  
> +/**
> + * Deallocate all possible data stored within the entry.
> + */
> +static void clean_entry(struct entry *entry, struct traceeval *teval)
> +{
> +	if (!entry) 

Should we check for NULL 'teval' as well?

> +		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);

Probably good to set 'teval->hist = NULL' just for good hygiene.  This keeps
later users from accidentally having a use-after-free.
Steven Rostedt Aug. 2, 2023, 10:20 p.m. UTC | #3 
On Wed, 2 Aug 2023 15:54:03 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > +/**
> > + * Deallocate all possible data stored within the entry.
> > + */
> > +static void clean_entry(struct entry *entry, struct traceeval *teval)
> > +{
> > +	if (!entry)   
> 
> Should we check for NULL 'teval' as well?

No, because teval can't be NULL here. As to get here, we need to go through
traceeval_release() which does the check for !teval.

-- Steve
> 
> > +		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);
> > +}
Steven Rostedt Aug. 2, 2023, 10:21 p.m. UTC | #4 
On Fri, 28 Jul 2023 15:04:38 -0400
Stevie Alvarez <stevie.6strings@gmail.com> wrote:

> +/**
> + * 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);
> +	}
>  }

Nit, but it is usually cleaner to just have:

void traceeval_release(struct traceeval *teval)
{
	if (!teval)
		return;

	hist_table_release(teval);
	type_release(teval->def_keys);
	type_release(teval->def_vals);
	free(teval);
}


Less indents ;-)

-- Steve
diff mbox series

Patch

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)
 {
+	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]);
+			break;
+		default:
+			break;
+		}
+	}
+}
 
+/**
+ * Deallocate all possible data stored within the entry.
+ */
+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