This does not match kernel behavior. You seem to be able to configure
multicast_snooping=auto,enabled with multicast_snooping off just fine.
Possibly this constrained was inspired by `ip link`, which says:
mcast_router MULTICAST_ROUTER - set bridge's multicast router if IGMP
snooping is enabled.
But kernel doesn't enforce this:
ip link delete br0 2>/dev/null; \
ip link add br0 type bridge mcast_router 1 mcast_snooping 0; \
grep ^ /sys/devices/virtual/net/br0/bridge/{multicast_router,multicast_snooping}
gives:
/sys/devices/virtual/net/br0/bridge/multicast_router:1
/sys/devices/virtual/net/br0/bridge/multicast_snooping:0
We probably should not implement additional constrains on top of what
kernel does, if the conditions are that obscure.
Fixes: e01d3b4c2b ('nm-setting-bridge: add 'multicast-router' bridge option')
nm_keyfile_read() and nm_keyfile_write() will be public API.
As such, it must be flexible and extendible for future needs.
There is already the handler callback that fully solves this
(e.g. a future handler event could request whether a certain
behavior is enabled or not).
As additional possibility for future extension, add a flags
argument. Currently no flags are implemented.
UBSan marks these:
libnm-core/tests/test-setting.c:2146:2: runtime error: left shift of 65521 by 16 places cannot be represented in type 'int'
#0 0x561739bed935 in test_tc_config_qdisc libnm-core/tests/test-setting.c:2146
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
This solves a bug exposed by the following cmds:
$ nmcli c add type bond ifname bond0 con-name bond0
$ nmcli c modify bond0 +bond.options miimon=100
$ nmcli -f bond.options c show bond0
bond.options: mode=balance-rr
Here we just added the option 'miimon=100', but it doesn't get saved in
because nm_settings_connection_set_connection() which is responsible for
actually updating the connection compares the new connection with old
one and if and only if the 2 are different the update is carried out.
The bug is triggered because when comparing, if default values are taken into
account, then having 'miimon=100' or not having it it's essentially the
same for compare(). While this doesn't cause a bond to have a wrong
setting when activated it's wrong from a user experience point of view
and thus must be fixed.
When this patch is applied, the above
commands will give the following results:
$ nmcli c add type bond ifname bond0 con-name bond0
$ nmcli c modify bond0 +bond.options miimon=100
$ nmcli -f bond.options c show bond0
bond.options: mode=balance-rr,miimon=100
Fix unit tests and also add a new case covering this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=1806549
Fix 'miimon' and 'arp_interval' validation, they can both be set indeed,
the kernel does not impose this limitation, nevertheless is sensible to
keep the defaults as previously (miimon=100, arp_interval=0).
Also add unit test.
Just looking at the hashtable entry of 'updelay' and 'downdelay' options
is wrong, we have to inspect their values to check if they're
actually enabled or not.
Otherwise bond connections with valid settings will fail
when created:
$ nmcli c add type bond ifname bond99 bond.options miimon=0,updelay=0,mode=0
Error: Failed to add 'bond-bond99' connection: bond.options: 'updelay' option requires 'miimon' option to be set
Also add unit tests.
https://bugzilla.redhat.com/show_bug.cgi?id=1805184
Fixes: d595f7843e ('libnm: add libnm/libnm-core (part 1)')
Keyfile support was initially added under GPL-2.0+ license as part of
core. It was moved to "libnm-core" in commit 59eb5312a5 ('keyfile: merge
branch 'th/libnm-keyfile-bgo744699'').
"libnm-core" is statically linked with by core and "libnm". In
the former case under terms of GPL-2.0+ (good) and in the latter case
under terms of LGPL-2.1+ (bad).
In fact, to this day, "libnm" doesn't actually use the code. The linker
will probably remove all the GPL-2.0+ symbols when compiled with
gc-sections or LTO. Still, linking them together in the first place
makes "libnm" only available under GPL code (despite the code
not actually being used).
Instead, move the GPL code to a separate static library
"shared/nm-keyfile/libnm-keyfile.la" and only link it to the part
that actually uses the code (and which is GPL licensed too).
This fixes the license violation.
Eventually, it would be very useful to be able to expose keyfile
handling via "libnm". However that is not straight forward due to the
licensing conflict.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/381
"nm-setting.c" (and property_to_dbus()) should stay independent of
actualy settings implementations. Instead, the property-info should
control the behavior.
What I like about this change is also that the generic handling is not a
flags "handle_secrets_for_vpn", but it just says to skip checking the
param-spec flags and directly call the to_dbus_fcn(). It's just a
generally useful thing to do, to let the to_dbus_fcn() function also
handle checking the property flags. The fact that only vpn.secrets
properties uses this for a certain pupose, is abstracted in a way that
makes sense.
In total, we register 447 property informations. Out of these,
326 are plain, GObject property based without special implementations.
The NMSettInfoProperty had all function pointers directly embedded,
currently this amounts to 5 function pointers and the "dbus_type" field.
That means, at runtime we have 326 times trivial implementations with
waste 326*6*8 bytes of NULL pointers. We can compact these by moving
them to a separate structure.
Before:
447 * 5 function pointers
447 * "dbus_type" pointer
= 2682 pointers
After:
447 * 1 pointers (for NMSettInfoProperty.property_type)
89 * 6 pointers (for the distinct NMSettInfoPropertType data)
= 981 pointers
So, in total this saves 13608 byes of runtime memory (on 64 bit arch).
The 89 NMSettInfoPropertType instances are the remaining distinct instances.
Note that every NMSettInfoProperty has a "property_type" pointer, but most of them are
shared. That is because the underlying type and the operations are the same.
Also nice is that the NMSettInfoPropertType are actually constant,
static fields and initialized very early.
This change also makes sense form a design point of view. Previously,
NMSettInfoProperty contained both per-property data (the "name") but
also the behavior. Now, the "behavioral" part is moved to a separate
structure (where it is also shared). That means, the parts that are
concerned with the type of the property (the behavior) are separate
from the actual data of the property.
Add test for checking the meta data for expected consistency.
This is also useful if you want to check something about the meta data
programatically.
For example, if you have the question which (if any) properties
are GObject based but also implement a to_dbus_fcn() function. Then you
can extend this code with some simple printf debugging to get a list of
those.
Or, if you want to find how many NMSettInfoProperty instances are in
static data (e.g. to determine how much memory is used). You can easily
modify this code to count them (and find 447 properties). Out of these,
326 are plain GObject based properties. Meaning, we could refactor the
code to create smaller NMSettInfoProperty instances for those, saving
thus (326 * 4 * sizeof (gpointer)) bytes (10K).
Such questions are interesting when refactoring the code.
It's important whether a setting is present or not. Keyfile writer
omits properties that have a default value, that means, if the setting
has all-default values, it would be dropped. For [proxy] that doesn't
really matter, because we tend to normalize it back. For some settings
it matters:
$ nmcli connection add type bluetooth con-name bt autoconnect no bluetooth.type dun bluetooth.bdaddr aa:bb:cc:dd:ee:ff gsm.apn a
Connection 'bt' (652cabd8-d350-4246-a6f3-3dc17eeb028f) successfully added.
$ nmcli connection modify bt gsm.apn ''
When storing this to keyfile, the [gsm] section was dropped
(server-side) and we fail an nm_assert() (omitted from the example
output below).
<error> [1566732645.9845] BUG: failure to normalized profile that we just wrote to disk: bluetooth: 'dun' connection requires 'gsm' or 'cdma' setting
<trace> [1566732645.9846] keyfile: commit: "/etc/NetworkManager/system-connections/bt.nmconnection": profile 652cabd8-d350-4246-a6f3-3dc17eeb028f (bt) written
<trace> [1566732645.9846] settings: update[652cabd8-d350-4246-a6f3-3dc17eeb028f]: update-from-dbus: update profile "bt"
<trace> [1566732645.9849] settings: storage[652cabd8-d350-4246-a6f3-3dc17eeb028f,3e504752a4a78fb3/keyfile]: change event with connection "bt" (file "/etc/NetworkManager/system-connections/>
<trace> [1566732645.9849] settings: update[652cabd8-d350-4246-a6f3-3dc17eeb028f]: updating connection "bt" (3e504752a4a78fb3/keyfile)
<debug> [1566732645.9857] ++ connection 'update connection' (0x7f7918003340/NMSimpleConnection/"bluetooth" < 0x55e1c52480e0/NMSimpleConnection/"bluetooth") [/org/freedesktop/NetworkManager>
<debug> [1566732645.9857] ++ gsm [ 0x55e1c5276f80 < 0x55e1c53205f0 ]
<debug> [1566732645.9858] ++ gsm.apn < 'a'
Of course, after reload the connection on disk is no loner valid.
Keyfile writer wrote an invalid setting.
# nmcli connection reload
Logfile:
<warn> [1566732775.4920] keyfile: load: "/etc/NetworkManager/system-connections/bt.nmconnection": failed to load connection: invalid connection: bluetooth: 'dun' connection requires 'gsm' or 'cdma' setting
...
<trace> [1566732775.5432] settings: update[652cabd8-d350-4246-a6f3-3dc17eeb028f]: delete connection "bt" (3e504752a4a78fb3/keyfile)
<debug> [1566732775.5434] Deleting secrets for connection /org/freedesktop/NetworkManager/Settings (bt)
<trace> [1566732775.5436] dbus-object[9a402fbe14c8d975]: unexport: "/org/freedesktop/NetworkManager/Settings/55"
Otherwise, it just looks odd:
"not priority 31265 from 0.0.0.0/0 fwmark 0xcb87 table 52103"
Better is:
"priority 31265 not from 0.0.0.0/0 fwmark 0xcb87 table 52103"
The "not" specifier should come after the priority. It makes more sense
to read it that way. As far as parsing the string is concerned, the
order does not matter. So this change in behavior is no problem.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/228
nmtst_get_rand_int() was originally named that way, because it
calls g_rand_int(). But I think if a function returns an uint32, it
should also be named that way.
Rename.
For each artifical team property we need to track whether it was
explicitly set (i.e., present in JSON/GVariant or set by the user
via NMSettingTeam/NMSettingTeamPort API).
--
As a plus, libnm is now no longer concerned with the underling default values
that teamd uses. For example, the effective default value for "notify_peers.count"
depends on the selected runner. But libnm does not need to care, it only cares
wheher the property is set in JSON or not. This also means that the default (e.g. as
interesting to `nmcli -o con show $PROFILE`) is independent from other properties
(like the runner).
Also change the default value for the GObject properties of
NMSettingTeam and NMSettingTeamPort to indicate the "unset" value.
For most properties, the default value is a special value that is
not a valid configuration itself.
For some properties the default value is itself a valid value, namely,
"runner.active", "runner.fast_rate", "port.sticky" and "port.prio".
As far as NMTeamSetting is concerned, it distinguishes between unset
value and set value (including the default value). That means,
when it parses a JSON or GVariant, it will remember whether the property
was present or not.
When using API of NMSettingTeam/NMSettingTeamPort to set a property to the
default value, it marks the property as unset. For example, setting
NM_SETTING_TEAM_RUNNER_ACTIVE to TRUE (the default), means that the
value will not be serialized to JSON/GVariant. For the above 4
properties (where the default value is itself a valid value) this is a
limitation of libnm API, as it does not allow to explicitly set
'"runner": { "active": true }'. See SET_FIELD_MODE_SET_UNLESS_DEFAULT,
Note that changing the default value for properties of NMSetting is problematic,
because it changes behavior for how settings are parsed from keyfile/GVariant.
For team settings that's not the case, because if a JSON "config" is
present, all other properties are ignore. Also, we serialize properties
to JSON/GVariant depending on whether it's marked as present, and not
whether the value is set to the default (_nm_team_settings_property_to_dbus()).
--
While at it, sticter validate the settings. Note that if a setting is
initialized from JSON, the strict validation is not not performed. That
means, such a setting will always validate, regardless whether the values
in JSON are invalid according to libnm. Only when using the extended
properties, strict validation is turned on.
Note that libnm serializes the properties to GVariant both as JSON "config"
and extended properties. Since when parsing a setting from GVariant will
prefer the "config" (if present), in most cases also validation is
performed.
Likewise, settings plugins (keyfile, ifcfg-rh) only persist the JSON
config to disk. When loading a setting from file, strict validation is
also not performed.
The stricter validation only happens if as last operation one of the
artificial properties was set, or if the setting was created from a
GVariant that has no "config" field.
--
This is a (another) change in behavior.
The order of the fields in the JSON object does not really matter.
Note that with the recent rework the order changed. Before it was
arbitrarily, now it still is arbitrary.
Reorder again, to follow the same order as `man teamd.conf`.
Completely refactor the team/JSON handling in libnm's NMSettingTeam and
NMSettingTeamPort.
- team handling was added as rh#1398925. The goal is to have a more
convenient way to set properties than constructing JSON. This requires
libnm to implement the hard task of parsing JSON (and exposing well-understood
properties) and generating JSON (based on these "artificial" properties).
But not only libnm. In particular nmcli and the D-Bus API must make this
"simpler" API accessible.
- since NMSettingTeam and NMSettingTeamPort are conceptually the same,
add "libnm-core/nm-team-utils.h" and NMTeamSetting that tries to
handle the similar code side-by-sdie.
The setting classes now just delegate for everything to NMTeamSetting.
- Previously, there was a very fuzzy understanding of the provided
JSON config. Tighten that up, when setting a JSON config it
regenerates/parses all other properties and tries to make the
best of it. When modifying any abstraction property, the entire
JSON config gets regenerated. In particular, don't try to merge
existing JSON config with the new fields. If the user uses the
abstraction API, then the entire JSON gets replaced.
For example note that nm_setting_team_add_link_watcher() would not
be reflected in the JSON config (a bug). That only accidentally worked
because client would serializing the changed link watcher to
GVariant/D-Bus, then NetworkManager would set it via g_object_set(),
which would renerate the JSON, and finally persist it to disk. But
as far as libnm is concerned, nm_setting_team_add_link_watcher() would
bring the settings instance in an inconsistent state where JSON and
the link watcher property disagree. Setting any property must
immediately update both the JSON and the abstraction API.
- when constucting a team setting from D-Bus, we would previously parse
both "config" and abstraction properties. That is wrong. Since our
settings plugins only support JSON, all information must be present
in the JSON config anyway. So, when "config" is present, only the JSON
must be parsed. In the best case, the other information is redudant and
contributes nothing. In the worse case, they information differs
(which might happen if the client version differs from the server
version). As the settings plugin only supports JSON, it's wrong to
consider redundant, differing information from D-Bus.
- we now only convert string to JSON or back when needed. Previously,
setting a property resulted in parsing several JSON multiple times
(per property). All operations should now scale well and be reasonably
efficient.
- also the property-changed signals are now handled correctly. Since
NMTeamSetting knows the current state of all attributes, it can emit
the exact property changed signals for what changed.
- we no longer use libjansson to generate the JSON. JSON is supposed
to be a machine readable exchange format, hence a major goal is
to be easily handled by applications. While parsing JSON is not so
trivial, writing a well-known set of values to JSON is.
The advantage is that when you build libnm without libjansson support,
then we still can convert the artificial properties to JSON.
- Requiring libjansson in libnm is a burden, because most of the time
it is not needed (as most users don't create team configurations). With
this change we only require it to parse the team settings (no longer to
write them). It should be reasonably simple to use a more minimalistic
JSON parser that is sufficient for us, so that we can get rid of the
libjansson dependency (for libnm). This also avoids the pain that we have
due to the symbol collision of libjansson and libjson-glib.
https://bugzilla.redhat.com/show_bug.cgi?id=1691619
We should not just disable tests with an #if.
Instead, mark them as skipped. This way, we still compile them, and we
even run them (showing a message why they are skipped).
- 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.
- 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.
Currently, nm_setting_wired_get_s390_option() returns the key
in an undefined order. Hence, the keyfile writer and the test
need to awkwardly sort the keys first. That will be solved better
in the next commit, when nm_setting_wired_get_s390_option() returns
the items sorted by key.
In some cases it is convenient to specify ranges of bridge vlans, as
already supported by iproute2 and natively by kernel. With this commit
it becomes possible to add a range in this way:
nmcli connection modify eth0-slave +bridge-port.vlans "100-200 untagged"
vlan ranges can't be PVIDs because only one PVID vlan can exist.
https://bugzilla.redhat.com/show_bug.cgi?id=1652910
(cherry picked from commit 7093515777)
Replace nm_utils_str_simpletokens_extract_next() by
nm_utils_escaped_tokens_split().
nm_utils_escaped_tokens_split() should become our first choice for
parsing and tokenizing.
Note that both nm_utils_str_simpletokens_extract_next() and
nm_utils_escaped_tokens_split() need to strdup the string once,
and tokenizing takes O(n). So, they are roughtly the same performance
wise. The only difference is, that as we iterate through the tokens,
we might abort early on error with nm_utils_str_simpletokens_extract_next()
and not parse the entire string. But that is a small benefit, since we
anyway always strdup() the string (being O(n) already).
Note that to-string will no longer escape ',' and ';'. This is a change
in behavior, of unreleased API. Also note, that escaping these is no
longer necessary, because nmcli soon will also use nm_utils_escaped_tokens_*().
Another change in behavior is that nm_utils_str_simpletokens_extract_next()
treated invalid escape sequences (backslashes followed by an arbitrary
character), buy stripping the backslash. nm_utils_escaped_tokens_*()
leaves such backslashes as is, and only honors them if they are followed
by a whitespace (the delimiter) or another backslash. The disadvantage
of the new approach is that backslashes are treated differently
depending on the following character. The benefit is, that most
backslashes can now be written verbatim, not requiring them to escape
them with a double-backslash.
Yes, there is a problem with these nested escape schemes:
- the caller may already need to escape backslash in shell.
- then nmcli will use backslash escaping to split the rules at ','.
- then nm_ip_routing_rule_from_string() will honor backslash escaping
for spaces.
- then iifname and oifname use backslash escaping for nm_utils_buf_utf8safe_escape()
to express non-UTF-8 characters (because interface names are not
necessarily UTF-8).
This is only redeamed because escaping is really only necessary for very
unusual cases, if you want to embed a backslash, a space, a comma, or a
non-UTF-8 character. But if you have to, now you will be able to express
that.
The other upside of these layers of escaping is that they become all
indendent from each other:
- shell can accept quoted/escaped arguments and will unescape them.
- nmcli can do the tokenizing for ',' (and escape the content
unconditionally when converting to string).
- nm_ip_routing_rule_from_string() can do its tokenizing without
special consideration of utf8safe escaping.
- NMIPRoutingRule takes iifname/oifname as-is and is not concerned
about nm_utils_buf_utf8safe_escape(). However, before configuring
the rule in kernel, this utf8safe escape will be unescaped to get
the interface name (which is non-UTF8 binary).
(cherry picked from commit b6d0be2d3b)
Add NMIPRoutingRule API with a few basic rule properties. More
properties will be added later as we want to support them.
Also, add to/from functions for string/GVariant representations.
These will be needed to persist/load/exchange rules.
The to-string format follows the `ip rule add` syntax, with the aim
to be partially compatible. Full compatibility is not possible though,
for various reasons (see code comment).
