[systemd-devel] [PATCH][RFC][V2] systemd-analyze: rewrite in C. (Was: systemd-analyze-197 broken)

[Lennart Poettering]

On Sun, 20.01.13 18:00, Peeters Simon (peeters.simon at gmail.com) wrote:

> Hej all,
> 
> Because of the discution about the python dependencies for systemd-analyze
> I made a rewrite in C.
> 
> The patch is available from my github repo[1] so please test it.
> (especially efi systems with gummyboot since I am still stuck with
> BIOS)
>
> 
> What is new about this c implementation?
>  - written in C.
>  - no deps on graphical libraries.
>  - fixes a bug in 'blame' where systemd-analyze would skip oneshot services.
>  - generates cleaner svg files.
>  - can serve as a basis for further bootchart/analyze intgration.
>  - faster[2]: (numbers from my netbook)
>                    C         Python
>     systemd-analyze time
>         real    0m0.052s    0m1.419s
>         user    0m0.020s    0m0.427s
>         sys     0m0.013s    0m0.100s
> 
>     systemd-analyze blame
>         real    0m3.852s    0m10.225s
>         user    0m0.990s    0m7.850s
>         sys     0m0.213s    0m0.980s
> 
>     systemd-analyze plot
>         real    0m3.861s    0m11.824s
>         user    0m1.030s    0m9.203s
>         sys     0m0.220s    0m1.030s
> 
> Changes between this version and the one Auke published:
>  - code deduplication.
>  - removal of unnescecary copy pasted code.
>  - fixed the 'blame' bug mentioned above.

Looks pretty good to me. But next time, please send this to the ML
again, so that I can review inline!

Before I merge this a few (minor) comments:

Try to keep global variables at a minimum, but if you do use them they
definitely need a "static". That might not strictly be necessary in
"main" programs, but is generally a good idea thre too.

"float"s make very little sense on modern computers. Always use
"doubles", as that's what CPUs use, what printf prints and so on. The
only valid reason to use "floats" if you have huge array of them and
want so save some bits.

Functions need to be static too, unless exported.

When you allocate a variable it is fine to initialize it to a constant
value, but try to avoid initializing them with a function call. This
is only allowed in newer C specs, and I am not sure that it's that good
an extension, since it doesn't help readability.

property_getull() appears a bit like overkill, we know the monotonic
times are 64bit entities, unsigned.

Instead of this:

if (...) {
        something();
}

We prefer this:

if (...) 
        something();

I'd really like to see some additional error checking. For example,
property_getull() currently returns an unedefined value when it
misparses something. Skipping error checking is OK in a few cases where
it is sufficiently clear that the error doesn't matter mutch or one
couldn't do anything about it anway. But in that case you still have to
return something, and returning un-initialized variables is never OK.

The last thing is the only thing I'd really really like see fixed before
we merge this, the other stuff is just nitpicking...

Lennart

-- 
Lennart Poettering - Red Hat, Inc.