[systemd-devel] Fragile journal interleaving

[Uoti Urpala]

On Thu, 2017-12-14 at 10:35 +0100, Lennart Poettering wrote:
> On Do, 14.12.17 03:56, Uoti Urpala (uoti.urpala at pp1.inet.fi) wrote:
> > BTW current sd_journal_next() documentation claims "The journal is
> > strictly ordered by reception time, and hence advancing to the next
> > entry guarantees that the entry then pointing to is later in time than
> > then previous one, or has the same timestamp." Isn't this OBVIOUSLY
> > false when talking about realtime timestamps if you don't include
> > caveats about changing the machine time?
> 
> Well, but the philosophical concept of time beyond the implementation
> detail of Linux CLOCK_REALTIME is of course monotonic, hence I think
> it's actually correct what is written there. That said a patch that
> clarifies this would of course be very welcome...

Well if you talk about actual timestamp values then it's not true due
to  time changes. If you talk about the "physical reality" time when it
was logged, then it's true for a single journal file, but not true when
interleaving multiple journal files, as the interleaving process may
have to use the timestamps and they're not perfect (even ignoring the
issues discussed in this thread).


> > +                if (j->current_location.type == LOCATION_DISCRETE)
> > +                        // only == 0 or not matters
> 
> Please use C comments, /* */, see CODING_STYLE.

That is a standard C comment... IMO it's quite silly to insist on this
kind of "This is not the C I first learned! I refuse to update the way
I do things!" attitude for systemd. "//" comments and free placement of
variable declarations are normal C and no more questionable than any
other random feature. In practice, the ONLY reason anyone opposes them
is that they were added at a later point. This is exactly the same kind
of resistance to change that underlies most "sysvinit worked just fine
for me, systemd is horrible" attitudes. So systemd in particular really
should do better.


> > +                        found = compare_with_location(f,
> > &j->current_location);
> 
> this returns an int, and you assign it to a bool. If the "int" was
> actually used in a "boolean" way that would be OK, but it actually
> returns < 0, 0, > 0, hence we should make the conversion explicit here
> and write this as "found = c_w_l(…) != 0".

Well that int/bool distinction was basically why I added the comment
above. I guess adding a redundant comparison is an alternative, matter
of taste mostly...