I'd like to contribute but I don't know how...

[Alex Arvelaez]

On Wed, Feb 21, 2018 at 10:20:07PM -0500, valdis.kletnieks at vt.edu wrote:
> On Thu, 22 Feb 2018 02:33:08 +0000, Alex Arvelaez said:
> > Hello,
> >
> > I'd like to contribute to the linux kernel eventually but I'm not sure
> 
> https://lists.kernelnewbies.org/pipermail/kernelnewbies/2017-April/017765.html
> 
> > how, I grabbed a copy of the source code and I found a FIXME that looks
> > like I could fix:
> >
> > /* File: /usr/src/linux/tools/perf/util/string.c
> >  * FIXME: replace this with an expression using log10() when we
> >  * find a suitable implementation, maybe the one in the dvb drivers...
> >  */
> 
> Step 0:  Verify that the comment still matches the code, *and* that the change
> is still desired.  Hint:  Why do they want log10()? What does the current code
> do? What, if anything, will break if you change it?

It's in a function that creates boolean expressions from an array of
integers and it looks they hardcoded 28 bytes per each number. I think the
intent of using log10() is to only allocate as many bytes as the largest
number so expressions with lower numbers(in magnitude) would use less
memory.

> 
> Do you understand *why* '28' was used? And why they didn't just go
> ahead and use the perfectly usable log10() found in -lm ?

I'll look into this, I think they are including stdlib.h so it's a very
good point. I think they used 28 to be on the safe side 28 bytes gives
room for 27 digits per number.

They also put the formula they're trying to approximate:

/* "%s == %d || " = log10(MAXINT) * 2 + 8 chars for the operators */

> For bonus points, understand the code's behavior (or misbehavior) on 32 versus
> 64 bit architectures, and whether or not that's actually a problem.

I don't think that would be a problem but I have to take a closer look
at the implementation of log10().

> For extra bonus points, figure out how long that FIXME has been there.  And why.

The commit is from 2015, I couldn't find much about it on the lkml.org
archives. According the commit message the function's used to generate
to generate expressions to filter syscalls when tracing them:

perf -e read,write,fork //we only track specified syscalls in this case

> (Note that some of these are things that I don't know the answer to offhand. ;)
> 
> > I found the implementation they mention, is it okay to just copy paste
> > it into the file? I'm not sure where else to ask this kind of question...
> 
> If it's going to be used in more than one place, it should be refactored
> into a function, and both usage sites reworked to call the function rather
> than inline.

It's only a couple functions, they defined log10() in terms of log2().
I don't really know how to avoid inlining though, I'll look into it.

> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies