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.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
