linux-kernel - Re: [PATCH v2 2/2] tracing: Shrink the size of struct ftrace_event

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130727084528.GA6923@pd.tnic>
Date:	Sat, 27 Jul 2013 10:45:28 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Li Zefan <lizefan@...wei.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] tracing: Shrink the size of struct
 ftrace_event_field

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/