Re: [pve-devel] [PATCH qemu-server 2/3] qmeventd: cancel 'forced cleanup' when normal cleanup succeeds

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

On Thu, Sep 22, 2022 at 01:37:57PM +0200, Dominik Csapak wrote:
> On 9/22/22 12:14, Matthias Heiserer wrote:
> > On 21.09.2022 14:49, Dominik Csapak wrote:
> > > instead of always sending a SIGKILL to the target pid.
> > > It was not that much of a problem since the timeout previously was 5
> > > seconds and we used pifds where possible, thus the chance of killing the
> > > wrong process was rather slim.
> > > 
> > > Now we increased the timeout to 60s which makes the race a bit more likely
> > > (when not using pidfds), so remove it from the 'forced_cleanups' list when
> > > the normal cleanup succeeds.
> > > 
> > > Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> > > ---
> > >   qmeventd/qmeventd.c | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/qmeventd/qmeventd.c b/qmeventd/qmeventd.c
> > > index e9ff5b3..de5efd0 100644
> > > --- a/qmeventd/qmeventd.c
> > > +++ b/qmeventd/qmeventd.c
> > > @@ -415,6 +415,25 @@ cleanup_qemu_client(struct Client *client)
> > >       }
> > >   }
> > > +static void
> > > +remove_cleanup_data(void *ptr, void *client_ptr) {
> > Not that it really matters, but is there a reason we don't use
> > remove_cleanup_data(struct CleanupData *ptr, struct Client *client_ptr)
> > and let the caller deal with types?
> > > +    struct CleanupData *data = (struct CleanupData *)ptr;
> > > +    struct Client *client = (struct Client *)client_ptr;
> > > +
> > > +    if (data->pid == client->pid) {
> > > +    forced_cleanups = g_slist_remove(forced_cleanups, ptr);
> > > +    free(ptr);
> > > +    }
> > > +}
> > > + > +static void
> > > +remove_from_forced_cleanup(struct Client *client) {
> > > +    if (g_slist_length(forced_cleanups) > 0) {
> > > +    VERBOSE_PRINT("removing %s from forced cleanups\n", client->qemu.vmid);
> > > +    g_slist_foreach(forced_cleanups, remove_cleanup_data, client);
> > that is, here `(void (*)(void*, void*)) remove_cleanup_data`. Seems a bit cleaner to me.
> > > +    }
> > > +}
> > > +
> > >   void
> > >   cleanup_client(struct Client *client)
> > >   {
> > > @@ -441,6 +460,7 @@ cleanup_client(struct Client *client)
> > >           break;
> > >       }
> > > +    remove_from_forced_cleanup(client);
> > >       free(client);
> > >   }
> > 
> 
> i just kept the style we use for the existing call to *_foreach.
> 
> my guess is that the intention was to keep the function close to what glib defines
> (although that uses 'gpointer'). doing as you suggested introduces a big
> cast that is confusing to read IMHO (for people not that familiar with c at least ;) )
> that could be solved with casting to 'GFunc' (not sure if that's considered good style?)
> but in the end, i don't have strong feeling either way

Just to follow this up: The main argument I can give for the current
style is that a cast works for any function signature and therefor
removes one possible compile-time check.
Sure, you can mess up the parameter cast in the function body, but
that's arguably less likely.

Also, since they're usually `void*` you wouldn't actually *need* to
repeat the type in the function body:

    -struct CleanupData *data = (struct CleanupData *)ptr;
    +struct CleanupData *data = ptr;

is actually sufficient in C.