Re: [PATCH 2/5] histograms: traceeval initialize

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 2 Aug 2023 15:07:21 -0600
Ross Zwisler <zwisler@google.com> wrote:

> > diff --git a/src/histograms.c b/src/histograms.c
> > new file mode 100644
> > index 0000000..13830e4
> > --- /dev/null
> > +++ b/src/histograms.c
> > @@ -0,0 +1,285 @@
> > +
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * libtraceeval histogram interface implementation.
> > + *
> > + * Copyright (C) 2023 Google Inc, Stevie Alvarez <stevie.6strings@gmail.com>
> > + */
> > +
> > +#include <histograms.h>
> > +#include <stdio.h>
> > +#include <stdbool.h>
> > +#include <string.h>
> > +#include <stdarg.h>
> > +
> > +/**
> > + * Iterate over @keys, which should be an array of struct traceeval_type's,
> > + * until reaching an instance of type TRACEEVAL_TYPE_NONE.
> > + * @i should be a declared integer type.
> > + */
> > +#define for_each_key(i, keys)	\
> > +	for (i = 0; (keys)[(i)].type != TRACEEVAL_TYPE_NONE; (i)++)
> > +
> > +/** A key-value pair */
> > +struct entry {
> > +	union traceeval_data	*keys;
> > +	union traceeval_data	*vals;
> > +};
> > +
> > +/** A table of key-value entries */
> > +struct hist_table {
> > +	struct entry	*map;
> > +	size_t		nr_entries;
> > +};
> > +
> > +/** Histogram */
> > +struct traceeval {
> > +	struct traceeval_type		*def_keys;
> > +	struct traceeval_type		*def_vals;
> > +	struct hist_table		*hist;
> > +};  
> 
> Should this be called something else?  'struct traceeval' is already defined
> in src/trace-analysis.c in this same codebase, and the other def is used all
> over the place in include/traceeval.h.

Yeah, we could call it something slightly different for now, but the goal
is that it will supersede the struct traceeval in trace-analysis.c, and the
code there will be removed.

We haven't hit version 1.0 yet because I hated the old API and until 1.0 is
reached, the API is allowed to change.

> 
> > +
> > +/** Iterate over results of histogram */
> > +struct traceeval_iterator {};  // TODO
> > +
> > +/**
> > + * Print error message.
> > + * Additional arguments are used with respect to fmt.
> > + */
> > +static void print_err(const char *fmt, ...)
> > +{
> > +	va_list ap;
> > +
> > +	va_start(ap, fmt);
> > +	vfprintf(stderr, fmt, ap);
> > +	va_end(ap);
> > +	fprintf(stderr, "\n");
> > +}
> > +
> > +// TODO
> > +int traceeval_compare(struct traceeval *orig, struct traceeval *copy)
> > +{
> > +	return -1;
> > +}
> > +
> > +/**
> > + * Resize a struct traceeval_type array to a size of @size + 1.
> > + *
> > + * Returns a pointer to the resized array, or NULL if the provided pointer was
> > + * freed to due lack of memory.
> > + */
> > +static struct traceeval_type *type_realloc(struct traceeval_type *defs,  
> 
> Probably should have been feedback for patch 1, but 'traceeval_type' also has
> a collision.  'struct traceeval_type' is defined in include/histograms.h, and
> 'enum traceeval_type' is defined in include/libtraceeval.h.
> 
> IMO even if the compiler lets us do this, we shouldn't because it'll confuse
> cscope/LSMs/grep and people reading the code.

Again, I think it may be fine, as we will eventually remove the old code.


> 
> > +		size_t size)
> > +{
> > +	struct traceeval_type *tmp_defs = NULL;
> > +	tmp_defs = realloc(defs,
> > +			(size + 1) * sizeof(struct traceeval_type));
> > +	if (!tmp_defs)
> > +		goto fail_type_realloc;
> > +	return tmp_defs;
> > +
> > +fail_type_realloc:
> > +	for (int i = 0; i < size; i++) {
> > +		if (defs[i].name)
> > +			free(defs[i].name);  
> 
> Because the extra entries returned by realloc() are uninitialized, I think you
> risk trying to free 'name' values that are non-NULL but not real pointers.
> 
> Should you instead try to iterate from 0 until you find an entry with type
> TRACEEVAL_TYPE_NONE?  Or do you guarantee elsewhere that all 'name' pointers
> are either valid or NULL?  If so, it might be good to do that initialization
> in this function so the free path makes sense in the same context and so new
> users of this function don't violate that assumption.

I believe I mentioned the same thing (or similar) in my reply.

-- Steve

> 
> > +	}
> > +	free(defs);
> > +	return NULL;
> > +}
> > +
> > +/**
> > + * Clone traceeval_type array @defs to the heap. Must be terminated with
> > + * an instance of type TRACEEVAL_TYPE_NONE.
> > + * Returns NULL if @defs is NULL, or a name is not null terminated.  
>