Found by covscan:
NetworkManager-1.22.0/shared/nm-glib-aux/nm-dbus-aux.c:361:
missing_va_end: va_end was not called for "ap".
Fixes: ce36494c0a ('shared: add nm_dbus_error_is() helper')
"nm-glib-aux/nm-logging-fwd.h" provides macros like _LOGD() to be reused
by various parts which implement logging (by defining _NMLOG() accordingly).
libnm also has logging, however it uses different logging levels
aside LOGD_DEBUG.
Instead, implement _LOGD() using a define _LOGL_DEBUG, so that libnm can
redefine thos _LOGL_DEBUG defines and use the _LOGD() macro.
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
"shared/nm-utils" got long renamed and split into separate parts. The remaining
tests are really to test nm-std-aux and nm-glib-aux (no libnm dependencies). Move
the tests to the appropriate place.
inet_aton() also supports IPv4 addresses in octal (with a leading '0')
or where not all 4 digits of the address are present.
Add nm_utils_parse_inaddr_bin_full() to optionally fallback to
parse the address with inet_aton().
Note taht inet_aton() also supports all crazy formats, including
ignoring trailing garbage after a whitespace. We don't want to accept
that in general.
Note that even in legacy format we:
- accept everything that inet_pton() would accept
- additionally, we also accept some forms which inet_aton() would
accept, but not all.
That means, the legacy format that we accept is a superset of
inet_pton() and a subset of inet_aton(). Which is desirable.
We have our NM specific logging and log levels. Maybe we should
not have that, and instead only rely on syslog (like systemd)
or glog(). Anyway, currently we have one way and it makes sense
that this is also used outside from "src".
Move the helper function to parse log levels from string to
"nm-logging-base.h" so that we can use the same logging levels
outside of core.
This moves code that is currently GPL2+ licensed to
LGPL2.1+. However as far as I see, this code was entirely written
by Red Hat employees who would not object with this change. Also,
it's as obvious and trivial as it gets.
We have "nm-logging-fwd.h", which (as the name implies) is header-only.
Add instead a "nm-logging-base.c", which also contains implementation for
logging functions that are not only useful under "src/nm-logging.c"
We will rework NMClient entirely. Then, the synchronous initialization will also use
the asynchronous code paths. The difference will be that with synchronous initialization,
all D-Bus interaction will be done with an internal GMainContext as current thread default,
and that internal context will run until initialization completes.
Note that even after initialization completes, it cannot be swapped back to the user's
(outer) GMainContext. That is because contexts are essentially the queue for our
D-Bus events, and we cannot swap from one queue to the other in a race
free manner (or a full resync). In other words, the two contexts are not in sync,
so after using the internal context NMClient needs to stick to that (at least, until
the name owner gets lost, which gives an opportunity to resync and switch back to the
user's main context).
We thus need to hook the internal (inner) GMainContext with the user's (outer) context,
so when the user iterates the outer context, events on the inner context get dispatched.
Add nm_utils_g_main_context_create_integrate_source() to create such a GSource for
integrating two contexts.
Note that the use-case here is limited: the integrated, inner main context must
not be explicitly iterated except from being dispatched by the integrating
source. Otherwise, you'd get recursive runs, possible deadlocks and general
ugliness. NMClient must show restrain how to use the inner context while it is
integrated.
Some compilers don't convert arrays as _Generic() type selectors to
their pointer type. That means, for those compilers the generic type
would be an array and not a pointer. Work around that by adding zero
to the pointer/array argument.
Also, I cannot get this to work with "clang-3.4.2-9.el7". Disable it
for that compiler. The value of the generic check is anyway that it only
needs to work with some compiler combinations. That will trigger a
compilation failure and we can fix the implementation also for compilers
that don't support the macro.
See-also: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1930.htm
There are two macros: NM_GOBJECT_PROPERTIES_DEFINE_BASE() and
NM_GOBJECT_PROPERTIES_DEFINE(). The former just defines the
property enums and the obj_properties array. The latter also
defines the functions _notify() and _nm_gobject_notify_together_impl().
That means, depending on whether you actually use _notify(), you have
to choose one of the macros. I think that is unnecessarily cumbersome.
Let's mark the function as _nm_unused so that the compiler doesn't
complain about the unused function. I don't think it's a problem
to use NM_GOBJECT_PROPERTIES_DEFINE() even if you don't actually use
_notify().
We preferably should use our convenience macros like _LOGD().
Since those macros expand to _NMLOG() (which needs to be defined
separately), we can move it to "nm-logging-fwd.h" and reuse.
glib really likes the numeric source IDs. That is, g_idle_add(), g_timeout_add(),
etc. return those IDs, that can then be destroyed with g_remove_source() (or
nm_clear_g_source()).
I think these numeric IDs are really not great.
- API like g_idle_add() and g_remove_source() only works with the g_main_context_get_default()
instance. That means, you cannot use this API for any other contexts. If you'd insist on using
numeric IDs, you'd need to call g_main_context_find_source_by_id() on the right context
first (but you'd also have to track the context alongside the ID).
- g_remove_source() requires first a call to g_main_context_find_source_by_id(). This involves
taking a mutex and doing an extra hash lookup.
Instead, it often seems preferable to use the GSource instance directly. It works
with any context, it can be referenced and unreferenced, and it can be destroyed, and
avoids the overhead of g_main_context_find_source_by_id().
The only downside really is that keeping a GSource pointer takes one pointer size, while
the guint source ID is usually only 4 bytes.
Anyway, I think we should deal more with GSource instances directly. Hence, add this
convenience macro, that works like nm_clear_g_source().
Note that we should always set the source-tag of our GTask.
This allows us to better assert that the user uses the right
_finish() method for the task.
The plain g_task_new() does not have a souce-tag argument. Hence, we would
always need to explicitly call g_task_set_source_tag().
Likewise, to check the source tag, we would always need to write
g_return_val_if_fail (g_task_is_valid (result, self), FALSE);
g_return_val_if_fail (g_async_result_is_tagged (result, tag), FALSE);
Actually, g_async_result_is_tagged() uses the GAsyncResultIface to
call iface->is_tagged(). This has unnecessary overhead, so we should
just call g_task_get_source_tag() directly.
Add helper functions for that.
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.
Having G_PID_FORMAT macro is useful, but it's only available in
recent glib versions. Add a compat implementation and a test that
our assumptions hold.
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().
