Messages in this thread | | | Date | Sat, 7 Apr 2012 11:08:27 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH 02/15] tools/events: Add files to create libtraceevent.a |
| |
On Fri, Apr 06, 2012 at 06:15:44PM -0400, Steven Rostedt wrote: > > Yeah, the name "do_warning" is awkward. Can we call it emit_warning or > > _warning or whatever? > > As this is just a macro defined as a helper in this file (not global), > I'm not sure it needs to be changed. "emit" has two more characters to > type over "do" ;-)
I know you're lazy :-) but "do_warning"? That's just nasty. How about "_warning" or something?
[..]
> > I see you're using kernel-doc style comments. Is this aimed at providing > > documentation for this library? If so, good idea! > > That was the plan. It also forced me to document the API.
I like that. This'll probably give more users to the lib.
> > > + * pevent_buffer_init - init buffer for parsing > > > + * @buf: buffer to parse > > > + * @size: the size of the buffer > > > + * > > > + * For use with pevent_read_token(), this initializes the internal > > > + * buffer that pevent_read_token() will parse. > > > + */ > > > +void pevent_buffer_init(const char *buf, unsigned long long size) > > > +{ > > > + init_input_buf(buf, size); > > > +} > > > > Can't we unify pevent_buffer_init and init_input_buf()? I mean, it is > > useless to have that kind of indirection. > > Yeah I agree. This was more of a historical artifact. It use to be just > init_input_buf() then I needed to export it for other users. Instead of > doing a full rename, I cheesed out and did this.
:-)
> > In general, is "pevent" the prefix for this library's functions which > > are getting exported to users? > > Yep. It's the "namespace" used. But if people do not like it, we can > come up with another name. I just want it to be a short prefix.
Agreed, maybe even shorter: "te" for "traceevent" See, the namespace of the kernel userspace tools is not that polluted yet so two-letter namespaces are still up for grabs.
[..]
> > > + * pevent_print_funcs - print out the stored functions > > > + * @pevent: handle for the pevent > > > + * > > > + * This prints out the stored functions. > > > + */ > > > +void pevent_print_funcs(struct pevent *pevent) > > > +{ > > > + int i; > > > + > > > + if (!pevent->func_map) > > > + func_map_init(pevent); > > > + > > > + for (i = 0; i < (int)pevent->func_count; i++) { > > > > simply declare i as "unsigned int" and you don't need the cast here. > > I think I did that once but it caused problems elsewhere. But we can > look at it again.
It shouldn't since the 'i' is a local variable to that function and it doesn't prevent you from keeping pevent->func_count an unsigned int which is probably visible elsewhere.
[..]
> > > > Ok, enough for today. I'll review the rest only if you really want me to > > :-). > > > > Sure thanks for the comments now. The checkpatch clean ups should > probably be fixed before this goes in, and removing the breakpoint > function. But the other changes probably should wait. I would like what > goes in to be as close to what is in trace-cmd as possible. Perhaps, we > should add the cleanups as a separate patch. Just to have a nice > transition.
Ok, makes sense. Let's leave those for another day.
-- Regards/Gruss, Boris.
| |