Re: [pbs-devel] [PATCH proxmox] daemon: clean up middle process of double fork

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 29, 2024 4:37 pm, Dominik Csapak wrote:
> so we don't leave around a zombie process when the old daemon still
> needs to run, because of e.g. a running task.
> 
> Since this is mostly a cosmetic issue though, only try a clean up
> once, so we don't unnecessarily block or run into other issues here.
> (It could happen that it didn't exit at that point, but it's very
> unlikely.)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> maybe the comment could be improved, but i tried to be not overly
> verbose there, since it's not really an issue anyway
> 
>  proxmox-daemon/src/server.rs | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-daemon/src/server.rs b/proxmox-daemon/src/server.rs
> index efea9078..edc64795 100644
> --- a/proxmox-daemon/src/server.rs
> +++ b/proxmox-daemon/src/server.rs
> @@ -165,10 +165,12 @@ impl Reloader {
>                  // No matter how we managed to get here, this is the time where we bail out quickly:
>                  unsafe { libc::_exit(-1) }
>              }
> -            Ok(ForkResult::Parent { child }) => {
> +            Ok(ForkResult::Parent {
> +                child: middle_child,
> +            }) => {
>                  log::debug!(
>                      "forked off a new server (first pid: {}), waiting for 2nd pid",
> -                    child
> +                    middle_child
>                  );
>                  std::mem::drop(pnew);
>                  let mut pold = std::fs::File::from(pold);
> @@ -211,6 +213,13 @@ impl Reloader {
>                      log::error!("child vanished during reload: {}", e);
>                  }
>  
> +                // try exactly once to get rid of the zombie process of middle_child, but
> +                // non blocking and without error handling, since it's just cosmetic
> +                let _ = nix::sys::wait::waitpid(
> +                    middle_child,
> +                    Some(nix::sys::wait::WaitPidFlag::WNOHANG),
> +                );

looking at the possible errors here:

       EAGAIN The PID file descriptor specified in id is nonblocking and
       the process that it refers to has not terminated.

not using pidfds here, not applicable

       ECHILD (for wait()) The calling process does not have any
       unwaited-for children.

we are not calling wait, but waitpid, not applicable

       ECHILD (for waitpid() or waitid()) The process specified by pid
       (waitpid()) or idtype and id (waitid()) does not exist or is not
       a child of the calling process.  (This can  happen for one's own
       child if the action for SIGCHLD is set to SIG_IGN.  See also the
       Linux Notes section about threads.)

this one would mean the code above is buggy, so logging the error would
make sense?

       EINTR  WNOHANG was not set and an unblocked signal or a SIGCHLD
       was caught; see signal(7).

we set WNOHANG, so not applicable

       EINVAL The options argument was invalid.

this would also mean we do something wrong and we should log the error

       ESRCH  (for wait() or waitpid()) pid is equal to INT_MIN.

shouldn't happen either

so I think logging the error here (which should never happen ;)) should
be fine?

other than that, consider this:

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> +
>                  Ok(())
>              }
>              Err(e) => {
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel