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

[Jan Janssen]

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 ''

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)

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)

Anyway, gonna go sulk now for not having come up with such nice code
in the first place :(

Jan