lucab / libsystemd-rs

A pure-Rust client library to work with systemd
https://crates.io/crates/libsystemd
Other
106 stars 19 forks source link

logging: directly use `memfd_crate` syscall #144

Closed daniestevez closed 1 year ago

daniestevez commented 1 year ago

memfd is only available starting with Linux 3.17 and glibc 2.27. In old systems, this may not be available.

To support usage in these old systems, this adds a feature flag "memfd" that makes using "memfd" optional. The feature flag is included in the default features, so this does not change the default behaviour.

When the crate is built without the memfd feature flag, the journal_send function will return an error if the message does not fit in a UNIX socket, instead of trying to use memfd to send the message.


As a rationale for this, I'm running on an old ARMv7 system that doesn't have memfd. In fact, I'm cross-compiling with cross, which is a popular way of cross-compiling Rust crates. When building for armv7-unknown-linux-gnueabihf, the current stable version of cross links against glibc 2.23, so this PR improves the portability of this crate when being built for ARM.

swsnr commented 1 year ago

glibc 2.23 is seven years old, kernel 3.17 almost ten. I don't think I'd like to support anything that old.

We could perhaps use a raw syscall for memfd_create. That's what tracing-journald does, probably for the same reason. But I don't think we should still account for kernels that do not include this syscall.

My opinion, anyways. It's @lucab's call.

daniestevez commented 1 year ago

Let's decide if we want to support this, and if so I'll do the suggested code changes and document the feature flag.

We're actually using kernel 3.17 in that system, so I guess that using a raw syscall would work. However, I don't think it pays off to invest significant effort in doing that. I'm using this to log to journald using systemd-journal-logger crate, and the modified version without memfd works just fine (the size of the log messages is small enough that the fit in the UNIX socket). This seems a small change that doesn't clutter the code base, and by guarding it after a feature flag that is enabled by default it cannot hurt anyone but those using it because they need it (in fact the intention is that only people that need this because the crate won't build against their libc should disable the feature flag).

I'm aware that this system is very old, but all I can say is that that's outside of my control and that having ancient kernels and libraries is not so uncommon in embedded systems.

lucab commented 1 year ago

I'm not a fan of ancient systems, but your glibc version is somehow still within Rust version requirements so I think it's fair to at least try to support it. We won't be build-testing ancient environments in CI (because they are a pain to maintain), so this will be somehow best-effort.

To that extent, I'd prefer avoiding a new feature. Using the raw syscall may look ugly but at least it doesn't alter the code flow.

But I don't think we should still account for kernels that do not include this syscall.

This should already degrade gracefully at runtime (by error-ing out).

daniestevez commented 1 year ago

I have force-pushed another attempt at this, this time using a syscall.

I looked at how nix implements memfd_create(), saw that it decides at compile time whether to call the libc memfd_create() or to perform a syscall, and copied over the code that does the syscall.

I'm not so happy with this solution, because I now feel that the place to fix this problem would be nix instead of libsystemd-rs. What I'd prefer is, when I'm building for this old ARM system, for nix to choose the syscall version at compile time. However, it doesn't seem possible to check or specify the glibc version to use it in a #[cfg()]. Still, this solution works for my purposes.

This is somewhat related to https://github.com/nix-rust/nix/issues/1972, but there there are two problems, because the nix memfd_create function is not being called, but somehow the linker ends up wanting to link this function.

daniestevez commented 1 year ago

Force-pushed to fix the pipeline in older rustc versions (they have RawFd somewhere else).

swsnr commented 1 year ago

I'm not so happy with this solution, because I now feel that the place to fix this problem would be nix instead of libsystemd-rs.

Perhaps, but then again nix didn't wrap a safe API around memfd anyways. We had to use their unsafe "sys" code which is a wrapper around libc which is a wrapper around the syscall. So if we have to be unsafe, then we can just as well cut out all the middlemen and do the syscall ourselves 🤷🏿