tracing-journald: Send large journal payloads through memfd #1744
Conversation
|
/cc @Ralith |
Linux limits the maximum amount of data permitted for a single Unix datagram; if our payload exceeds this platform-dependent size we write the content to a memfd and pass this memfd to journald, per https://systemd.io/JOURNAL_NATIVE_PROTOCOL/. Add a test which checks a large message. Closes #1698
|
Thanks for the contribution! |
See #1698: Properly write large payloads to journal. I'd appreciate a very careful review; this cmsg stuff is nasty, and while it's well documented in `cmsg(3)` I had to fiddle a bit because the corresponding functions in libc aren't const and thus don't permit a direct allocation of the buffer as most `cmsg` C code around does. Closes #1698 ## Motivation Linux limits the maximum amount of data permitted for a single Unix datagram; sending large payloads directly will fail. ## Solution Follow systemd.io/JOURNAL_NATIVE_PROTOCOL/ and check for `EMSGSIZE` from `send()`; in this case write the payload to a memfd, seal it, and pass it on to journald via a corresponding SCM_RIGHTS control message. Per discussion in #1698 this adds no dependency on `nix`, and instead implements fd forwarding directly with some bits of unsafe `libc` code.
See #1698: Properly write large payloads to journal. I'd appreciate a very careful review; this cmsg stuff is nasty, and while it's well documented in `cmsg(3)` I had to fiddle a bit because the corresponding functions in libc aren't const and thus don't permit a direct allocation of the buffer as most `cmsg` C code around does. Closes #1698 ## Motivation Linux limits the maximum amount of data permitted for a single Unix datagram; sending large payloads directly will fail. ## Solution Follow systemd.io/JOURNAL_NATIVE_PROTOCOL/ and check for `EMSGSIZE` from `send()`; in this case write the payload to a memfd, seal it, and pass it on to journald via a corresponding SCM_RIGHTS control message. Per discussion in #1698 this adds no dependency on `nix`, and instead implements fd forwarding directly with some bits of unsafe `libc` code.
See #1698: Properly write large payloads to journal. I'd appreciate a very careful review; this cmsg stuff is nasty, and while it's well documented in `cmsg(3)` I had to fiddle a bit because the corresponding functions in libc aren't const and thus don't permit a direct allocation of the buffer as most `cmsg` C code around does. Closes #1698 ## Motivation Linux limits the maximum amount of data permitted for a single Unix datagram; sending large payloads directly will fail. ## Solution Follow systemd.io/JOURNAL_NATIVE_PROTOCOL/ and check for `EMSGSIZE` from `send()`; in this case write the payload to a memfd, seal it, and pass it on to journald via a corresponding SCM_RIGHTS control message. Per discussion in #1698 this adds no dependency on `nix`, and instead implements fd forwarding directly with some bits of unsafe `libc` code.
See #1698: Properly write large payloads to journal. I'd appreciate a very careful review; this cmsg stuff is nasty, and while it's well documented in `cmsg(3)` I had to fiddle a bit because the corresponding functions in libc aren't const and thus don't permit a direct allocation of the buffer as most `cmsg` C code around does. Closes #1698 ## Motivation Linux limits the maximum amount of data permitted for a single Unix datagram; sending large payloads directly will fail. ## Solution Follow systemd.io/JOURNAL_NATIVE_PROTOCOL/ and check for `EMSGSIZE` from `send()`; in this case write the payload to a memfd, seal it, and pass it on to journald via a corresponding SCM_RIGHTS control message. Per discussion in #1698 this adds no dependency on `nix`, and instead implements fd forwarding directly with some bits of unsafe `libc` code.
See #1698: Properly write large payloads to journal. I'd appreciate a very careful review; this cmsg stuff is nasty, and while it's well documented in `cmsg(3)` I had to fiddle a bit because the corresponding functions in libc aren't const and thus don't permit a direct allocation of the buffer as most `cmsg` C code around does. Closes #1698 ## Motivation Linux limits the maximum amount of data permitted for a single Unix datagram; sending large payloads directly will fail. ## Solution Follow systemd.io/JOURNAL_NATIVE_PROTOCOL/ and check for `EMSGSIZE` from `send()`; in this case write the payload to a memfd, seal it, and pass it on to journald via a corresponding SCM_RIGHTS control message. Per discussion in #1698 this adds no dependency on `nix`, and instead implements fd forwarding directly with some bits of unsafe `libc` code.
# 0.2.1 (December 29, 2021) This release improves how `tracing-journald` communicates with `journald`, including the handling of large payloads. ### Added - Use an unconnected socket, so that logging can resume after a `journald` restart ([#1758]) ### Fixed - Fixed string values being written using `fmt::Debug` ([#1714]) - Fixed `EMSGSIZE` when log entries exceed a certain size ([#1744]) A huge thank-you to new contributor @lunaryorn, for contributing all of the changes in this release! [#1714]: #1714 [#1744]: #1744 [#1758]: #1758
# 0.2.1 (December 29, 2021) This release improves how `tracing-journald` communicates with `journald`, including the handling of large payloads. ### Added - Use an unconnected socket, so that logging can resume after a `journald` restart ([#1758]) ### Fixed - Fixed string values being written using `fmt::Debug` ([#1714]) - Fixed `EMSGSIZE` when log entries exceed a certain size ([#1744]) A huge thank-you to new contributor @lunaryorn, for contributing all of the changes in this release! [#1714]: #1714 [#1744]: #1744 [#1758]: #1758
| #[cfg(not(unix))] | ||
| fn send_payload(&self, _opayload: &[u8]) -> io::Result<()> { | ||
| Err(io::Error::new( | ||
| io::ErrorKind::Unsupported, |
There was a problem hiding this comment.
MSRV is currently set to 1.49 but io::ErrorKind::Unsupported was stabilized in 1.53.
This wasn't caught on CI because it's gated behind #[cfg(not(unix))] 🙃
There was a problem hiding this comment.
ughhh, that's annoying. currently we don't build MSRV on windows (or macOS) to avoid having a really huge number of CI builds... but maybe we should be.
There was a problem hiding this comment.
Is this really much of an issue? Does anyone actually use tracing-journald on Windows? I only added this because tracing-journald gets built on Windows on the pipeline; it's only there to silence Github actions, I never assumed anyone would actually build this on Windows.
There was a problem hiding this comment.
In other words as far as I'm concerned we could also just use unimplemented!() here and have it panic.
There was a problem hiding this comment.
Replacing it with NotFound would be fine. We shouldn't panic.
There was a problem hiding this comment.
And by the way, did anyone actually hit this MSRV violation in their build?
I "hit" it, but only inasmuch as I was doing a minver check on the repository from windows.
For #2231 I just replaced this with ErrorKind::Other.
MSRV tests on other OSes
I think in general it's fine to assume the Linux build will catch things, as the OS-gated functionality is rare enough, and we can fix violations when they come up.
If we do want to check other targets, it should be via cross-compile where possible since linux hosted runner time is cheaper, and just a check is sufficient.
(Ultimately CI-wise this comes back to the downside of a monorepo being testing the entire monorepo to verify a subpackage-local change... though this is sometimes desirable since e.g. tracing-core gets a significant amount of test coverage only from testing of tracing or even tracing-subscriber...)
don't compile on windows
There are AIUI four reasonable choices (of which only the latter pair are legally semver compatible):
#![cfg]the entire crate to only expose any API on platforms where it makes sense. This is IIRC the approach taken by winapi. Consumers#[cfg]their usage.compile_error!on platforms where the crate doesn't make any sense. Consumers need to use a target-dependent dependency as well as#[cfg]their usage.panic!(or error) entrypoints into the API on unsupported platforms. Consumers#[cfg]their usage (or tolerate an error on supported platforms as well).- Stub functionality on unsupported platforms. IIUC tracing-journald is purely a sink, so stubbing it as
> /dev/null($nullpwsh) isn't wrong, just perhaps unexpected. Consumers can unconditionally use the API and just implicitly degrade on unsupported platforms.
There was a problem hiding this comment.
I'll make another merge request which uses Other.
There was a problem hiding this comment.
Oh, @CAD97 already opened one, thanks a lot 🙏
There was a problem hiding this comment.
@hawkw I doubt that anyone's building and using tracing-journald on Windows; that wouldn't make any sense.
But I'm fine with using ErrorKind::Other.
There was a problem hiding this comment.
I think NotFound would be wrong, semantically; after all, we're not searching for anything here
Semantically, we are searching for JOURNALD_PATH. This is the error that will be returned when running on a Unix without journald, and unconditionally when running on Windows.
|
Indeed, didn't occur to me to look at it this way 🤔 I just thought about directly indicating it as unsupported.
Should I make an MR to change it for the sake of consistency? Or is it irrelevant because this is dead code anyway?
Am 20. Juli 2022 18:52:51 UTC schrieb Benjamin Saunders ***@***.***>:
***@***.*** commented on this pull request.
…
> @@ -109,6 +114,48 @@ impl Subscriber {
self.field_prefix = x;
self
}
+
+ #[cfg(not(unix))]
+ fn send_payload(&self, _opayload: &[u8]) -> io::Result<()> {
+ Err(io::Error::new(
+ io::ErrorKind::Unsupported,
> I think NotFound would be wrong, semantically; after all, we're not searching for anything here
Semantically, we are searching for `JOURNALD_PATH`. This is the error that will be returned when running on a Unix without journald, and unconditionally when running on Windows.
--
Reply to this email directly or view it on GitHub:
#1744 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
|
Probably not worth the trouble since it's dead, yeah. If someone's feeling motivated to make this code extra pretty, we'd probably be better off just reducing the amount of dead code that gets compiled outright. |
See #1698: Properly write large payloads to journal.
I'd appreciate a very careful review; this cmsg stuff is nasty, and while it's well documented in
cmsg(3)I had to fiddle a bit because the corresponding functions in libc aren't const and thus don't permit a direct allocation of the buffer as mostcmsgC code around does.Closes #1698
Motivation
Linux limits the maximum amount of data permitted for a single Unix datagram; sending large payloads directly will fail.
Solution
Follow systemd.io/JOURNAL_NATIVE_PROTOCOL/ and check for EMSGSIZE from
send(); in this case write the payload to a memfd, seal it, and pass it on to journald via a corresponding SCM_RIGHTS control message.Per discussion in #1698 this adds no dependency on nix, and instead implements fd forwarding directly with some bits of unsafe libc code.