[systemd-devel] Fragile journal interleaving

[Lennart Poettering]

On Do, 14.12.17 03:56, Uoti Urpala (uoti.urpala at pp1.inet.fi) wrote:

> On Wed, 2017-12-13 at 18:40 +0100, Lennart Poettering wrote:
> > On Mi, 13.12.17 00:43, Uoti Urpala (uoti.urpala at pp1.inet.fi) wrote:
> > > correct position, so I don't see why the monotonicity discard would be
> > > needed for that case either?
> > 
> > I figure you are right.
> > 
> > Any chance you can prep a patch for this change?
> 
> A patch with a fairly long commit message explaining the rationale and
> related issues attached.

Could you submit through github please?

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

>          for (;;) {
>                  bool found;
>  
> -                if (j->current_location.type == LOCATION_DISCRETE) {
> -                        int k;
> -
> -                        k = compare_with_location(f, &j->current_location);
> -
> -                        found = direction == DIRECTION_DOWN ? k > 0 : k < 0;
> -                } else
> +                if (j->current_location.type == LOCATION_DISCRETE)
> +                        // only == 0 or not matters

Please use C comments, /* */, see CODING_STYLE.

> +                        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".

Thanks!

Lennart

-- 
Lennart Poettering, Red Hat