Re: [pve-devel] [PATCH qemu-server 1/3] qmeventd: rework 'forced_cleanup' handling and set timeout to 60s

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 ;-)