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

Update daemonize crate #81

Open
ariel-miculas opened this issue Mar 15, 2023 · 5 comments
Open

Update daemonize crate #81

ariel-miculas opened this issue Mar 15, 2023 · 5 comments

Comments

@ariel-miculas
Copy link
Collaborator

There are some new commits in https://github.com/knsd/daemonize/commits/master that should be cherry picked into https://github.com/ariel-miculas/daemonize

@ariel-miculas
Copy link
Collaborator Author

The goal is to get rid of the custom fork if possible, maybe pin daemonize to the 0.4.0 version. The author of the library is not cooperating: knsd/daemonize#50

@polarathene
Copy link

polarathene commented Sep 12, 2024

Did the author disable the issues? The PR linked to #49 which I assume was an issue describing some context at some point?

@ariel-miculas
Copy link
Collaborator Author

Yes, @polarathene, you are right, the author disabled the issues. This is the commit that made use of the AsRawFd trait implementation in Daemonize. Since then I have removed the libfuse dependency and as a result I didn't need the output redirection anymore, i.e. my custom patches to the Daemonize crate. As you can see here, PuzzleFS now uses version "0.4.1" of Daemonize instead of the latest version because we make use of the exit_action function that was removed in later versions of Daemonize (no idea why, though).

@polarathene
Copy link

polarathene commented Sep 13, 2024

This turned out a bit longer than originally expected 😂

No expectation for a response, I don't have that much experience with this particular task itself so I could be misunderstanding the issue you were facing with the exit_action requirement.

EDIT: If you're familiar with the whole forking/daemonization process going on and the changes between 0.4.1 and 0.5.0 you can ignore the remainder of the comment. As per the 2nd edit below, I realize exit_action was for better communicating a failure before the parent exits and you lose stdout/stderr to the daemonization.


we make use of the exit_action function that was removed in later versions of Daemonize (no idea why, though).

The exit_code feature support was specifically for the first fork child (reference from 0.4.1 first fork in start()).

Whereas with start()in 0.5.0 it has:

Do you actually need exit_action()?

From the 0.4.1 source, we have:

  • privileged_action():

    Execute action just before dropping privileges.
    Most common usecase is to open listening socket.
    Result of action execution will be returned by start method.

  • exit_action():

    Execute action just before exiting the parent process.
    Most common usecase is to synchronize with forked processes.

Your usage doesn't seem relevant to exit_action()?:

puzzlefs/exe/src/main.rs

Lines 122 to 133 in ab7a74f

let daemonize = Daemonize::new().exit_action(move || {
let mut read_buffer = [0];
if let Err(e) = recv.read_exact(&mut read_buffer) {
info!("error reading from pipe {e}")
} else if read_buffer[0] == b'f' {
// in case of failure, 'f' is written into the pipe
// we explicitly exit with an error code, otherwise exit(0) is done by daemonize
exit(1);
}
});
match daemonize.start() {

Here you're using read_exact() to block on receiving a single byte before continuing. And that failure condition depends upon performing daemonize.start() to handle the returned result before returning for the error handling of the caller that would write f?:

puzzlefs/exe/src/main.rs

Lines 245 to 261 in ab7a74f

let (recv, mut init_notify) = os_pipe::pipe()?;
if let Err(e) = mount_background(
image,
&m.tag,
&mountpoint,
m.options,
manifest_verity,
recv,
&init_notify,
) {
if let Err(e) = init_notify.write_all(b"f") {
error!("puzzlefs will hang because we couldn't write to pipe, {e}");
}
error!("mount_background failed: {e}");
return Err(e);
}

Perhaps you're a bit more familiar with what's going on in puzzlefs there, but I don't quite follow why you want to run this logic on the parent process before it exits with 0 as it's meant to. Wouldn't that belong better in privileged_action()? (which provides a return value to start() that you match on to handle the result?)

I have seen the related commits, but I'm probably not grokking something 😅

  • Jan 2023 (introduce daemonize): 3bfb0ee
  • March 2023 (added the f logic): 1a0aa29

EDIT: I assume the intention is to avoid the parent process from exiting too soon, waiting until the child process has been successful after the daemonize.start() call to return the status of mount() / error to then notify the parent via the pipe that's delaying exit until a byte arrives?


Comparing 0.4.1 to 0.5.0 and general control flow

From what I understand perform_fork() (0.4.1 vs 0.5.0) calls libc::fork() which should return the child PID to the parent process if successful while the child process receives 0, but if the fork failed you'd get -1 and need to lookup the appropriate errno code (unrelated to the -1, it's a separate call).

In 0.4.1 the perform_fork() method was handling each of those parent / child cases. Your exit_action was for successful daemonization where the parent received a PID of the child, and as part of this process it's meant to exit the parent process with 0 status code. It looks like a failure scenario just returned a error for you to handle, but didn't handle this errno lookup for you to give more context.

In 0.5.0, a check_err() function was added, which instead performs the errno lookup for you and returns that as the error. Now the next two conditions are handling PID for child (0 == None) and parent (Some(pid)).

With 0.5.0 when you call start() it will not go directly to the perform_fork() call, but instead match on execute() where the parent will either be an error with errno, or None (successful) matching this block for the child and this block for the parent.

That's quite a bit to digest, but it's a bit less vague for control flow from parent to grand child?


Alternatives

Have you considered if that crate is still worthwhile for how you're leveraging it?

There's daemonize-me that's an alternative that referenced daemonize, it uses nix instead of libc calls directly, but hasn't been touched in 2 years. Still it appears to offer some hooks if you really need them (there are some closed issues and a 2.1 branch from a while ago intended to improve on the hook support, but it seems no progress is being made there).

nix::unistd::daemon(false, false) would roughly match the defaults of daemonize, but it's skips quite a bit of the daemonization process but you don't appear to need opt-in to any of that. For reference here is the daemon() libc code that nix would call (single fork() followed by setsid()).

// Return values if failing to fork described here:
// - https://man7.org/linux/man-pages/man3/daemon.3.html#RETURN_VALUE
//
// nix `daemon()` provides result with `errno`:
// - https://docs.rs/nix/latest/src/nix/unistd.rs.html#994-997
// - https://docs.rs/nix/latest/nix/errno/enum.Errno.html#method.result
let daemonized = nix::unistd::daemon(false, false);

@ariel-miculas
Copy link
Collaborator Author

The exit_action handler achieves two things.
First of all, as you said, it prevents the parent process from exiting too soon. After I run puzzlefs mount and it finishes its execution, I expect to be able to access the new mountpoint immediately. This is why I wait until one byte is written to the pipe. This is especially useful in scripts, e.g.:

mkdir /tmp/mnt
puzzlefs mount /tmp/puzzlefs tag /tmp/mnt
ls /tmp/mnt

Without the delay, the ls command would race with the mounting of the filesystem, so it could return an empty directory in some cases and the mounted filesystem in other cases.

Secondly, if PuzzleFS is not able to mount the filesystem, I expect the exit code to reflect this. Without the exit_action handler, the mount command would return 0 even when the mounting fails.

PS: the pipe mechanism is reused is this case because of this feature: https://github.com/project-machine/puzzlefs?tab=readme-ov-file#notification-when-the-mountpoint-is-ready

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

No branches or pull requests

2 participants