Re: [PATCH v2 2/2] tracing: Shrink the size of struct ftrace_event_field

[Borislav Petkov <bp@alien8.de>]

On Fri, Jul 26, 2013 at 11:47:22PM -0400, Steven Rostedt wrote:
> On Sat, 2013-07-27 at 11:32 +0800, Li Zefan wrote:
>  
> >  struct event_filter {
> > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> > index 7d85429..d72694d 100644
> > --- a/kernel/trace/trace_events.c
> > +++ b/kernel/trace/trace_events.c
> > @@ -106,6 +106,9 @@ trace_find_event_field(struct ftrace_event_call *call, char *name)
> >  	return __find_event_field(head, name);
> >  }
> >  
> > +/* detect bit-field overflow */
> > +#define VERIFY_SIZE(type) WARN_ON(type > field->type)
> > +
> 
> One small nit. Move this macro definition into the function itself,
> right above the macro usage. That way it will be much easier to review
> in the future, as people don't need to go search for VERIFY_SIZE(). It
> will be right there with the usage. The aesthetics may be a bit off, but
> at least the code will be obvious at first glance at what is happening,
> and I think that's more important than the "look" of the code.
> 
> Also, it will be obvious that the variable name must match the field
> name.
> 
> Thanks,
> 
> -- Steve
> 
> >  static int __trace_define_field(struct list_head *head, const char *type,
> >  				const char *name, int offset, int size,
> >  				int is_signed, int filter_type)
> > @@ -120,13 +123,16 @@ static int __trace_define_field(struct list_head *head, const char *type,
> >  	field->type = type;
> >  
> >  	if (filter_type == FILTER_OTHER)
> > -		field->filter_type = filter_assign_type(type);
> > -	else
> > -		field->filter_type = filter_type;
> > +		filter_type = filter_assign_type(type);
> >  
> > +	field->filter_type = filter_type;
> >  	field->offset = offset;
> >  	field->size = size;
> > -	field->is_signed = is_signed;
> > +	field->is_signed = !!is_signed;
> > +
> > +	VERIFY_SIZE(filter_type);
> > +	VERIFY_SIZE(offset);
> > +	VERIFY_SIZE(size);

Isn't this wrap-a-macro-with-another-more-obscure-macro not a bit too
much?

I mean,
  	WARN_ON(filter_type > field->filter_type)

is much more readable than VERIFY_SIZE IMO.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--