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
process.
- 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.
The leading underscore has the notion that this would be a private function.
It really isn't, and it would be fine for the user to call it directly.
Just like we have g_slice_free() and g_slice_free1().
It is like strcmp(), but has a signature suitable for GCompareDataFunc.
This is necessary with nm_utils_ptrarray_find_binary_search()
to search a sorted strv array.
The fault is here really C, which doesn't allow inline static functions.
So, you need all kinds of slightly different flavors for the same
callbacks (with or without user-data).
Note that glib2 internally just casts strcmp() to GCompareDataFunc ([1]),
relying on the fact how arguments are passed to the function and
ignoring the additional user-data argument. But I find that really
ugly and probably not permissible in general C. Dunno whether POSIX
would guarantee for this to work. I'd rather not do such function
pointer casts.
[1] 0c0cf59858/glib/garray.c (L1792)
If there is only one argument, we can assume this is a plain string.
That is especially the case, because g_set_error() is G_GNUC_PRINTF()
and would warn if this would be a format string with missing parameters.
This is for convenience. Previously, one was compelled to explicitly
choose between nm_utils_error_set_literal() and nm_utils_error_set().
Now, it automatically chooses.
Note that there are a few things that won't work, like
nm_utils_error_set (error, code, "bogus %u escape");
But that's good. You get a compiler warning (as you used to)
and it's clear in this case you really need
nm_utils_error_set_literal().
Usually we avoid GSList, because I think it's not a great data type.
Anyway, our match-specs are just a GSList of strings, so we need some
API to handle them.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
2019-04-18 19:57:27 +02:00
Renamed from shared/nm-utils/nm-shared-utils.h (Browse further)
