The manual page claimed that for "connectivitiy-change" actions, the dispatcher
scripts would get as first argument (the device name) "none". That was not done,
only for "hostname" actions.
For consistency, maybe that should be adjusted to also pass "none" for connectivity
change events. However, "none" is really an odd value, if there is no device. Passing
an empty word is IMO nicer. So stick to that behavior, despite being inconsistent.
Also fix the documentation about that.
nm_str_hash() uses siphash24. We want to use that everywhere.
The only concern would be that it is slower. But if that's the concern,
then we would have many more performance critical places to care
about first.
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
There is already a way to hide/shadow scripts in "/usr/lib/NetworkManager/dispatcher.d":
by putting a file of the same name in "/etc/NetworkManager/dispatcher.d".
There is also the special case that if the file symlinks to "/dev/null", the
file is silently ignored. This is the proper way to hide a script.
I think we should also take a plain empty file as user indication to hide a script.
This way, one can simply hide a file with
# touch /etc/NetworkManager/dispatcher.d/10-ifcfg-rh-routes.sh
It's an alternative to symlinking to /dev/null.
In practice, nowadays g_free() is the same as free(), so there is no
difference. However, we still should not mix the two and use free()
for data that was allocated with malloc() -- in this case, the memory
was allocated by libc's realpath().
We don't need such data duplicated. The build setup should
have only one configuration_data() for patching such values.
Now we only have one global, immutable data_conf dictionary with
configuration values. Note that none of the users of data_conf uses all
entries, but as the entries are basically only dependent on the
meson/configure option and valid for the entire project, this simplifies
to handling.
Some variables belong to variables in their correspondent pkg-config
file.
These variables have been renamed to `dependency_variable` to
reflect the dependency and variables from pkg-config files they are
related to.
Some of these has also been fixed to use paths relative to
installation prefix.
Different objects used in the `test-dispatcher-envp` target
have been grouped together.
The dependency over the `libnm` library has been removed as it is
unnecessary.
Extra variables are used for sources of targets in the `dispatcher`
build file. These have been moved to the `source` parameter because
using them directly avoiding the creation of extra variablse doesn't
hurt readibility.
The compiler flags `cflags` variable has also been renamed to be
consistent with the rest of build files.
The `nm-default.h` header is used widely in the code by many
targets. This header includes different headers and needs different
libraries depending the compilation flags.
A new set of `*nm_default_dep` dependencies have been created to
ease the inclusion of different directorires and libraries.
This allows cleaner build files and avoiding linking unnecessary
libraries so this has been applied allowing the removal of some
dependencies involving the linking of unnecessary libraries.
Functions derived from generators as `configure_file`,
`custom_target` and `i18n.merge_file` can use placeholders like
`@BASENAME@` that removes the extension from the input filename
string.
The output string has been replaced by this placeholder that
allows in some cases the use of less variables.
WARNING: Project targetting '>= 0.44.0' but tried to use feature
introduced in '0.50.0': install arg in configure_file
From the documentation:
"install (added 0.50.0) When true, this generated file is installed
during the install step, and install_dir must be set and not
empty. When false, this generated file is not installed regardless of
the value of install_dir. When omitted it defaults to true when
install_dir is set and not empty, false otherwise."
The parameter can be omitted because install_dir is set.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/216
Some linux distros (e.g., RHEL, CentOS and Fedora) ship a dispatcher
script with their dhclient package in order to run user dhclient hook
scripts also when connections are run by NetworkManager.
The scripts convert the var names in the environment to the ones
expected in the dhclient hook scripts.
This feature is currently use by cloud-init to retrieve the MS Azure
server endpoint address, which is sent in the private dhcp option 245.
We just redefined the default dhclient var names for the private options
from "unknown_xyz" to "private_xyz". In order to let current cloud-init
version to be able to retrieve the Azure server endpoint address, add
the legacy var name "unknown_245" to the dispatcher script environment.
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.
We don't need it anymore.
Still, for tests let gdbus-codegen run and generate the sources and
compile them. We want to keep "dispatcher/nm-dispatcher.xml" and ensure
that it is still valid.
Just hook into GDBusConnection directly. No need for this additional
layer that provides nothing. It doesn't even provide extra type-safety,
because you still need to get the arguments of the signal handler right.
That that point, it's as hard as getting the format string for
g_variant_get() right.
It still adds lines of code, because the "Action" method has such a
large argument list.
"nm-dispatcher.c" does something rather simple. It is natural that it
has a bit of global data to keep track of that it's doing ("gl").
But this does not lend itself to pack the job of dispatcher into an
object. In fact, the Handler object was little more about packaging
the GDBus interface skeleton and a bit of state.
Global variables are often problematic because they makes unit testing hard.
But first of all, we have no test for this (we should). But it's not said
that you need an "object" to make testing easier. If we want to make
individual bits easier testable, we can just as well pass all required
parameters explicitly instead of accessing global variables. Since we
package global variables neatly in "gl", this is very simple to
refactor. Also, global variables can make code harder to understand. But
the problem is the amount of state that is accessible. This is not
alleviated by packaging the state in a Handler object.
As there is always only one handler instance, this provides very little
benefit.
I will drop the GDBus interface skeleton soon. So this Handler object
will have even less purpose. Drop it.
It's anyway a singleton that is still referenced by other components.
So unrefing it in the mainloop does not actually release any memory.
However, the GDBusConnection singleton is fundamental for the run of
the program. Keep it accessible in the global variables.
Note that soon I will drop the GDBusInterfaceSkeleton and only operate
on the GDBusConnection. Then it makes more sense to keep it around.
Note that usually we want to keep the amount of global state small.
But this connection is anyway a singleton (that we already implicitly
use). So, it doesn't change the amount of global state nor does it really
have much state (we either have a reference to the singleton or we don't).
Split command line parsing out of the main() function. For one, it's
an self-contained step, so we can make main() simpler.
Also, we don't need the GOptionEntry on the stack of the main() function
for the remainder of the program.
It's ugly to uncoordinated just call exit(). We should quit the mainloop
and clean up everything we had going.
Note that since Handler has no dispose() function, we also need to hack
a g_signal_handlers_disconnect_by_func(). This will change soon.
Silence messages like
find-scripts: Failed to open dispatcher directory '/usr/lib/NetworkManager/dispatcher.d': Error opening directory “/usr/lib/NetworkManager/dispatcher.d”: No such file or directory
find-scripts: Failed to open dispatcher directory '/usr/lib/NetworkManager/dispatcher.d/pre-up.d': Error opening directory “/usr/lib/NetworkManager/dispatcher.d/pre-up.d”: No such file or directory
The messages about loading the scripts are releated to a particular
request. Hence, they should be logged with that context.
Also, we should avoid using g_log() directly. Use our logging macros
instead.
Previously, we would log several messages with level "debug" / g_info().
_LOG_R_D (request, "start running ordered scripts...");
_LOG_R_D (request, "new request (%u scripts)", request->scripts->len);
_LOG_R_D (request, "completed: no scripts");
Note that this effectively logs a message for every event. I think that
is to verbose and not suitable for regular logging.
Only enable these messages if debug logging is enabled. As such, these debug
level messsages now are enabled together with the trace level messages.
What we currently print as "info" level is too verbose for regular
operation. It prints two messages for every dispatcher event. That's
already for debugging.
Next that will be downgraded, so rename "debug" to "trace" and "info" to
"debug".
There is only renaming, no change in behavior.
The library is called "libnm_core". So the dependency should be called
"libnm_core_dep", like in all other cases.
(cherry picked from commit c27ad37c27)
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
(cherry picked from commit af07ed01c0)
- use cleanup macros everywhere.
- In particular use nm_auto_clear_variant_builder to free the
GVariantBuilder in the error cases. Note that the error cases
anyway are asserted against, so during a normal test run there
was no leak. But we should not write software like that.
- use nm_utils_strsplit_set_with_empty() instead of g_strsplit_set().
We should use our variant also in unit-tests, because that way the
function gets more test coverage. And it likely performs better
anyway.
For static functions inside a module, the compiler determines on its own
whether to inline the function.
Also, "inline" was used at some places that don't immediatly look like
candidates for inlining. It was most likely a copy&paste error.
