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

Support file descriptor store messages #8

Merged
merged 2 commits into from
Oct 5, 2024

Conversation

cbranch
Copy link
Contributor

@cbranch cbranch commented Feb 8, 2024

Support for systemd's file descriptor store is added using new NotifyState variants and a new notify_with_fds function, which passes file descriptors over the NOTIFY_SOCKET. The capability is gated behind a "fdstore" feature to avoid adding dependencies that were not previously required, and no existing API is touched except for the feature-gated NotifyState enum.

As stored file descriptors can be named, you should be able to retrieve file descriptors by those names. This is implemented via the new listen_fds_with_names function, and is also useful if you are using socket activation.

@cbranch cbranch force-pushed the cbranch/notify-with-fds2 branch from 32b9723 to 6acde97 Compare February 9, 2024 21:04
@cbranch
Copy link
Contributor Author

cbranch commented Feb 9, 2024

Missed a clippy lint, this should pass now.

@cbranch
Copy link
Contributor Author

cbranch commented Mar 7, 2024

Do you have any opinions on the design choices made here? In particular, the changes added to support passing file descriptors through the notify socket.

listen_fdnames: Option<String>,
) -> io::Result<impl ExactSizeIterator<Item = (RawFd, String)>> {
let listen_fdnames = if let Some(names) = listen_fdnames {
names.split(':').map(|x| x.to_owned()).collect::<Vec<_>>()
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not super familiar with this functionality, but shouldn't we parse foo::bar as ["foo", "unknown", "bar"] instead of ["foo", "", "bar"]?

Copy link
Owner

Choose a reason for hiding this comment

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

Gentle nudge @cbranch, can you take a look at this? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified the behaviour of the libsystemd function and it indeed uses an empty string, though systemd should always provide a non-empty name. I added a comment with a code example for posterity.


let _guard = Guard;
fn listen_fds_internal(unset_env: bool) -> io::Result<impl ExactSizeIterator<Item = RawFd>> {
Copy link
Owner

Choose a reason for hiding this comment

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

Wish I could remember why listen_fds doesn't take unset_env.

fdstore = ["dep:sendfd"]

[dependencies]
sendfd = { version = "0.4", optional = true }
Copy link
Owner

Choose a reason for hiding this comment

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

Nice, too bad it doesn't support SCM_CREDENTIALS for sd_pid_notify and friends.

Copy link
Owner

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

Thanks, and sorry for the late review, I guess this slipped my mind.

@lnicola lnicola mentioned this pull request May 5, 2024
src/lib.rs Outdated Show resolved Hide resolved
@cbranch cbranch force-pushed the cbranch/notify-with-fds2 branch from 6acde97 to 03fd22a Compare May 28, 2024 19:02
cbranch added 2 commits May 28, 2024 20:06
Adds the `notify_with_fds` function and the `NotifyState::FdStore`
variant, to support systemd's file descriptor store feature. These are
gated behind the 'fdstore' flag along with the dependency required to
make it work.
Names can be received for inherited sockets using
`listen_fds_with_names` function instead of `listen_fds`. If the file
descriptor store is used, the names can be set for stored fds using the
`NotifyState::FdName` variant. Named fds can be removed by additionally
supplying the `FdStoreRemove` variant.
@cbranch cbranch force-pushed the cbranch/notify-with-fds2 branch from 03fd22a to b80e6eb Compare May 28, 2024 19:06
@cbranch
Copy link
Contributor Author

cbranch commented Jun 10, 2024

For the benefit of notifs: outstanding issues should be resolved, please go ahead and merge/release as you see fit.
It would be good to have a listen_fds with unset_env flag though!

@cbranch
Copy link
Contributor Author

cbranch commented Oct 1, 2024

Bumping this change - this would be great to get in a minor semver bump if you are ready to create one :)

@lnicola lnicola merged commit 7c6a290 into lnicola:master Oct 5, 2024
@lnicola
Copy link
Owner

lnicola commented Oct 5, 2024

https://crates.io/crates/sd-notify/0.4.3 is now out, sorry again for my abysmal response times 😔.

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.

3 participants