Re: [PATCH v13 09/29] tracing: Add 'hist' event trigger command

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 19 Feb 2016 17:27:50 -0600
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:

> > > +
> > > +struct hist_trigger_data {
> > > +	struct hist_field               *fields[TRACING_MAP_FIELDS_MAX];  
> > 
> > Hmm I'm confused, TRACING_MAP_FIELDS_MAX == 4
> > 
> > But below it we index into fields with n_fields. Which look to me can
> > be much bigger than 4.
> > 
> > Or is there some correlation between n_vals, n_keys and 4?
> >   
> 
> Yeah, the correlation is:
> 
>   n_fields = n_vals + n_keys
> 
> where n_keys can't be larger than TRACING_MAP_KEYS_MAX, and n_vals can't
> be larger than TRACING_MAP_FIELDS_MAX - TRACING_MAP_KEYS_MAX, where
> currently TRACING_MAP_KEYS_MAX = 2 and TRACING_MAP_FIELDS_MAX = 4, which
> means the max number of values is currently 2 as well.  So n_fields
> shouldn't be bigger than 4, but...
> 
> Looking through the code to make sure, I realized that it should be
> fields[TRACING_MAP_FIELDS_MAX + 1] because of the hitcount, which is an
> always-defined value field and takes the first slot in fields[]
> (followed by the rest of the vals, then the keys).  Previous versions
> had separate keys[] and vals[] arrays, but that was changed to be more
> uniform and somehow adding 1 for the hitcount was overlooked.  Obviously
> I'll fix that size, but the rest of the code should be ok wrt that.
> 

When you respin, can you add conditions like:

	if (WARN_ON(n_vals > 
		    TRACING_MAP_FIELDS_MAX - TRACING_MAP_KEYS_MAX))
		return;

where those values are updated. Same for n_keys.

Maybe even add some warnons where index is done, against
TRACING_MAP_FIELDS_MAX. As this accounting is not that trivial to keep
track of. I'd feel better if we have warnings to make sure the values
are capped at what we expect them to be capped at.

Thanks,

-- Steve