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.csa...@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.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
