[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libguestfs] [PATCH libnbd 1/2] lib: Avoid killing subprocess twice.



On Thu, Sep 26, 2019 at 01:42:19PM -0500, Eric Blake wrote:
> On 9/26/19 11:40 AM, Richard W.M. Jones wrote:
> >If the user calls nbd_kill_subprocess, we shouldn't kill the process
> >again when we close the handle (since the process has likely gone and
> >we might be killing a different process).
> >---
> >  lib/handle.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> >diff --git a/lib/handle.c b/lib/handle.c
> >index 2af25fe..5ad818e 100644
> >--- a/lib/handle.c
> >+++ b/lib/handle.c
> >@@ -315,6 +315,8 @@ nbd_unlocked_kill_subprocess (struct nbd_handle *h, int signum)
> >      return -1;
> >    }
> >+  h->pid = -1;
> >+
> 
> Ouch - this means we completely forget about the child process, even
> if signum == SIGHUP and was meant merely to get the server to reload
> state rather than to kill it (we've talked about making nbdkit have
> a way to reload configuration); and prevents a client from using the
> API a second time to send a more severe signal like SIGKILL if the
> first didn't have the desired effect.
> 
> You are right, however, that once this is called, we have to be more
> careful that any future interaction with the pid does not race with
> a scenario where our child has gone away, and some other process
> come into its place.  Is it viable to check /proc to see if the
> child process is the same one that we spawned, for example,
> /proc/CHILD/status looking to see if PPid: still points to us?

Yes, although that wouldn't work on FreeBSD.

The patch as I wrote it is wrong (as I think you imply above) because
it leaves a zombie child around, so a long-running libnbd-using
process will accumulate zombies.

Firstly my opinion is we shouldn't worry too much about reloading the
configuration, because it seems like it would be much more reliable to
create a new handle + command.

However sending signals of increasing severity does seem like
something a caller would want to do.

It seems rather hard to cope with all this within the existing API, so
possibly we need new API(s) here ...

Also pidfd's would help greatly if they were more widely available ......

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]