There are a few cases where we don't want to clear a potential
nm-generated/volatile flag, but only mark the connection as
unsaved.
Otherwise, we wrongly end up clearing these flags and the connection
is wrongly not NM_DEVICE_SYS_IFACE_STATE_EXTERNAL.
Fixes: 35dc6421de
ifnet shall use the new_connection argument, not NM_CONNECTION(self).
Also, let the caller of the virtual function provide the right new_connection,
not having the virtual function figure that out.
The current behavior of update_unsaved is confusing. Give the argument
an enum with a name that describes better what's happening. Also, it
makes the uses grep-able.
We also do this for libnm, where it causes visible changes
in behavior. But if somebody would rely on the hashing implementation
for hash tables, it would be seriously flawed.
30. NetworkManager-1.9.2/src/settings/plugins/keyfile/nms-keyfile-writer.c:218:
check_return: Calling "g_mkdir_with_parents" without checking return
value (as is done elsewhere 4 out of 5
times).
25. NetworkManager-1.9.2/src/platform/nm-linux-platform.c:3969:
check_return: Calling "_nl_send_nlmsg" without checking return value (as
is done elsewhere 4 out of 5 times).
34. NetworkManager-1.9.2/src/nm-core-utils.c:2843:
negative_returns: "fd2" is passed to a parameter that cannot be negative.
26. NetworkManager-1.9.2/src/devices/wwan/nm-modem-broadband.c:897:
check_return: Calling "nm_utils_parse_inaddr_bin" without checking
return value (as is done elsewhere 4 out of 5 times).
3. NetworkManager-1.9.2/src/devices/bluetooth/nm-bluez5-manager.c:386:
check_return: Calling "g_variant_lookup" without checking return value
(as is done elsewhere 79 out of 83 times).
16. NetworkManager-1.9.2/libnm-util/nm-setting.c:405:
check_return: Calling "nm_g_object_set_property" without checking return
value (as is done elsewhere 4 out of 5 times).
During write, it can regularly happen that the connection gets modified.
For example, keyfile never writes blobs as-is, it always writes the
blob to an external file, and replaces the certificate property with
a path.
Other reasons could be just bugs, where the reader and writer are not doing
a proper round trip (these cases should be fixed).
Refactor commit_changes(), to return the re-read connection to
the settings-connection class, and handle replacing the settings
there.
Also, prepare for another change. Sometimes we first call replace_settings()
followed by commit_changes(). It would be better to instead call commit_changes()
first, and only on success proceed with replace_settings(). Hence, commit_changes()
gets a new argument new_connection, that can be used to write another
connection to disk.
Don't delegate so much to the virtual function commit_changes().
Calling the callback is not the task of the virtual function,
because every implementation must do that.
There are some minor changes in behavior for ifnet, where we now
first setup the monitors and reload the parsers, before invoking
the callback.
Replace the usage of g_str_hash() with our own nm_str_hash().
GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.
Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.
This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.
At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
When first trying to write out the connections we need to ensure that the
keyfile directory exists, as the /etc/ tree may be either stateless or
reset initially.
Creating the directory on demand ensures that we have a chance for our
writes to actually work.
[lkundrak@v3.sk: dropped a comment for what seems obvious, minor style
fixes]
In practice, this should only matter when there are multiple
header files with the same name. That is something we try
to avoid already, by giving headers a distinct name.
When building NetworkManager itself, we clearly want to use
double-quotes for including our own headers.
But we also want to do that in our public headers. For example:
./a.c
#include <stdio.h>
#include <nm-1.h>
void main() {
printf ("INCLUDED %s/nm-2.h\n", SYMB);
}
./1/nm-1.h
#include <nm-2.h>
./1/nm-2.h
#define SYMB "1"
./2/nm-2.h
#define SYMB "2"
$ cc -I./2 -I./1 ./a.c
$ ./a.out
INCLUDED 2/nm-2.h
Exceptions to this are
- headers in "shared/nm-utils" that include <NetworkManager.h>. These
headers are copied into projects and hence used like headers owned by
those projects.
- examples/C
Writing a connection to keyfile and read it back should result
in the identical setting. That is, a full round-trip would not
alter any information.
That is however particularly not true for certificate properties, where
the keyfile writes blobs to file and coerces paths.
Thus, whenver writing a keyfile we must read back what we just
wrote and use that instead.
Add API to re-read the keyfile after writing it.
Usually, we would expect that whenever we serialize
something to disk, it can be read back exactly the same.
That is however not true for certificates, where we mangle
path and blobs while writing to file.
Anyway, extend the write-API to re-read what we just wrote.
The tests got extended to assert that whatever we write can
be read back the same.
Later, we want to reinject the reread connection into the settings
plugin again.
When a ifcfg-rh connection becomes unamanaged it is removed from the
connection list in NMSettings and marked as removed; it is however
kept alive in the plugin and can become managed again later. To avoid
failed assertions, the @removed flag of the NMSettingsConnection must
be cleared if the connection is not being disposed.
Keep the include paths clean and separate. We use directories to group source
files together. That makes sense (I guess), but then we should use this
grouping also when including files. Thus require to #include files with their
path relative to "src/".
Also, we build various artifacts from the "src/" tree. Instead of having
individual CFLAGS for each artifact in Makefile.am, the CFLAGS should be
unified. Previously, the CFLAGS for each artifact differ and are inconsistent
in which paths they add to the search path. Fix the inconsistency by just
don't add the paths at all.
The tests were checking that the keyfiles had permission
0600 and there was a check-local target to prepare the permissons
of the tests.
That is inconvenient, and it is unexpected to have a check-local
target for such a case.
Also, the tests were not testing that keyfile reader would
bail out on invalid permissions. So just skip the check for
testing.
In some places we use g_file_set_contents() after a umask() to limit
the permissions of the created file. Unfortunately if the containing
directory has a default ACL the umask will be ignored and the new file
will have a mode equal to the default ACL (since g_file_set_contents()
opens the file with mode 0666).
Calling a chmod() after the file gets created is insecure (see commit
60b7ed3bdc) and so the only solution seems to be to reimplement
g_file_set_contents() and accept a mode as parameter.
We already had similar functions in the tree, consolidate them into a
new generic utility function.
https://bugzilla.gnome.org/show_bug.cgi?id=769702
First of all, G_LOG_DOMAIN only matters when using g_log() directly.
Inside core, we always want to log via nm-logging. Every call to a
g_log() is a bug in the first place (like a failed assertion that logs
a g_critical() during g_return_if_fail()).
So, for all practic purposes, the logging domain is not used.
For nm-logging, the G_LOG_DOMAIN has no effect. Unless we find a proper
use of this domain, G_LOG_DOMAIN should not differ from what the rest of
core.
- use _NM_GET_PRIVATE() and _NM_GET_PRIVATE_PTR() everywhere.
- reorder statements, to have GObject related functions (init, dispose,
constructed) at the bottom of each file and in a consistent order w.r.t.
each other.
- unify whitespaces in signal and properties declarations.
- use NM_GOBJECT_PROPERTIES_DEFINE() and _notify()
- drop unused signal slots in class structures
- drop unused header files for device factories
- don't include "nm-default.h" in header files. Every source file must
include as first header "nm-default.h", thus our headers get the
default include already implicitly.
- we don't support compiling NetworkManager itself with a C++ compiler. Remove
G_BEGIN_DECLS/G_END_DECLS from internal headers. We do however support
users of libnm to use C++, thus they stay in public headers.
(cherry picked from commit f19aff8909)
