[systemd-devel] [PATCH v2] journalctl: Add support for showing messages from a previous boot

[Lennart Poettering]

On Thu, 30.05.13 17:24, Jan Janssen (medhefgo at web.de) wrote:

I like this idea!

> The format to specify the boot ID is inspired by git's ^n syntax
> and it even allows to look into the future.
> 
> Unfortunately, to get a chronological list of boot IDs, we
> need search through the journal. sd_journal_enumerate_unique()
> doesn't help us here, because order of returned values is undefined.
> 
> To make things less painful, an initial search for the reference
> boot ID is performed, which will either quickly fail so we don't have
> to needlessly walk the full journal or give us a cursor from which
> to start the slow lookup process.

Hmm, I think this should be implemented differently: we should define a
new message with a fixed message ID which is ussed once during boot,
which we then can search for. We already have
SD_MESSAGE_STARTUP_FINISHED which kinda does that, but is generated only
after startup finished. For this feature we should have a message that
is generated as early as possible in the boot process as possible
(i.e. right after the journal is up), and from PID1, and only once
during boot. We'd then simply search for this message ID in the
database, and this would return a nicely ordered list of boots. We then
pick the one we want and use it in an entirely new query.

> +                                <term><option>-b <optional><replaceable>ID</replaceable></optional></option></term>
> +                                <term><option>--boot=<optional><replaceable>ID</replaceable></optional></option></term>
> +
> <term><option>--this-boot=<optional><replaceable>ID</replaceable></optional></option></term>

If --this-boot= becomes the old obsolete name here it shouldn't show up
in the man page.

>  
> -                                <listitem><para>Show data only from
> -                                current boot. This will add a match
> -                                for <literal>_BOOT_ID=</literal> for
> -                                the current boot ID of the
> -                                kernel.</para></listitem>
> +                                <listitem><para>Show messages from specified
> +                                boot <replaceable>ID</replaceable>. This will
> +                                add a match for <literal>_BOOT_ID=</literal>.</para>
> +
> +                                <para>The argument is a 128 bit ID
> +                                optionally followed by the ancestry identifier
> +                                <literal>^n</literal>, which identifies the
> +                                chronologically nth previous boot ID. Supplying
> +                                a negative value will look for the chronologically
> +                                next boot ID. <literal>n</literal> may be ommitted,
> +                                in which case 1 is assumed. A value of 0 is
> +                                equivalent to the current boot ID. If the ancestry
> +                                indentifier is supplied, the boot ID itself may be
> +                                ommited and the current boot is
> -                                assumed.</para></listitem>

I am not sure I really like the "^" syntax. This after all is different
from git, as the "^" would works strictly by time, there is no real
"ancestral" information. (or in other words: the result of ^ differs
when you use different filters...).

Maybe we can use a different syntax? Something like "--boot=-5" or
"--boot=bd1b92058dd24e1eab573808e114f18b-5" or so?

> -               "  -b --this-boot         Show data only from current boot\n"
> -               "  -k --dmesg             Show kmsg log from current boot\n"
> +               "  -b --boot[=ID]         Show data only from ID or current boot if unspecified\n"
> +               "     --this-boot         Alias for --boot
> -               (deprecated)\n"

Here too our rule is to not document the deprecated option, but simply
support it silently in our code...

> -                        arg_this_boot = true;
> +                        if (optarg)
> +                            arg_boot_id_descriptor = optarg;
> +                        else if (optind < argc && argv[optind][0] != '-') {
> +                            arg_boot_id_descriptor = argv[optind];
> +                            optind++;
> +                        }
> +                        arg_boot_id = true;

This needs to follow our coding sytle (i.e. 8char indenting).

> +static int get_ancestor_boot_id(sd_journal *j, sd_id128_t *boot_id,
> int degree)

degree should probably be an "unsigned" rather than an "int". We try to
use types that indicate the sensible range of the variable, and a
negative value here doesn't appear to make sense, so please use "unsigned".

> +{
> +    int r;
> +    sd_id128_t last_id;
> +    bool boot_id_found = false;
> +    char match[9+32+1] = "_BOOT_ID=";
> +    _cleanup_free_ char *cursor = NULL;


Indenting/coding style...

Lennart

-- 
Lennart Poettering - Red Hat, Inc.