Re: [ANNOUNCE] Linux Trace Toolkit 0.9.5

[Roman Zippel]

Hi,

Karim Yaghmour wrote:

> > You should consider using more inline functions, instead of lots of "do
> > {...} while(0)" macros.
> 
> Sure, but what particular advantages do you see in this case?

Type safety, it's also easier to read and edit.

> > Do we really need more usages of uint32_t or uint8_t in the kernel?
> 
> I'd really like to avoid using them, but I don't see any other way
> since the binary traces have to be readable on multiple machine
> architectures and, indeed, multiple OSes. pid_t, for example, isn't
> a universally agreed upon type, hence the need for uintXYZ_t.

There is u32 or u8, which is usually used in the kernel. In user space
you can map them to whatever you like.

> > Instead of using lots of "#ifdef __arch__" you should move this into
> > <asm/trace.h>.
> 
> I hadn't thought of it, thanks. Actually, there are only 2 files needing
> this, drivers/trace/tracer.h and drivers/trace/tracer.c. The variations
> among architectures in tracer.h will be dropped in any case in the near
> future. So there's only tracer.c who has a portion of isolated code which
> has lots of "#ifdef __arch__". Given that this code is very isolated (i.e.
> in only one segment of one function) and there will never be any need for
> it to propagate elsewhere in the trace code, is it worth it to create an
> <asm/trace.h> which will only have 2 lines for most archs?

It might be useful for other things, e.g. for using something more
efficient than do_gettimeofday.

> > Comments are nice, but IMO your code does a bit too much of it, e.g.:
> 
> I understand your point of view, but there is a rational behind this
> way of coding. The purpose is to have the code and the explanation in
> the same place. So to come back to the snippet you were refering to:
> 
> >   /* Everything is OK */
> >   return 0;
> 
> I often see return's without explanations. The question is then: what
> does the value returned mean? This is what this comment is about.

You already have this above the function and it describes the return
value at single place and not all over the function.

> But I don't want to get into a religious fight over code commentary.
> The point is, I've been maintaining the kernel and user-space code this
> way for a while and it's paid off (i.e. most people reading the code
> understand it the first way through). If the comments weren't in sync
> with the code I'd agree that comments are dangerous, but this isn't
> the case so I don't see that there is any harm in this level of
> commenting.

IMO most comments are too obvious, I think it would be useful to
describe the more general things (e.g. like locking strategies) at a
single place and comments in functions should concentrate more on the
nonobvious stuff.

> BTW, what about an m68k port? ;)

As soon as I need it. :)

BTW to make it easier for other people to review your patch, I suggest
you include a direct link next time. Splitting the patch in a core part
and tracepoints part also helps reading the patch.

bye, Roman