This actually allows the compiler/linker to mark the memory as read-only and any
modification will cause a segmentation fault.
I would also think that it allows the compiler to put the structure directly
beside the outer constant array (in which this pointer is embedded). That is good
locality-wise.
- g_ascii_strtoll() accepts leading spaces, but it leaves
the end pointer at the first space after the digit. That means,
we accepted "1: 0" but not "1 :0". We should either consistently
accept spaces around the digits/colon or reject it.
- g_ascii_strtoll() accepts "\v" as a space (just like `man 3 isspace`
comments that "\v" is a space in C and POSIX locale.
For some reasons (unknown to me) g_ascii_isspace() does not treat
"\v" as space. And neither does NM_ASCII_SPACES and
nm_str_skip_leading_spaces().
We should be consistent about what we consider spaces and what not.
It's already odd to accept '\n' as spaces here, but well, lets do
it for the sake of consistency (so that it matches with our
understanding of ASCII spaces, albeit not POSIX's).
- don't use bogus error domains in "g_set_error (error, 1, 0, ..."
That is a bug and we have NM_UTILS_ERROR exactly for error instances
with unspecified domain and code.
- as before, accept a trailing ":" with omitted minor number.
- reject all unexpected characters. strtoll() accepts '+' / '-'
and a "0x" prefix of the numbers (and leading POSIX spaces). Be
strict here and only accepts NM_ASCII_SPACES, ':', and hexdigits.
In particular, don't accept the "0x" prefix.
This parsing would be significantly simpler to implement, if we could
just strdup() the string, split the string at the colon delimiter and
use _nm_utils_ascii_str_to_int64() which gets leading/trailing spaces
right. But let's save the "overhead" of an additional alloc.
Only read the keyfile databases once and cache them for the remainder of
the program.
- this avoids the overhead of opening the file over and over again.
- it also avoids the data changing without us expecting it. The state
files are internal and we don't support changing it outside of
NetworkManager. So in the base case we read the same data over
and over. In the worst case, we read different data but are not
interested in handling the changes.
- only write the file when the content changes or before exiting
(normally).
- better log what is happening.
- our state files tend to grow as we don't garbage collect old entries.
Keeping this all in memory might be problematic. However, the right
solution for this is that we come up with some form of garbage
collection so that the state files are reaonsably small to begin with.
It will be used for "/var/lib/NetworkManager/seen-bssids" and
"/var/lib/NetworkManager/timestamps" which currently is implemented
in NMSettingConnection.
ethtool/mii API is based on the ifname. As an interface can be renamed,
this API is inherently racy. We would prefer to use the ifindex instead.
The ifindex of a device cannot change (altough it can repeat, which opens a
different race *sigh*).
Anyway, we were already trying to minimize the race be resolving the
name from ifindex immediately before the call to ethtool/mii.
Do better than that. Now resolve the name before and after the call. If
the name changed in the meantime, we have an indication that a race
might have happend (but we cannot be sure).
Note that this can not catch every possible kind of rename race. If you are very
unlucky a swapping of names cannot be detected.
For getters this is relatively straight forward. Just retry when we
have an indication to fall victim to a race (up to a few times). Yes, we
still cannot be 100% sure, but this should be very reliable in practice.
For setters (that modify the device) we also retry. We do so under the
assumption that setting the same options multiple times has no bad effect.
Note that for setters the race of swapping interface names is particularly
bad. If we hit a very unlucky race condition, we might set the setting on
the wrong interface and there is nothing we can do about it. The retry only
ensures that eventually we will set it on the right interface.
Note that this involves one more if_indextoname() call for each operation (in
the common case when there is no renaming race). In cases where we make
multiple ioctl calls, we cache and reuse the information though. So, for such
calls the overhead is even smaller.
When we set the MTU on the link we remember its previous source
(ip-config, parent-device or connection profile) and don't change it
again afterwards to avoid interfering with user's manual changes. The
only exceptions when we change it again are (1) if the parent device
MTU changes and (2) if the new MTU has higher priority than the one
previously set.
To allow a live reapply of the MTU property we also need to clear the
saved source, or the checks described above will prevent setting the
new value.
Fixes: 2f8917237f ('device: rework mtu priority handling')
https://bugzilla.redhat.com/show_bug.cgi?id=1702657
The 'bt-type' property alias accepts values provided by
gen_func_bt_type(); instead the 'bluetooth.type' property can only be
set to [dun, panu, nap] and therefore it doesn't need special
handling.
Fix the following assertion failure:
g_object_ref: assertion 'G_IS_OBJECT (object)' failed.
nm_settings_add_connection() can return a NULL connection.
Fixes: f034f17ff6 ('settings: keep the added connection alive for a bit longer')
make[3]: Entering directory 'NetworkManager/_build/sub'
CC clients/cli/nmcli-common.o
cc1: error: ./clients/common: No such file or directory [-Werror=missing-include-dirs]
cc1: all warnings being treated as errors
The only generated header in $builddir/clients/common is settings-docs.h
and only libnmc.la needs it. Include the directory on the command line
only when we know it exists.
On Ubuntu 14.04 kernel (4.4.0-146-generic, x86_64) this easily causes
test failures:
make -j 8 src/platform/tests/test-route-linux \
&& while true; do \
NMTST_SEED_RANDOM= ./tools/run-nm-test.sh src/platform/tests/test-route-linux -p /route/rule \
|| break; \
done
outputs:
...
/route/rule/1:
nmtst: initialize nmtst_get_rand() with NMTST_SEED_RAND=22892021
OK
/route/rule/2: >>> failing...
>>> no fuzzy match between: [routing-rule,0x205ab30,1,+alive,+visible; [6] 0: from all suppress_prefixlen 8 none]
>>> and: [routing-rule,0x205c0c0,1,+alive,+visible; [6] 0: from all suppress_prefixlen -1579099242 none]
**
test:ERROR:src/platform/tests/test-route.c:1695:test_rule: code should not be reached
Merge the function pointer get_func() into to_dbus_fcn().
Previously, get_func() as handled separately from to_dbus_fnc()
(formerly synth_func()). The notion was that synth-func would syntetize
properties that are D-Bus only. But that distinction does not seem
very helpful to me.
Instaed, we want to convert a property to D-Bus. Period. The
implementation should be handled uniformly. Hence, now that is
all done by property_to_dbus().
Note that property_to_dbus() is also called as default implementation
for compare-property. At least, for properties that are backed by a
GObject property.
The naming was not very clear. How does get_func(), synth_func()
and to_dbus() relate? What does synth_func() do anyway?
Answers:
- get_func() and synth_func() do very similar. They should be merged
in a next step.
synth_func() has the notion of "synthetize" a property for
D-Bus. As such, these properties are a bit unusual in that they
don't have a backing GObject property in the setting. But it'd
rather treat such properties like other properties. The step
in that direction will be to merge the to-dbus functions.
- to_dbus() converts a GValue of the GObject property go GVariant.
It's a simplified form of get_func()/synth_func() and a better name
is gprop_to_dbus_fcn().
The same for gprop_from_dbus_fcn().
For now, just rename.
We have certain artificial properties that not only depend on one
property alone or that depend on a property in another(!) setting.
For that, we have synth_func.
Other than that, synth_func and get_func are really fundamentally
similar and should be merged. That is because the distinction whether a
property value is "synthetized" or just based on a plain property is
minor. It's better to have the general concept of "convert property to
GVariant" in one form only.
Note that compare_property() is by default implemented based
on get_func. Hence, if get_func and synth_func get merged,
compare_property() will also require access to the NMConnection.
Also it makes some sense: some properties are artificial and actually
stored in "another" setting of the connection. But still, the property
descriptor for the property is in this setting. The example is the
"bond.interface-name" which only exists on D-Bus. It's stored as
"connection.interface-name".
I don't really like to say "exists on D-Bus only". It's still a valid
property, despite in NMSetting it's stored somehow differently (or not
at all). So, this is also just a regular property for which we have a
property-info vtable.
Does it make sense to compare such properties? Maybe. But the point is that
compare_property() function needs sometimes access to the entire
connection. So add the argument.
Always properly set NMSettInfoProperty.dbus_type, instead of leaving it
unspecified for GObject property based properties, and detect it each
time anew with variant_type_for_gtype().
Instead, autodetect and remember the dbus-type during _properties_override_add_struct().
For types that need special handling (GBytes, enums and flags) set a to_dbus() function.
This allows us to handle properties uniformly by either calling the to_dbus() function
or g_dbus_gvalue_to_gvariant().
- use cleanup attribute in get_property_for_dbus() and return early.
- use NM_FLAGS_HAS() macro in _nm_setting_to_dbus().
- in nm_setting_get_dbus_property_type() use g_return*() asserts
instead of crash or hard asserts.
- return early from variant_type_for_gtype().
This is a provide packages that install dispatcher scripts should depend
on. It will make it easier to keep track of them and possibly split out
the dispatcher into an optional package if not needed.
The monitors have been in place since the dispatcher has been introduced.
They need the daemon to do extra work know where the files are supposed to
be. It seems to me the complexity is not worth it.
Let's remove them now, making it easier to modify the dispatcher to look
for scripts in other places.
The dispatcher looks there for scripts now. This actually doesn't break
the RPM build, since it doesn't mind extra empty directories in
buildroot. Good.
While the keys of s390-options are from a well-behaving set of names
(that is enforced by nm_connection_verify()), the values are arbitrary
strings.
Our settings plugin must be able to express all values of a connection,
hence we need to support escapes.
- the previous implementation of nm_setting_wired_get_s390_option()
returned the elements in an arbitrary order (because it just iterated
idx times over the unsorted hash table).
- the API for "s390-options" suggests both accessing by index and by
name. Storing the options in a hash-table is not optimal for lookup
by index. It also requires us to sort the elements over and over
again.
Use instead a sorted array. Note that add/remove of course requires to
move the elements (and has thus O(n)).
- "s390-options" are very seldomly set. We shouldn't pay the price in every
NMSettingWired to allocate a GHashTable and deal with it.
- don't assert in nm_setting_wired_add_s390_option() and
nm_setting_wired_remove_s390_option() that the key is valid.
ifcfg-rh reader understandably does not want to implement additional
logic to pre-validate the key, so any invalid keys would trigger an
assertion failure. We have verify() for this purpose.
