31. NetworkManager-1.9.2/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:974:
uninit_use_in_call: Using uninitialized value "contents_rest" when
calling "__strtok_r_1c".
33. NetworkManager-1.9.2/src/nm-core-utils.c:1957:
uninit_use: Using uninitialized value "s".
148. NetworkManager-1.9.2/src/nm-core-utils.c:1924:
uninit_use_in_call: Using uninitialized value "s" when calling
"nm_strstrip_avoid_copy".
We want to support large number of routes. Reduce the number
of copies, by adding internal accessor functions.
Also, work around a complaint from coverity:
46. NetworkManager-1.9.2/libnm-core/nm-utils.c:1987:
dereference: Dereferencing a null pointer "names".
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).
Now the plugin can only recognize team or bond slaves of type
ethernet, vlan or infiniband.
Instead, check the presence of a team or bond master for all types of
connection to allow arbitrary stacking of interfaces.
NMSettingGeneric has no properties at all. Hence, nm_connection_diff() would report that
a connection A with a generic setting and a connection B without a generic setting are
equal.
They are not. For empty settings, let nm_setting_diff() return also empty difference
hash.
Before commit 3ecb57fdc4 ("settings: get rid of callback arguments
for nm_settings_connection_delete()") audit logging was done in the
callback, which was run by nm_settings_connection_delete() with a
reference to the connection to keep it alive. Now we have to keep an
extra reference to ensure it doesn't go away.
Fixes: 3ecb57fdc4
Eventually, we want to fully implement policy routing and
handle rules as well. When that happens, we will use the
route-table setting to tell NetworkManager to handle the
rule file as well.
Since we currently don't yet support that, we should reject
configuring a non-zero routing table combined with a rule file,
because later we will change behavior in that case.
We should support an arbitrary number of routes and addresses.
Arguably, our accessors for shvarFile are O(n), hence with
large ifcfg files, we will have a performance problem. The
fix for that would be to index the files.
The only implementation of can_commit() was ifcfg-rh, which
bails out with complex routes.
Note that the only caller of can_commit() (update_auth_cb()),
immidiately afterwards called nm_settings_connection_commit_changes(),
which, a few layers down in nms_ifcfg_rh_writer_write_connection()
as first thing errors out in presence of complex routes.
The check was redundant.
In general, a can_commit() function before a commit_changes() makes
no sense, because commit_changes() can just fail with error.
write_ip4_aliases() does not collect internal in-memory state, instead
it writes state to disk and deletes extraneous alias files.
It should be done after we completed our pre-run checks to generated
the data we want to write.
Instead of having set_secret() for each call open the file,
mangle it, and write it back, collect all secrets and process
them at the end once.
Also, previously set_secrets() ignored failures to write a secret and
added the secret in plain to the ifcfg file. Let's not do that.
Also, purge all other entires form the secrets file. Not only
the once that we explicitly touch.
Eventually, we should generate all configuration in-memory
first, and only after validating everything write to disk.
That avoids that we start touching files, and later encounter
a fatal error that let's us abort writing the connection.
Also, previously, we would not purge the route file if
write_ip6_setting() returns early for slave types.
- we now safe all routes we have, not limited to 256.
- we use svUnsetAll() to delete the existing keys. This is
faster then probing them one-by-one, and not limited to
256 keys (which we were checking before).
Note that we always try to load an existing file and
drop the unneeded keys. We do that, so that unrelated
entries and comments don't get the deleted. Also, so
that the order of the variables is not changed.
When calling svOpenFileInternal() with @create, we don't care about
potential errors reading the file. We shouldn't return NULL in such
case, but always create a shvarFile instance.
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.
