Re: [PATCH v15 02/23] tracing: Add 'hist' event trigger command

From: Steven Rostedt
Date: Wed Mar 02 2016 - 09:33:29 EST


On Fri, 26 Feb 2016 10:01:05 -0600
Tom Zanussi <tom.zanussi@xxxxxxxxxxxxxxx> wrote:


> +static int create_hitcount_val(struct hist_trigger_data *hist_data)
> +{
> + hist_data->fields[HITCOUNT_IDX] =
> + create_hist_field(NULL, HIST_FIELD_FL_HITCOUNT);
> + if (!hist_data->fields[HITCOUNT_IDX])
> + return -ENOMEM;
> +
> + if (WARN_ON(++hist_data->n_vals > TRACING_MAP_VALS_MAX))

Never put side effects in a WARN_ON() macro. Some configs define
WARN_ON() as a nop. Which would break this code. Always do the
following:

hist_data->n_vals++;

if (WARN_ON(hist_data->n_vals > TRACING_MAP_VALS_MAX))
> + return -EINVAL;

> +
> + return 0;
> +}
> +
> +static int create_val_fields(struct hist_trigger_data *hist_data,
> + struct trace_event_file *file)
> +{
> + int ret;
> +
> + ret = create_hitcount_val(hist_data);
> +
> + return ret;
> +}
> +
> +static int create_key_field(struct hist_trigger_data *hist_data,
> + unsigned int key_idx,
> + struct trace_event_file *file,
> + char *field_str)
> +{
> + struct ftrace_event_field *field = NULL;
> + unsigned long flags = 0;
> + unsigned int key_size;
> + int ret = 0;
> +
> + if (WARN_ON(key_idx >= TRACING_MAP_FIELDS_MAX))
> + return -EINVAL;
> +
> + flags |= HIST_FIELD_FL_KEY;
> +
> + field = trace_find_event_field(file->event_call, field_str);
> + if (!field) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + key_size = field->size;
> +
> + hist_data->fields[key_idx] = create_hist_field(field, flags);
> + if (!hist_data->fields[key_idx]) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + key_size = ALIGN(key_size, sizeof(u64));
> + hist_data->fields[key_idx]->size = key_size;
> + hist_data->key_size = key_size;
> + if (hist_data->key_size > HIST_KEY_SIZE_MAX) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + if (WARN_ON(++hist_data->n_keys > TRACING_MAP_KEYS_MAX))

Same here.

> + return -EINVAL;
> +
> + ret = key_size;
> + out:
> + return ret;
> +}
> +

-- Steve