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

Thu Jul 18 05:05:07 PDT 2013

On Thu, Jul 18, 2013 at 01:05:25PM +0200, Jan Janssen wrote:
> On 07/18/2013 06:10 AM, Zbigniew Jędrzejewski-Szmek wrote:
> >On Tue, Jul 16, 2013 at 05:46:04PM +0200, Lennart Poettering wrote:
> >>On Tue, 16.07.13 17:42, Zbigniew Jędrzejewski-Szmek (zbyszek at in.waw.pl) wrote:
> >>
> >>>
> >>>On Tue, Jul 16, 2013 at 05:39:54PM +0200, Lennart Poettering wrote:
> >>>>On Fri, 28.06.13 17:26, Jan Janssen (medhefgo at web.de) wrote:
> >>>>Applied this one now. If people start complaining about its speed we can
> >>>>reinvestigate and do find some way for optimization...
> >>>We need to think about negative matches. Looking for previous boots
> >>>with negative matches should work nicely.
> >>
> >>The bisection tables make this less efficient but certainly possible.
> >>
> >>>I'd like to complain about the : in the syntax though.
> >>
> >>Hmm, what would you propose? There's still time to fix it!
> >I went ahead, and removed : from the syntax. It feels OK in my testing.
> >
> >And I also made one optimization, which is important imho: 'journactl -b'
> >will still use the boot id from sd_id128_get_boot() to avoid searching
> >through the tables, and 'journalctl -b BOOT_ID[+-0]' will just
> >use  BOOT_ID without searching through the tables. This should help
> >a lot when running with cold cache.
> >
> >Zbyszek
> >
> 
> I really don't like arguments to options that can start with "-", it
> can easily be confused with another option. Especially if one were ever
> to introduce options like "-0" to "-9". Also, not accepting long UUIDs
> is kind of restricting the user. But ultimately, this is
> bike-shedding...
> 
> But more importantly, you've introduced a bug:
> 
> $ ./journalctl -b a709bdcbaa1b422f8338a25fd2d4d61d
> Relative boot ID offset must start with a '+' or a '-', found ''
Arrgh.

> Also, for the challenged people (me), does this really guard the
> array access (count >= INT_MAX comes to my mind)? And if so, how?
> 
> 	if (relative > (int) count || relative <= -(int)count)
count comes from searching throught the database, so cannot really
get too large without a really long wait. Nevertheless, if it was
>= INT_MAX, than after casting to (int) it would become negative,
and the left side of the if should be satisfied.

> If you could silence this warning, it would be nice :P
> 
> src/journal/journalctl.c: In function ‘get_relative_boot_id’:
> src/journal/journalctl.c:747:63: warning: comparison between signed
> and unsigned integer expressions [-Wsign-compare]
>                                      (id - all_ids) + relative >= count)
Interesting that my gcc (4.7.2) didn't show that.

> Anyway, gonna go sulk now for not having come up with such nice code
> in the first place :(
The pull towards bikeshedding and µ-opts was too strong for me to
resist :) But I seriously think that the arguments with a dash will
turn out OK in practice.

Zbyszek
-- 
they are not broken. they are refucktored
                           -- alxchk