-
Notifications
You must be signed in to change notification settings - Fork 664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tracing-journald: Send large journal payloads through memfd #1744
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
//! memfd helpers. | ||
|
||
use libc::*; | ||
use std::fs::File; | ||
use std::io::Error; | ||
use std::io::Result; | ||
use std::os::raw::c_uint; | ||
use std::os::unix::prelude::{FromRawFd, RawFd}; | ||
|
||
fn create(flags: c_uint) -> Result<File> { | ||
let fd = unsafe { memfd_create("tracing-journald\0".as_ptr() as *const c_char, flags) }; | ||
if fd < 0 { | ||
Err(Error::last_os_error()) | ||
} else { | ||
Ok(unsafe { File::from_raw_fd(fd as RawFd) }) | ||
} | ||
} | ||
|
||
pub fn create_sealable() -> Result<File> { | ||
create(MFD_ALLOW_SEALING | MFD_CLOEXEC) | ||
} | ||
|
||
pub fn seal_fully(fd: RawFd) -> Result<()> { | ||
let all_seals = F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_WRITE | F_SEAL_SEAL; | ||
let result = unsafe { fcntl(fd, F_ADD_SEALS, all_seals) }; | ||
if result < 0 { | ||
Err(Error::last_os_error()) | ||
} else { | ||
Ok(()) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
//! socket helpers. | ||
|
||
use std::io::{Error, Result}; | ||
use std::mem::{size_of, zeroed}; | ||
use std::os::unix::net::UnixDatagram; | ||
use std::os::unix::prelude::{AsRawFd, RawFd}; | ||
use std::ptr; | ||
|
||
use libc::*; | ||
|
||
const CMSG_BUFSIZE: usize = 64; | ||
|
||
#[repr(C)] | ||
union AlignedBuffer<T: Copy + Clone> { | ||
buffer: T, | ||
align: cmsghdr, | ||
} | ||
|
||
fn assert_cmsg_bufsize() { | ||
let space_one_fd = unsafe { CMSG_SPACE(size_of::<RawFd>() as u32) }; | ||
assert!( | ||
space_one_fd <= CMSG_BUFSIZE as u32, | ||
"cmsghdr buffer too small (< {}) to hold a single fd", | ||
space_one_fd | ||
); | ||
} | ||
|
||
#[cfg(test)] | ||
#[test] | ||
fn cmsg_buffer_size_for_one_fd() { | ||
assert_cmsg_bufsize() | ||
} | ||
|
||
pub fn send_one_fd(socket: &UnixDatagram, fd: RawFd) -> Result<usize> { | ||
assert_cmsg_bufsize(); | ||
|
||
let mut cmsg_buffer = AlignedBuffer { | ||
buffer: ([0u8; CMSG_BUFSIZE]), | ||
}; | ||
let mut msg: msghdr = unsafe { zeroed() }; | ||
|
||
// We send no data body with this message. | ||
msg.msg_iov = ptr::null_mut(); | ||
msg.msg_iovlen = 0; | ||
|
||
msg.msg_control = unsafe { cmsg_buffer.buffer.as_mut_ptr() as _ }; | ||
msg.msg_controllen = unsafe { CMSG_SPACE(size_of::<RawFd>() as _) as _ }; | ||
|
||
let mut cmsg: &mut cmsghdr = | ||
unsafe { CMSG_FIRSTHDR(&msg).as_mut() }.expect("Control message buffer exhausted"); | ||
|
||
cmsg.cmsg_level = SOL_SOCKET; | ||
cmsg.cmsg_type = SCM_RIGHTS; | ||
cmsg.cmsg_len = unsafe { CMSG_LEN(size_of::<RawFd>() as _) as _ }; | ||
|
||
unsafe { ptr::write(CMSG_DATA(cmsg) as *mut RawFd, fd) }; | ||
|
||
let result = unsafe { sendmsg(socket.as_raw_fd(), &msg, libc::MSG_NOSIGNAL) }; | ||
|
||
if result < 0 { | ||
Err(Error::last_os_error()) | ||
} else { | ||
// sendmsg returns the number of bytes written | ||
Ok(result as usize) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing it with
NotFound
would be fine. We shouldn't panic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.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...)
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).> /dev/null
($null
pwsh) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make another merge request which uses
Other
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, @CAD97 already opened one, thanks a lot 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.