They are basically the same, with a minor difference where the @filename
argument determines whether to write a new file or do an update.
Also, rename them, to give them a nms_* prefix in the header file.
As writing a connection to disk might modify it, we re-read
it back and use what we actually found on disk.
For example, if you have a connection with ipv6.method=ignore,
ifcfg-rh writer will not persist the ipv6.route-metric. That
is likely a bug in the writer. Before this patch, changing
the route metric would seemingly succeed, but on the next reload
from this, the changes are lost.
We should fix such bugs. Regardless, it's better to pick up
what we wrote to disk, instead of later.
Previously, we would first call replace_settings(), followed by
commit_changes(). There are two problems with that:
- commit_changes() might fail easily, for example if the settings
plugin cannot handle the connection. In that case, we fail the operation,
but still we already replaced the settings in memory. We should
first write to disk, and only when that succeeded, replace our
settings.
Also, note that replace_settings() cannot really fail at that
point, because we already validate the setting previously
(everything else would be a bug).
- commit_changes() might modify the connection while writing it.
We re-read it and replace the settings. If we already replaced
it before, we replcace the settings twice -- needlessly.
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.
The virtual function replace_and_commit() had only one implementation: ifcfg-rh.
Refactor the code, to delegate less. That is, the main part of
replace-and-commit is not delegated to a virtual function.
Now, the virtual function is only a pre-check hook, so that
the ifcfg-rh implementation can abort the function.
There are no functional changes.
Also, need to avoid danling pointers in clear_monitor().
This was not really a problem, because we would always call
cancel() before setup(). Still, it's fragile.
In many scenarios, we have no use for the file descriptor
after nm_utils_fd_get_contents(). We just want to read it
and close it.
API wise, it would be nice that the get_contents() function never
closes the passed in fd and it's always responsibility of the caller.
However, that costs an additional dup() syscall that could
be avoided, if we allow the function to (optionally) close
the file descriptor.
libnm-core limits the rande for GATEWAY_PING_TIMEOUT to 0 to 600.
See commit e86f8354a7, "device: restart
ping process when it exits with an error".
The reader must not pass value out of range to g_object_set().
Clamp and warn.
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.
We added "ipv4.route-table-sync" and "ipv6.route-table-sync" to not change
behavior for users that configured policy routing outside of NetworkManager,
for example, via a dispatcher script. Users had to explicitly opt-in
for NetworkManager to fully manage all routing tables.
These settings were awkward. Replace them with new settings "ipv4.route-table"
and "ipv6.route-table". Note that this commit breaks API/ABI on the unstable
development branch by removing recently added API.
As before, a connection will have no route-table set by default. This
has the meaning that policy-routing is not enabled and only the main table
will be fully synced. Once the user sets a table, we recognize that and
NetworkManager manages all routing tables.
The new route-table setting has other important uses: analog to
"ipv4.route-metric", it is the default that applies to all routes.
Currently it only works for static routes, not DHCP, SLAAC,
default-route, etc. That will be implemented later.
For static routes, each route still can explicitly set a table, and
overwrite the per-connection setting in "ipv4.route-table" and
"ipv6.route-table".
- clearify in the manual page that setting retry to 1 means to try
once, without retry.
- log the initially set retry value in nm_settings_connection_get_autoconnect_retries().
- use nm_settings_connection_get_autoconnect_retries() in
nm_settings_connection_can_autoconnect().
Distinguish between connections blocked from autoconnecting by user
request and connections blocked because they failed (and would fail
again).
Later, the reason will be used to unblock failed connection when some
conditions change.
The previous parsing was done using regex. One could implement a
complex regex to parse the setting. However, as it was implemented,
the regex would just pick out parts of the line that it expects,
and ignore unknown parts.
Let's be strict about what we parse. The only strong requirement
is that NM can parse everything that was written by NM itself.
Eventually, we could extend the parser to accept everything that
initscripts accept.
Initscripts split the line at $IFS and do filename globbing on the
arguments. That is ugly, because globbing is of coures wrong (we don't
do that). But also, the splitting at $IFS cannot be escaped, hence for
initscripts it is impossible to use '<space><tab><newline>'. We do that
too, as it makes it easy to parse. Later we may want to extend this to
allow a form of escaping/quoting.
Yes, we may now ignore routes that are not defined as we expect them.
svGetValueStr() is preferred over svGetValueStr_cp() because it may safe
an additional string copy (if the value needs no unescaping/unquoting).
Also, use nm_utils_strsplit_set() because it saves to copy each word.
There are some changes here. For example, read_8021x_list_value()
previously would not strip empty words. When switching from
g_strsplit_set() to nm_utils_strsplit_set(), empty words are implicitly
skipped.
Due to a bug, NetworkManager used to write device routes with "via (null)".
That was fixed in commit af8aac9b54 and
bug rh#1452648.
Add a unit test to ensure we keep accepting such (invalid) routes that
NetworkManager once wrote.
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]
- kernel ignores rtm_tos for IPv6 routes. While iproute2 accepts it,
let libnm reject TOS attribute for routes as well.
- move the tos field from NMPlatformIPRoute to NMPlatformIP4Route.
- the tos field is part of the weak-id of an IPv4 route. Meaning,
`ip route add` can add routes that only differ by their TOS.
This drops some redundant rules and orderes the remaining ones by
precedence.
The 'root' rules take precedence over the 'default' rules, so order
the file accordingly.
It is not necessary to repeat send_destination rules, as the default
rules already allows everyone to send to the interface.
Moreover, it is not necessary to restrict the ownership of the name
in the default context, as this is already done by the system-wide
default rule.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Coverity complains about not checking the return value:
src/settings/nm-settings-connection.c:2329: check_return: Calling "g_key_file_load_from_file" without checking return value (as is done elsewhere 6 out of 7 times).
While at it, refactor the code and check whether the timestamp
is valid.
There are a lot of places where we want to either write a number,
or conditionally clear it. Like:
mtu = nm_setting_wireless_get_mtu (s_wireless);
if (mtu)
svSetValueInt64 (ifcfg, "MTU", mtu);
else
svUnsetValue (ifcfg, "MTU");
