Re: [PATCH 1/3] tracing/ftrace: implement a set_flag callback for tracers

[Ingo Molnar <mingo@elte.hu>]

i've got some syntactic nits:

> +	/*
> +	 * Increase the size with names of options specific
> +	 * of the current tracer.
> +	 */
> +	if (trace_opts) {
> +		for (i = 0; trace_opts[i].name; i++) {
> +			len += strlen(trace_opts[i].name);
> +			len += 3;

magic constant '3' needs a comment at least.

> +	if (trace_opts) {
> +		for (i = 0; trace_opts[i].name; i++) {
> +			if (tracer_flags & trace_opts[i].bit)
> +				r += sprintf(buf + r, "%s ",
> +					trace_opts[i].name);
> +			else
> +				r += sprintf(buf + r, "no%s ",
> +					trace_opts[i].name);
> +		}
> +	}

how about making a dummy empty trace_opts for all tracers (set by 
plugin init if the init function finds a NULL there) - that way the 
'if (trace_opts)' can go away and you can just iterate away on that 
array of options.

> +		if (strncmp(cmp, opts->name, len) == 0) {
> +			if (trace->set_flag)
> +					ret = trace->set_flag(trace_flags->val,
> +							opts->bit, !neg);

one tab too much. Also, the condition can be avoided by creating a 
dummy ->set_flag() that just returns 0.

> @@ -2469,6 +2536,7 @@ tracing_trace_options_write(struct file *filp, const char __user *ubuf,
>  	char *cmp = buf;
>  	int neg = 0;
>  	int i;
> +	int ret;

variable should be one line higher, to not break the upside down 
christmas tree layout. [ :-) ]

> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -259,6 +259,29 @@ enum print_line_t {
>  	TRACE_TYPE_UNHANDLED	= 2	/* Relay to other output functions */
>  };
>  
> +
> +/*
> + * An option specific to a tracer. This is a boolean value.
> + * The bit is the bit index that sets its value on the
> + * flags value in struct tracer_flags.
> + */
> +struct tracer_opt {
> +	const char *name; /* Will appear on the trace_options file */
> +	u32 bit; /* The mask assigned in val field in tracer_flags */
> +};

> +
> +/*
> + * The set of specific options for a tracer. Your tracer
> + * have to set the initial value of the flags val.
> + */
> +struct tracer_flags {
> +	u32	val;
> +	struct tracer_opt *opts;
> +};

please look at the file you modify and try to clone the micro-style it 
follows. For example in this file it would be more standard to use 
vertical alignment for structure definitions:

> +struct tracer_flags {
> +	u32			val;
> +	struct tracer_opt	*opts;
> +};

> +
> +/* Makes more easy to define a tracer opt */
> +#define TRACER_OPT(s, b) .name = #s, .bit = b

ditto.

>  #endif
>  	enum print_line_t	(*print_line)(struct trace_iterator *iter);
> +	/* If you handled the flag setting, return 0 */
> +	int			(*set_flag)(u32 old_flags, u32 bit,
> +							int set);
>  	struct tracer		*next;
>  	int			print_max;
> +	struct tracer_flags *flags;

ditto - here the new field deviates from he existing style.

also note how the 'int set);' line is unnecessarily broken to a 
separate line - it could be in the same line where the set_flag field 
begins.

> @@ -484,6 +511,7 @@ enum trace_iterator_flags {
>  #define TRACE_ITER_SYM_MASK \
>  	(TRACE_ITER_PRINT_PARENT|TRACE_ITER_SYM_OFFSET|TRACE_ITER_SYM_ADDR)
>  
> +
>  extern struct tracer nop_trace;

spurious looking newline insertion.

	Ingo