On Thu, Sep 22, 2022 at 01:31:49PM +0200, Dominik Csapak wrote:
> [snip]
> > > -/*
> > > - * SIGALRM and cleanup handling
> > > - *
> > > - * terminate_client will set an alarm for 5 seconds and add its client's
> > > PID to
> > > - * the forced_cleanups list - when the timer expires, we iterate the
> > > list and
> > > - * attempt to issue SIGKILL to all processes which haven't yet stopped.
> > > - */
> > > -
> > > -static void
> > > -alarm_handler(__attribute__((unused)) int signum)
> > > -{
> > > - alarm_triggered = 1;
> > > -}
> > > -
> >
> > wasn't this intentionally decoupled like this?
> >
> > alarm_handler just sets the flag
> > actual force cleanup is conditionalized on the alarm having triggered,
> > but the cleanup happens outside of the signal handler..
> >
> > is there a reason from switching away from these scheme? we don't need
> > to do the cleanup in the signal handler (timing is already plenty fuzzy
> > anyway ;))
>
> no real reason, i found the code somewhat cleaner, but you're right,
> we probably want to keep that, and just trigger it regularly
>From what I can tell the only point of this signal is to interrupt
`epoll()` after a while to call the cleanup/kill handler since we only
have a single worker here that needs to do some work after a timeout.
Why not either:
- set a bool instead of calling `alarm()` which causes the next
`epoll()` call to use a timeout and call the cleanups if epoll turns
up empty
- or create a timerfd (timerfd_create(2)) in the beginning which we
add to the epoll context and use `timerfd_settime(2)` in place of
`alarm()`, which will also wake up the epoll call without having to add
timeouts to it
`alarm()` is just such a gross interface...
In theory we'd also be able to ditch all of those `EINTR` loops as we
wouldn't be expecting any interrupts anymore... (and if we did expect
them, we could add a `signalfd(2)` to `epoll()` as well ;-)
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
