Skip to content
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

Make memfd_create syscall directly #1912

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

9999years
Copy link
Contributor

@9999years 9999years commented Feb 7, 2022

Fixes #1879

Motivation

journald-tracing>=0.2.1 doesn't build with old glibc.

Solution

Make the memfd_create syscall ourselves.

Open questions

  • What happens when memfd_create isn't supported? Do large payloads just get trashed?

cc @lunaryorn @Ralith

@9999years 9999years requested review from davidbarsky, hawkw and a team as code owners February 7, 2022 16:18
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me!

AFAIK, the memfd_create function just makes the syscall exactly the way this code does (or at least, the musl version does; the glibc source tree is harder to navigate so i gave up), so i don't think there's anything we need to worry about by doing it ourselves. looks good to me!

tracing-journald/src/memfd.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Feb 7, 2022

Open questions

  • What happens when memfd_create isn't supported? Do large payloads just get trashed?

also, i believe the relevant syscall has existed in Linux since approximately forever ago, so we probably don't need to worry about it not being available.

@hawkw
Copy link
Member

hawkw commented Feb 7, 2022

CI failure is due to a dependency breaking MSRV and is unrelated, I'll resolve that separately --- you don't need to worry about it.

@hawkw hawkw requested a review from Ralith February 7, 2022 17:20
@hawkw
Copy link
Member

hawkw commented Feb 7, 2022

updated with the latest master, which should fix the MSRV checks

Copy link
Contributor

@swsnr swsnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a small nitpick, but otherwise 👍

As for the question, yes, if the syscall doesn't exist the call will fail with ENOSYS and the message is lost.

I'd just take this risk; in the ticket the OP said it exists since some 3.x version, and I don't think anyone still uses kernels that old 🙂

But if you'd like to make it safe I guess we could take a look at the systemd code from before the call was added to glibc to see what it used instead back then, and use that as a fallback on ENOSYS.

But personally I'd just document it and bother no more 🙂

tracing-journald/src/memfd.rs Show resolved Hide resolved
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@hawkw hawkw merged commit 3ed1575 into tokio-rs:master Feb 7, 2022
@9999years 9999years deleted the journald/fix-build-old-glibc branch February 7, 2022 20:26
hawkw pushed a commit that referenced this pull request Feb 7, 2022
Fixes #1879

## Motivation

`journald-tracing>=0.2.1` doesn't build with old glibc.

## Solution

Make the `memfd_create` syscall ourselves.

cc @lunaryorn @Ralith
@hawkw hawkw mentioned this pull request Feb 7, 2022
hawkw pushed a commit that referenced this pull request Feb 7, 2022
Fixes #1879

## Motivation

`journald-tracing>=0.2.1` doesn't build with old glibc.

## Solution

Make the `memfd_create` syscall ourselves.

cc @lunaryorn @Ralith
hawkw added a commit that referenced this pull request Feb 8, 2022
# 0.2.3 (February 7, 2022)

### Fixed

- Fixed missing `memfd_create` with `glibc` versions < 2.25 ([#1912])

### Changed

- Updated minimum supported Rust version to 1.49.0 ([#1913])

Thanks to @9999years for contributing to this release!

[#1912]: #1912
[#1913]: #1913
hawkw added a commit that referenced this pull request Feb 8, 2022
# 0.2.3 (February 7, 2022)

### Fixed

- Fixed missing `memfd_create` with `glibc` versions < 2.25 ([#1912])

### Changed

- Updated minimum supported Rust version to 1.49.0 ([#1913])

Thanks to @9999years for contributing to this release!

[#1912]: #1912
[#1913]: #1913
/// RHEL 7, etc.
///
/// See: https://github.com/tokio-rs/tracing/issues/1879
fn memfd_create_syscall(flags: c_uint) -> i64 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broken in 32-bit environments where c_long is, of course, 32 bit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good catch, that should return a c_long

kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.2.3 (February 7, 2022)

### Fixed

- Fixed missing `memfd_create` with `glibc` versions < 2.25 ([tokio-rs#1912])

### Changed

- Updated minimum supported Rust version to 1.49.0 ([tokio-rs#1913])

Thanks to @9999years for contributing to this release!

[tokio-rs#1912]: tokio-rs#1912
[tokio-rs#1913]: tokio-rs#1913
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracing-journald fix for #1698 fails to build on glibc < 2.27
5 participants