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)')
Don't spread the validation for the interface name between multiple
places. There should be one place only, so when you search for how
this property gets verified, you can find the single place.
That requires to move the special handling for OVS interfaces to
NMSettingConnection.
Since we already have _nm_setting_ovs_interface_verify_interface_type(),
that is easy.
We should return the chosen type whenever we can verify the setting.
Previously, the normalized-type output argument was only set when
normalization was actually necessary.
On most cases, the caller cares whether the setting verifies and which
interface type is chosen. It's much less likely that a caller cares
only about the normalized-type if normalization is actually necessary.
Whenever we return TRUE (indicating that the setting is valid), also
return the chosen interface-type.
_nm_setting_ovs_interface_verify_interface_type() does verify and
normalize both. Especially for verify, it's useful to run the operation
without having a NMSettingOvsInterface instance, because we might
want to know how normalization would react, if we had a
NMSettingOvsInterface instance.
Allow for that.
The interface-name property has several deprecated aliases, like
"bridge.interface-name". For backward compatibility, we keep handling
them.
In particular, the "missing_from_dbus_fcn" handler is set. This handles
the case where GVariant only contains the deprecated form, but not
"connection.interface-name".
Previously, from_dbus_fcn() would check whether the deprecated form was
present, and -- only if that form was invalid -- prefer it. The idea was
to fail validation if the deprecated property was invalid.
I think that is not necessary. Just completely ignore the deprecated property,
if the new property is present.
What might make sense is to check whether the deprecated and the new
form are both present, that they are identical. However, I don't think
that is worth the effort.
There should not be multiple places to validate the interface-name.
The check in "nm-setting-infiniband.c" is unnecessary and wrong.
It's unnecessary, because _nm_connection_verify() takes care to
first verify the NMSettingConnection instance.
It's wrong, because it does not check the property the same way as
NMSettingConnection does (e.g. it does not check for valid UTF-8).
Fully sort the settings in _nm_connection_verify(). Previously, only the
NMSettingConnection instance was sorted first (as required). The remaining
settings were in undefined order. That means, we would validate settings
in undefined order, and if multiple settings have an issue, the reported
error would be undefined.
Instead, use nm_connection_get_settings() which fully sorts the settings
(and of course, sorts NMSettingConnection first as we require it).
Also, this way we no longer need to allocate multiple GSList instances
but only malloc() one array large enough to contain all settings.
Clang 10 doesn't like NM_IN_SET() with strings and is right about that:
../libnm-core/tests/test-general.c:7763:9: error: result of comparison against a string literal is unspecified (use an explicit string comparison function instead) [-Werror,-Wstring-compare]
(void) NM_IN_SET ("a", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16");
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
However, NM_IN_STRSET() should work.
verify() should validate options in a deterministic order, so that
the same profile (with same libnm version) gives the same failure
reason every time.
Hence, visit the options in sorted order, like we do for nm_setting_bond_get_option().
Internally, the options are tracked in a hash table and of undefined
sort order. However, nm_setting_bond_get_option() always returns a stable
(sorted) order.
Move "mode" as first, because that is usually the most interesting option.
The effect is:
$ nmcli -o connection show "$BOND_PROFILE"
...
-bond.options: arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,mode=balance-rr,use_carrier=0
+bond.options: mode=balance-rr,arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,use_carrier=0
This doesn't affect keyfile, which sorts the hash keys themself (and
doesn't treat the "mode" special).
This however does affect ifcfg-rh writer how it writes the BONDING_OPTS
variable. I think this change is fine and preferable.
strcmp() is hard to understand visually. Especially when different patterns
are mixed, like:
if ( !strcmp (name, NM_SETTING_BOND_OPTION_MIIMON)
&& strcmp (value, "0") != 0) {
quoting 'man ovs-vswitchd.conf.db':
"The name must be alphanumeric and must not contain forward or backward
slashes."
OVS actually accepts a wider range of chars (all printable UTF-8 chars),
NetworkManager restricts this to ASCII char as it's a safer option for
now since OVS is not well documented on this matter.
https://bugzilla.redhat.com/show_bug.cgi?id=1788432
Fixes: e7d72a14f6 ('libnm-core: use different ifname validation function for OVS bridges, ports and interfaces')
OVS bridges and ports do not have the length limitation of 15 bytes, the
only requirements are that all chars must be alphanumeric and not be
forward or backward slashes.
For OVS interfaces only 'patch' types do not have the length limit, all
the other types do (according to whether they have a corresponding
kernel link or not).
Add related unit test.
https://bugzilla.redhat.com/show_bug.cgi?id=1788432
'self' is guaranteed of being not-NULL since we have the
assertion 'g_return_val_if_fail (NM_IS_SETTING_OVS_INTERFACE (self),FALSE);'
at the beginning of the function.
nm_utils_is_valid_iface_name() is a public API of libnm-core, let's use
our internal API.
$ sed -i 's/\<nm_utils_is_valid_iface_name\>/nm_utils_ifname_valid_kernel/g' $(git grep -l nm_utils_is_valid_iface_name)
$ git shortlog -n -s a3e75f3294 -- libnm-core/nm-errors.h
11 Dan Winship
5 Thomas Haller
2 Beniamino Galvani
Note that the header also contains code that was copied from other
files. However, that code originates from libnm itself (and thus was
already LGPL licensed).
All contributors agreed to the relicensing according to "RELICENSE.md".
In all the cases, we don't want to perform locale dependent comparison.
$ sed -i 's/\<strcasecmp\>/g_ascii_\0/g' $(git grep -w -l strcasecmp -- ':(exclude)shared/systemd/' )
Building with GCC 10 gives the following error:
multiple definition of_nm_jansson_json_object_iter_key';
libnm/.libs/liblibnm.a(libnm_core_la-nm-json.o):/builddir/build/BUILD/NetworkManager-1.23.1/libnm-core/nm-json.c:24: first defined here /usr/bin/ld:
libnm/.libs/liblibnm.a(libnm_core_la-nm-team-utils.o):/usr/include/jansson.h:202: multiple definition of _nm_jansson_json_object_iter';
This happens because GCC 10 defaults to -fno-common and so multiple
definitions of the same global variable are not merged together.
_nm_jansson_json_* symbols are defined in nm-json.c as void pointers
and, due to the following macros in nm-json.h:
#define json_object_iter_next (*_nm_jansson_json_object_iter_next)
...
the function declaration in jansson.h:
void *json_object_iter_next(json_t *object, void *iter);
becomes a global variable as well:
void *(*_nm_jansson_json_object_iter_next)(json_t *object, void *iter);
So, the symbol is present in nm-json.o and all other object files that
include nm-json.h, and -fcommon is required. Without it, it would be
necessary to define the symbols only in one place (for example,
nm-json.c), but then static inline functions from the jannson.h header
would still refer to the original (missing) jansson functions.
For the moment, just use -fcommon.
We should use the same "is-valid" function everywhere.
Since nm_utils_ipaddr_valid() is part of libnm, it does not qualify.
Use nm_utils_ipaddr_is_valid() instead.
and _nm_utils_inet6_ntop() instead of nm_utils_inet6_ntop().
nm_utils_inet4_ntop()/nm_utils_inet6_ntop() are public API of libnm.
For one, that means they are only available in code that links with
libnm/libnm-core. But such basic helpers should be available everywhere.
Also, they accept NULL as destination buffers. We keep that behavior
for potential libnm users, but internally we never want to use the
static buffers. This patch needs to take care that there are no callers
of _nm_utils_inet[46]_ntop() that pass NULL buffers.
Also, _nm_utils_inet[46]_ntop() are inline functions and the compiler
can get rid of them.
We should consistently use the same variant of the helper. The only
downside is that the "good" name is already taken. The leading
underscore is rather ugly and inconsistent.
Also, with our internal variants we can use "static array indices in
function parameter declarations" next. Thereby the compiler helps
to ensure that the provided buffers are of the right size.
nmtst_main_context_iterate_until*() iterates until the condition is
satisfied. If that doesn't happen within timeout, it fails an assertion.
Rename the function to make that clearer.
Add VRF support to the daemon. When the device we are activating is a
VRF or a VRF's slave, put routes in the table specified by the VRF
connection.
Also, introduce a VRF device type in libnm.
