Re: [systemd-devel] [PATCH v5] journalctl: Add support for showing messages from a previous boot
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 (zbys...@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 (medhe...@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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v5] journalctl: Add support for showing messages from a previous boot
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 (zbys...@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 (medhe...@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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v5] journalctl: Add support for showing messages from a previous boot
On Tue, Jul 16, 2013 at 05:46:04PM +0200, Lennart Poettering wrote: On Tue, 16.07.13 17:42, Zbigniew Jędrzejewski-Szmek (zbys...@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 (medhe...@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 -- they are not broken. they are refucktored -- alxchk ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v5] journalctl: Add support for showing messages from a previous boot
On Fri, 28.06.13 17:26, Jan Janssen (medhe...@web.de) wrote: Unfortunately, order of sd_journal_enumerate_unique() is undefined, so we have to sort the boot IDs. For that we seach for any log entry with a specific boot ID and then use the realtime stamp to order everything. --- Hi, I redid the boot ID look up to use enumerate_unique. This is quite fast if the cache is warm but painfully slow if it isn't. It has a slight chance of returning the wrong order if realtime clock jumps around. This one has to do n searches for every boot ID there is plus a sort, so it depends heavily on cache hotness. This is in contrast to the other way of look-up through filtering by a MESSAGE_ID, which only needs about 1 seek + whatever amount of relative IDs you want to walk. I also have a linked-list + (in-place) mergesort version of this patch, which has pretty much the same runtime. But since this one is using libc sorting and armortized allocation, I prefer this one. To summarize: The MESSAGE_ID way is a *lot* faster but can be incomplete due to rotation, while the enumerate+sort will find every boot ID out there but will be painfully slow for large journals and cold caches. You choose :P Applied this one now. If people start complaining about its speed we can reinvestigate and do find some way for optimization... Thanks, Lennart -- Lennart Poettering - Red Hat, Inc. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH v5] journalctl: Add support for showing messages from a previous boot
On Tue, Jul 16, 2013 at 05:39:54PM +0200, Lennart Poettering wrote: On Fri, 28.06.13 17:26, Jan Janssen (medhe...@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. I'd like to complain about the : in the syntax though. Zbyszek -- they are not broken. they are refucktored -- alxchk ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
