[PATCH] device: avoid signals sent to the mbim-proxy process

[Dan Williams]

On Mon, 2017-08-07 at 11:58 -0700, Ben Chan wrote:
> 
> 
> On Mon, Aug 7, 2017 at 8:58 AM, Dan Williams <dcbw at redhat.com> wrote:
> > On Sun, 2017-08-06 at 13:13 +0200, Aleksander Morgado wrote:
> > > If e.g. mbim-proxy is started by ModemManager and we send a
> > Ctrl+C to
> > > it, the signal would be propagated to the mbim-proxy process and
> > we
> > > would kill it right away, while leaving ModemManager still around
> > > wondering why the socket to the proxy got a HUP.
> > >
> > > Avoid this, by making sure the mbim-proxy gets its own process
> > group.
> > 
> 
> lgtm
>  
> > The way you've done it here is AFAIK the standard way to detach a
> > child
> > process and make it standalone.  We've done that for years in most
> > of
> > the things NetworkManager spawns, for example (VPN daemons, DHCP
> > daemon, avahi-autoipd, dnsmasq, teamd, etc).
> > 
> > Though instead of (0,0) NM uses:
> > 
> >         pid_t pid;
> > 
> >         pid = getpid ();
> >         setpgid (pid, pid);
> > 
> > Which I think is a bit more idiomatic.
> 
> Isn't that equivalent to `setpgid (0, 0)`? Is `setgpid (pid, pid)`
> preferred for some reason?

It should get called after the fork(), so it'll get the child's pid. 
Maybe you read getuid() instead (which since MM is root *would* be 0?)

Dan

> > Dan
> > 
> > > ---
> > >
> > > Hey,
> > >
> > > Eric, adding you in CC because I got this issue while testing
> > your
> > > ModemManager patch to act on proxy crashes. I was running MM in
> > --
> > > debug mode and sent a Ctrl-C to stop it, and that actually ended
> > up
> > > killing the proxy underneath and your reprobe logic hitting in...
> > :)
> > >
> > > What do you guys think? I believe this is reasonable, the proxy
> > > should really be a complete independent spawn not tied to the
> > process
> > > which originated it.
> > >
> > > The same change would be applicable to qmi-proxy.
> > >
> > > ---
> > >  src/libmbim-glib/mbim-device.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libmbim-glib/mbim-device.c b/src/libmbim-
> > glib/mbim-
> > > device.c
> > > index 68544af..4b7d431 100644
> > > --- a/src/libmbim-glib/mbim-device.c
> > > +++ b/src/libmbim-glib/mbim-device.c
> > > @@ -1077,6 +1077,13 @@ wait_for_proxy_cb (GTask *task)
> > >  }
> > >
> > >  static void
> > > +spawn_child_setup (void)
> > > +{
> > > +    if (setpgid (0, 0) < 0)
> > > +        g_warning ("couldn't setup proxy specific process
> > group");
> > > +}
> > > +
> > > +static void
> > >  create_iochannel_with_socket (GTask *task)
> > >  {
> > >      MbimDevice *self;
> > > @@ -1138,7 +1145,7 @@ create_iochannel_with_socket (GTask *task)
> > >                              argc,
> > >                              NULL, /* envp */
> > >                              G_SPAWN_STDOUT_TO_DEV_NULL |
> > > G_SPAWN_STDERR_TO_DEV_NULL,
> > > -                            NULL, /* child_setup */
> > > +                            (GSpawnChildSetupFunc)
> > > spawn_child_setup,
> > >                              NULL, /* child_setup_user_data */
> > >                              NULL,
> > >                              &error)) {
> > > --
> > > 2.13.3
> > > _______________________________________________
> > > libmbim-devel mailing list
> > > libmbim-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/libmbim-devel
> > 
> 
> _______________________________________________
> libmbim-devel mailing list
> libmbim-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libmbim-devel