After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 761299 - [review, openvpn] properly handle child/openvpn processes [th/handle-child-process-bgo761299]
[review, openvpn] properly handle child/openvpn processes [th/handle-child-pr...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-01-29 16:22 UTC by Thomas Haller
Modified: 2016-02-02 17:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2016-01-29 16:22:03 UTC 
please review
Comment 1 Dan Williams 2016-01-29 22:01:55 UTC 
LGTM except for the g_usleep (1).  That wouldn't give a child process a lot of time to clean itself up?
Comment 2 Thomas Haller 2016-01-30 12:23:49 UTC 
(In reply to Dan Williams from comment #1)
> LGTM except for the g_usleep (1).  That wouldn't give a child process a lot
> of time to clean itself up?

yeah... it's not clear what to do in this case... maybe only send SIGKILL to the still alive process and not SIGKILL at all?

Best would be that the process sticks around until we all child processes terminate (and still properly send SIGKILL after the 2 seconds timeout).
Comment 3 Thomas Haller 2016-01-31 13:12:47 UTC 
how about v2?

Now, we still send only SIGTERM in the dispose of the service instance, but wait before shutdown until all child processes properly terminated.
Comment 4 Beniamino Galvani 2016-02-02 12:31:25 UTC 
> service: fix handling child processes and their cleanup

+       pid_data->watch_id = 0;
+       gl.pids_pending_list = g_slist_remove (gl.pids_pending_list , pid_data);
+       pids_pending_data_free (pid_data);

The first line can be dropped since pids_pending_data_free() already
does that.

	/* OpenVPN doesn't supply useful exit codes :( */
-       if (error == 0)
+        if (WIFEXITED (status) && WEXITSTATUS (status) == 0)

extra space.

+             g_message ("openvpn[%ld] exited with success", (long) pid);
+    }
+       else if (WIFSTOPPED (status))

Indentation of '}'.

The rest LGTM.
Comment 5 Thomas Haller 2016-02-02 15:22:43 UTC 
(In reply to Beniamino Galvani from comment #4)
> > service: fix handling child processes and their cleanup
> 
> +       pid_data->watch_id = 0;
> +       gl.pids_pending_list = g_slist_remove (gl.pids_pending_list ,
> pid_data);
> +       pids_pending_data_free (pid_data);

I think watch_id should be explicitly set to zero to prevent pids_pending_data_free() calling g_source_remove on it. So this seems right.



> The first line can be dropped since pids_pending_data_free() already
> does that.
> 
> 	/* OpenVPN doesn't supply useful exit codes :( */
> -       if (error == 0)
> +        if (WIFEXITED (status) && WEXITSTATUS (status) == 0)
> 
> extra space.
> 
> +             g_message ("openvpn[%ld] exited with success", (long) pid);
> +    }
> +       else if (WIFSTOPPED (status))
> 
> Indentation of '}'.


Fixed


Thanks