Add monotonic_usec

[Firstyear]

Resolve #11 - add support for the monotonic-usec notification that is required from v253 and onwards for reload to succeed.

[mbuesch]

Are you sure that SystemTime uses clock_gettime + CLOCK_MONOTONIC internally?
The documentation of SystemTime says:

Distinct from the Instant type, this time measurement is not monotonic

and the implementation is not guaranteed:

Disclaimer: These system calls might change over time.

https://doc.rust-lang.org/std/time/struct.SystemTime.html

Does systemd compare the sent time stamp to CLOCK_MONOTONIC internally? That would not work, if we sent it a time stamp based off of a different clock.

I think sd-notify should call libc::clock_gettime(CLOCK_MONOTONIC) directly.

[lnicola]

Thanks for the PR. I agree, that should be Instant, but it doesn't expose the values, so clock_gettime is needed.

I'm not sure how I feel about us sending it automatically. At this time (heh), it's documented as:

This is typically used in combination with "RELOADING=1"

I don't see any other use case for it, but:

• if we send it and add the variant, the user might send it twice by mistake
• if we send it and don't add the variant, some future use might still require the variant
• if we don't send it, having to call libc::clock_gettime will be pretty annoying for the user
• we could expose a function that calls libc::clock_gettime

Overall, I was in favour of not sending it automatically, but it feels like we should just handle it on our side.

[Firstyear]

Seems we were not the only one bitten:

https://gitlab.isc.org/isc-projects/bind9/-/issues/4377

So I think if a reload message is sent you have to send a monotonic clock message too :(

[Firstyear]

I think sd-notify should call libc::clock_gettime(CLOCK_MONOTONIC) directly.

Yep, my mistake :)

[Firstyear]

Need to finish fixing the error handling now btw.

[Firstyear]

@lnicola With the call to monotonic_time_usecs since that returns io::Result, but in Display we need fmt::Result, do you think we make MonotonicUsec take the i128 and we expose the monotonic_usec fn?

Alternately we could do a constructor on NotifyState::monotonic_usec_now()

[lnicola]

Yeah, let's store it in there and add NotifyState::monotonic_usec_now().

[Firstyear]

Okay, should be good for another review :)

[lnicola]

I just pushed a commit to your branch to clean things up a little, can you take a look?

Also r? @mbuesch since you're here 😛.

[Firstyear]

@lnicola Yep, thank you! Looks good :)

[lnicola]

This didn't build (no_run is "don't run", not "don't build"), but should be fine to run.

[Firstyear]

Not sure what you mean?

cargo test
...
test src/lib.rs - NotifyState<'_>::monotonic_usec_now (line 116) ... ok

[lnicola]

I had to add the use, otherwise:

---- src/lib.rs - NotifyState<'_>::monotonic_usec_now (line 116) stdout ----
error[E0433]: failed to resolve: use of undeclared type `NotifyState`
 --> src/lib.rs:118:9
  |
4 | let _ = NotifyState::monotonic_usec_now().expect("Failed to read monotonic time");
  |         ^^^^^^^^^^^ use of undeclared type `NotifyState`
  |
help: consider importing this enum
  |
3 + use sd_notify::NotifyState;
  |

And we can remove no_run since it doesn't do anything bad.

Not sure how it passed CI, maybe it didn't run?

[mbuesch]

Looks good to me. Thanks to all of you involved :)

[Firstyear]

No problem, happy to have helped!

[lnicola]

I squashed and force-pushed, but tried to preserve the attribution, hope you don't mind. Thanks for the PR!

[lnicola]

Available at https://crates.io/crates/sd-notify/0.4.4.

[Firstyear]

Thank you so much!