On Sun, 20.01.13 18:00, Peeters Simon (peeters.si...@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.
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
