[systemd-devel] [PATCH] systemd-analyse: add "tree" command

[Lennart Poettering]

On Tue, 23.04.13 15:04, harald at redhat.com (harald at redhat.com) wrote:

> From: Harald Hoyer <harald at redhat.com>
> 
> "tree" prints a tree of the critical chain of units

Really nice work! However, maybe the name "tree" is suboptimal? You
already say "critical chain", maybe we should name it something like
this too? Esepcially, since by default it's not a tree at all... 

Maybe "systemd-analyze time-chain" or "systemd-analyze critical-chain"
or so? Anybody else got ideas?

> With the "--fuzz=<ms>" parameter one can display more units around
> the critical units.

As mentioned on Google+, we really should treat all time units uniformly
and encode them internally in usec_t, and parse them via parse_sec().

> +                <para><command>systemd-analyze tree</command> prints
> +                a tree of the critical chain of units.
> +                Note that the output might be misleading as the
> +                initialization of one service might depend on socket
> +                activation and because of the parallel execution
> +                of units.</para>

Needs to mention that this means "critical" as in "time" not as in
"importance"...

> +static int list_dependencies_print(const char *name, int level, unsigned int branches,
> +                                   bool last, struct unit_times *times, struct boot_times *boot) {
> +        int i;
> +        char ts[FORMAT_TIMESPAN_MAX], ts2[FORMAT_TIMESPAN_MAX];

Nitpick: in most areas of systemd we tried to use "unsigned" rather than
"int", where signedness never makes sense. "int" is more often than not
just the type people will default when they just need a variable, but I
think we should always think about the valid range of a variable when
declaring it.

> +
> +        for (i = level - 1; i >= 0; i--) {
> +                printf("%s", draw_special_char(branches & (1 << i) ? DRAW_TREE_VERT : DRAW_TREE_SPACE));
> +        }

Single-line blocks please without {}.

> +        for (i = 0; i < n; i++) {
> +                hashmap_put(h, times[i].name, &times[i]);
> +        }

Same here. Also, lacks failure check (OOM...).

Lennart

-- 
Lennart Poettering - Red Hat, Inc.