Commit a780b048 authored by Thomas Haller's avatar Thomas Haller

dns/dnsmasq: refactor tracking of dnsmasq process

Several points.

- We spawn the dnsmasq process directly. That has several downsides:

  - The lifetime of the process is tied to NetworkManager's. When
    stopping NetworkManager, we usually also stop dnsmasq. Or we keep
    the process running, but later the process is no longer a child process
    of NetworkManager and we can only kill it using the pidfile.

  - We don't do special sandboxing of the dnsmasq process.

- Note that we want to ensure that only one dnsmasq process is running
at any time. We should track that in a singletone. Note that NMDnsDnsmasq
is not a singleton. While there is only one instance active at any time,
the DNS plugin can be swapped (e.g. during SIGHUP). Hence, don't track the
process per-NMDnsDnsmasq instance, but in a global variable "gl_pid".

- Usually, when NetworkManager quits, it also stops the dnsmasq process.
Previously, we would always try to terminate the process based on the
pidfile. That is wrong. Most of the time, NetworkManager spawned the
process itself, as a child process. Hence, the PID is known and NetworkManager
will get a signal when dnsmasq exits. The only moment when NetworkManager should
use the pidfile, is the first time when checking to kill the previous instance.
That is: only once at the beginning, to kill instances that were
intentionally or unintentionally (crash) left running earlier.
This is now done by _gl_pid_kill_external().

- Previously, before starting a new dnsmasq instance we would kill a
possibly already running one, and block while waiting for the process to
disappear. We should never block. Especially, since we afterwards start
the process also in non-blocking way, there is no reason to kill the
existing process in a blocking way. For the most part, starting dnsmasq
is already asynchronous and so should be the killing of the dnsmasq

- Drop GDBusProxy and only use GDBusConnection. It fully suffices.

- When we kill a dnsmasq instance, we actually don't have to wait at
all. That can happen fully in background. The only pecularity is that
when we restart a new instance before the previous instance is killed,
then we must wait for the previous process to terminate first. Also, if
we are about to exit while killing the dnsmasq instance, we must register
nm_shutdown_wait_obj_*() to wait until the process is fully gone.
parent b288ea13
......@@ -2681,7 +2681,9 @@ nm_utils_get_start_time_for_pid (pid_t pid, char *out_state, pid_t *out_ppid)
g_return_val_if_fail (pid > 0, 0);
nm_sprintf_buf (filename, "/proc/%"G_GUINT64_FORMAT"/stat", (guint64) pid);
G_STATIC_ASSERT_EXPR (sizeof (GPid) >= sizeof (pid_t));
nm_sprintf_buf (filename, "/proc/%"G_PID_FORMAT"/stat", (GPid) pid);
if (!g_file_get_contents (filename, &contents, &length, NULL))
goto fail;
......@@ -1154,6 +1154,15 @@ nm_utils_dbus_normalize_object_path (const char *path)
guint64 nm_utils_get_start_time_for_pid (pid_t pid, char *out_state, pid_t *out_ppid);
static inline gboolean
nm_utils_process_state_is_dead (char pstate)
/* "/proc/[pid]/stat" returns a state as the 3rd fields (see `man 5 proc`).
* Some of these states indicate the the process is effectively dead (or a zombie).
return NM_IN_SET (pstate, 'Z', 'x', 'X');
gpointer _nm_utils_user_data_pack (int nargs, gconstpointer *args);
This diff is collapsed.
......@@ -1100,7 +1100,7 @@ const char *const NM_PATHS_DEFAULT[] = {
const char *
nm_utils_find_helper(const char *progname, const char *try_first, GError **error)
nm_utils_find_helper (const char *progname, const char *try_first, GError **error)
return nm_utils_file_search_in_paths (progname, try_first, NM_PATHS_DEFAULT, G_FILE_TEST_IS_EXECUTABLE, NULL, NULL, error);
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment