The comparison checking for MAC address equality had previously been flipped around.
Fixes: b084ad7f2b ('libnm-core: canonicalize hardware addresses in settings')
(cherry picked from commit 641e717797)
It's simply not valid to read the ref-count without an atomic.
The compiler might optimize out the assignment to "r" and read the
_ref_count field multiple times. Thereby, we might at first appear
to be larger than > 1, and later pass 1 to compare-and-exchange.
We need an atomic get here.
Fixes: 19d4027824 ('refstr: inline nm_ref_string_{ref,unref}()')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1847
(cherry picked from commit 5f7a027f59)
Older gcc versions don't like this. The _Pragam() itself is to workaround a
-Wnonnull-compare warning with gcc 14.
After all, we use compiler warning extensively. They are our linters and have
necessarily false positives. To make them usable across a wide range of
compilers, is a constant effort.
Here is another one.
The error:
./src/libnm-std-aux/nm-std-aux.h: In function ‘nm_utils_addr_family_other’:
./src/libnm-std-aux/nm-std-aux.h:230:36: error: expected expression before ‘#pragma’
230 | #define NM_PRAGMA_DIAGNOSTICS_PUSH _Pragma("GCC diagnostic push")
| ^~~~~~~
./src/libnm-std-aux/nm-std-aux.h:232:5: note: in expansion of macro ‘NM_PRAGMA_DIAGNOSTICS_PUSH’
232 | NM_PRAGMA_DIAGNOSTICS_PUSH _Pragma(_NM_PRAGMA_WARNING_DO(warning))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:291:9: note: in expansion of macro ‘NM_PRAGMA_WARNING_DISABLE’
291 | NM_PRAGMA_WARNING_DISABLE("-Wnonnull-compare"); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:953:9: note: in expansion of macro ‘nm_assert’
953 | nm_assert(true || NM_UNIQ_T(xx, uniq) == (x)); \
| ^~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:961:27: note: in expansion of macro ‘_NM_IN_SET’
961 | #define NM_IN_SET(x, ...) _NM_IN_SET(NM_UNIQ, ||, typeof(x), x, __VA_ARGS__)
| ^~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1493:15: note: in expansion of macro ‘NM_IN_SET’
1493 | nm_assert(NM_IN_SET((addr_family), NM_AF_INET, NM_AF_INET6))
| ^~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1502:9: note: in expansion of macro ‘nm_assert_addr_family’
1502 | nm_assert_addr_family(NM_UNIQ_T(_addr_family, uniq)); \
| ^~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1510:33: note: in expansion of macro ‘_NM_IS_IPv4’
1510 | #define NM_IS_IPv4(addr_family) _NM_IS_IPv4(NM_UNIQ, addr_family)
| ^~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1515:12: note: in expansion of macro ‘NM_IS_IPv4’
1515 | return NM_IS_IPv4(addr_family) ? NM_AF_INET6 : NM_AF_INET;
| ^~~~~~~~~~
Fixes: 62c1745f62 ('std-aux: suppress "-Wnonnull-compare" warning in nm_assert()')
(cherry picked from commit e4b154e1b0)
When doing reapply on linux bridge interface, NetworkManager will reset
the VLAN filtering and default PVID which cause PVID been readded to all
bridge ports regardless they are managed by NetworkManager.
This is because Linux kernel will re-add PVID to bridge port upon the
changes of bridge default-pvid value.
To fix the issue, this patch introduce netlink parsing code for
`vlan_filtering` and `default_pvid` of NMPlatformLnkBridge, and use that
to compare desired VLAN filtering settings, skip the reset of VLAN
filter if `default_pvid` and `vlan_filtering` are unchanged.
Signed-off-by: Gris Ge <fge@redhat.com>
(cherry picked from commit 02c34d538c)
GKeyfile changed something about how to handle invalid escape sequences.
As we don't want to test GKeyfile (per-se), just adjust to test to not
hit the problem.
This would fail with glib2-2.79.1-1.fc40:
# ./tools/run-nm-test.sh -m src/core/tests/config/test-config -p /config/set-values
TAP version 13
# random seed: R02Sb8afff1ec38ca5a1b7713e8c40eb4f56
# Start of config tests
# GLib-GIO-DEBUG: _g_io_module_get_default: Found default implementation local (GLocalVfs) for ?gio-vfs?
# (src/core/tests/config/test-config.c:1107) invalid value in config-data .intern.with-whitespace.key2 = (null) (instead of " b c\, d ")
./tools/run-nm-test.sh: line 307: 245847 Trace/breakpoint trap (core dumped) "${NMTST_DBUS_RUN_SESSION[@]}" "$TEST" "${TEST_ARGV[@]}"
exec "src/core/tests/config/test-config" failed with exit code 133
Upstream systemd fixed this compiler warning. What really needs to be
done, is re-importing the upstream code.
In the meantime, suppress the warning that hits on GCC 14.
This is a temporary workaround!
See-also: fdd84270df
When doing reapply on linux bridge interface, NetworkManager will reset
the VLAN filtering and default PVID which cause PVID been readded to all
bridge ports regardless they are managed by NetworkManager.
This is because Linux kernel will re-add PVID to bridge port upon the
changes of bridge default-pvid value.
To fix the issue, this patch introduce netlink parsing code for
`vlan_filtering` and `default_pvid` of NMPlatformLnkBridge, and use that
to compare desired VLAN filtering settings, skip the reset of VLAN
filter if `default_pvid` and `vlan_filtering` are unchanged.
Signed-off-by: Gris Ge <fge@redhat.com>
Older gcc versions don't like this. The _Pragam() itself is to workaround a
-Wnonnull-compare warning with gcc 14.
After all, we use compiler warning extensively. They are our linters and have
necessarily false positives. To make them usable across a wide range of
compilers, is a constant effort.
Here is another one.
The error:
./src/libnm-std-aux/nm-std-aux.h: In function ‘nm_utils_addr_family_other’:
./src/libnm-std-aux/nm-std-aux.h:230:36: error: expected expression before ‘#pragma’
230 | #define NM_PRAGMA_DIAGNOSTICS_PUSH _Pragma("GCC diagnostic push")
| ^~~~~~~
./src/libnm-std-aux/nm-std-aux.h:232:5: note: in expansion of macro ‘NM_PRAGMA_DIAGNOSTICS_PUSH’
232 | NM_PRAGMA_DIAGNOSTICS_PUSH _Pragma(_NM_PRAGMA_WARNING_DO(warning))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:291:9: note: in expansion of macro ‘NM_PRAGMA_WARNING_DISABLE’
291 | NM_PRAGMA_WARNING_DISABLE("-Wnonnull-compare"); \
| ^~~~~~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:953:9: note: in expansion of macro ‘nm_assert’
953 | nm_assert(true || NM_UNIQ_T(xx, uniq) == (x)); \
| ^~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:961:27: note: in expansion of macro ‘_NM_IN_SET’
961 | #define NM_IN_SET(x, ...) _NM_IN_SET(NM_UNIQ, ||, typeof(x), x, __VA_ARGS__)
| ^~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1493:15: note: in expansion of macro ‘NM_IN_SET’
1493 | nm_assert(NM_IN_SET((addr_family), NM_AF_INET, NM_AF_INET6))
| ^~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1502:9: note: in expansion of macro ‘nm_assert_addr_family’
1502 | nm_assert_addr_family(NM_UNIQ_T(_addr_family, uniq)); \
| ^~~~~~~~~~~~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1510:33: note: in expansion of macro ‘_NM_IS_IPv4’
1510 | #define NM_IS_IPv4(addr_family) _NM_IS_IPv4(NM_UNIQ, addr_family)
| ^~~~~~~~~~~
./src/libnm-std-aux/nm-std-aux.h:1515:12: note: in expansion of macro ‘NM_IS_IPv4’
1515 | return NM_IS_IPv4(addr_family) ? NM_AF_INET6 : NM_AF_INET;
| ^~~~~~~~~~
Fixes: 62c1745f62 ('std-aux: suppress "-Wnonnull-compare" warning in nm_assert()')
With a static array, we indicate that the argument must not be NULL.
Gcc-14.0.1-0.2.fc40 now warns against that:
CC src/libnm-base/libnm_base_la-nm-base.lo
In file included from ../src/libnm-std-aux/nm-default-std.h:102,
from ../src/libnm-glib-aux/nm-default-glib.h:11,
from ../src/libnm-glib-aux/nm-default-glib-i18n-lib.h:13,
from ../src/libnm-base/nm-base.c:3:
../src/libnm-base/nm-base.c: In function 'nm_net_devname_infiniband':
../src/libnm-std-aux/nm-std-aux.h:191:12: error: 'nonnull' argument 'name' compared to NULL [-Werror=nonnull-compare]
191 | if (expr) \
| ^
../src/libnm-std-aux/nm-std-aux.h:202:27: note: in expansion of macro '_NM_BOOLEAN_EXPR_IMPL'
202 | _NM_BOOLEAN_EXPR_IMPL(NM_UNIQ, expr))
| ^~~~~~~~~~~~~~~~~~~~~
../src/libnm-glib-aux/nm-macros-internal.h:1693:31: note: in expansion of macro 'NM_BOOLEAN_EXPR'
1693 | #define _G_BOOLEAN_EXPR(expr) NM_BOOLEAN_EXPR(expr)
| ^~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmacros.h:1244:43: note: in expansion of macro '_G_BOOLEAN_EXPR'
1244 | #define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR(expr), 1))
| ^~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmessages.h:656:9: note: in expansion of macro 'G_LIKELY'
656 | if (G_LIKELY (expr)) \
| ^~~~~~~~
../src/libnm-base/nm-base.c:57:5: note: in expansion of macro 'g_return_val_if_fail'
57 | g_return_val_if_fail(name, NULL);
| ^~~~~~~~~~~~~~~~~~~~
../src/libnm-core-impl/nm-setting-wireguard.c: In function '_nm_wireguard_peer_set_public_key_bin':
../src/libnm-core-impl/nm-setting-wireguard.c:316:8: error: 'nonnull' argument 'public_key' compared to NULL [-Werror=nonnull-compare]
316 | if (!public_key)
| ^
Convert these checks to an nm_assert() to suppress the warning.
(cherry picked from commit 7a031eef5d)
When we use a "static" array declarator to a function, we understand
and tell the compiler that the argument must not be NULL.
But now gcc-14.0.1-0.2.fc40 starts warning about NULL checks for
such arguments.
static void foo(char args[static 10]) {
nm_assert(args);
sprintf(args, "hi");
}
Granted, the compiler is right, and we know that this condition is not
supposed to be violated. A logical thing would be just to drop the
assertion.
Instead, suppress "-Wnonnull-compare" warnings inside a nm_assert(). An
nm_assert() is more than a run time check, it's an additional
self-documenting code of the invariants.
It's fine to assert for something that is true. Actually, all the
conditions that we assert against, hold. The compiler telling us that
the condition that we assert against is valid, is not useful.
(cherry picked from commit 62c1745f62)
Otherwise, gcc-14.0.1-0.2.fc40 warns:
../src/libnm-core-impl/nm-utils.c: In function _nm_utils_strstrdictkey_create:
../src/libnm-core-impl/nm-utils.c:5076:16: error: allocation of insufficient size '1' for type 'NMUtilsStrStrDictKey' {aka 'struct _NMUtilsStrStrDictKey'} with size '2' [-Werror=alloc-size]
5076 | return g_malloc0(1);
| ^~~~~~~~~~~~
(cherry picked from commit 63ab0d926d)
gcc-14.0.1-0.2.fc40 warns:
CC src/libnm-core-impl/libnm_core_impl_la-nm-setting-team.lo
../src/libnm-core-impl/nm-setting-team.c: In function nm_team_link_watcher_new_ethtool:
../src/libnm-core-impl/nm-setting-team.c:127:13: error: allocation of insufficient size 16 for type NMTeamLinkWatcher with size 48 [-Werror=alloc-size]
127 | watcher = g_malloc(nm_offsetofend(NMTeamLinkWatcher, ethtool));
| ^
(cherry picked from commit 5715feebe7)
With a static array, we indicate that the argument must not be NULL.
Gcc-14.0.1-0.2.fc40 now warns against that:
CC src/libnm-base/libnm_base_la-nm-base.lo
In file included from ../src/libnm-std-aux/nm-default-std.h:102,
from ../src/libnm-glib-aux/nm-default-glib.h:11,
from ../src/libnm-glib-aux/nm-default-glib-i18n-lib.h:13,
from ../src/libnm-base/nm-base.c:3:
../src/libnm-base/nm-base.c: In function 'nm_net_devname_infiniband':
../src/libnm-std-aux/nm-std-aux.h:191:12: error: 'nonnull' argument 'name' compared to NULL [-Werror=nonnull-compare]
191 | if (expr) \
| ^
../src/libnm-std-aux/nm-std-aux.h:202:27: note: in expansion of macro '_NM_BOOLEAN_EXPR_IMPL'
202 | _NM_BOOLEAN_EXPR_IMPL(NM_UNIQ, expr))
| ^~~~~~~~~~~~~~~~~~~~~
../src/libnm-glib-aux/nm-macros-internal.h:1693:31: note: in expansion of macro 'NM_BOOLEAN_EXPR'
1693 | #define _G_BOOLEAN_EXPR(expr) NM_BOOLEAN_EXPR(expr)
| ^~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmacros.h:1244:43: note: in expansion of macro '_G_BOOLEAN_EXPR'
1244 | #define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR(expr), 1))
| ^~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmessages.h:656:9: note: in expansion of macro 'G_LIKELY'
656 | if (G_LIKELY (expr)) \
| ^~~~~~~~
../src/libnm-base/nm-base.c:57:5: note: in expansion of macro 'g_return_val_if_fail'
57 | g_return_val_if_fail(name, NULL);
| ^~~~~~~~~~~~~~~~~~~~
../src/libnm-core-impl/nm-setting-wireguard.c: In function '_nm_wireguard_peer_set_public_key_bin':
../src/libnm-core-impl/nm-setting-wireguard.c:316:8: error: 'nonnull' argument 'public_key' compared to NULL [-Werror=nonnull-compare]
316 | if (!public_key)
| ^
Convert these checks to an nm_assert() to suppress the warning.
When we use a "static" array declarator to a function, we understand
and tell the compiler that the argument must not be NULL.
But now gcc-14.0.1-0.2.fc40 starts warning about NULL checks for
such arguments.
static void foo(char args[static 10]) {
nm_assert(args);
sprintf(args, "hi");
}
Granted, the compiler is right, and we know that this condition is not
supposed to be violated. A logical thing would be just to drop the
assertion.
Instead, suppress "-Wnonnull-compare" warnings inside a nm_assert(). An
nm_assert() is more than a run time check, it's an additional
self-documenting code of the invariants.
It's fine to assert for something that is true. Actually, all the
conditions that we assert against, hold. The compiler telling us that
the condition that we assert against is valid, is not useful.
Otherwise, gcc-14.0.1-0.2.fc40 warns:
../src/libnm-core-impl/nm-utils.c: In function _nm_utils_strstrdictkey_create:
../src/libnm-core-impl/nm-utils.c:5076:16: error: allocation of insufficient size '1' for type 'NMUtilsStrStrDictKey' {aka 'struct _NMUtilsStrStrDictKey'} with size '2' [-Werror=alloc-size]
5076 | return g_malloc0(1);
| ^~~~~~~~~~~~
gcc-14.0.1-0.2.fc40 warns:
CC src/libnm-core-impl/libnm_core_impl_la-nm-setting-team.lo
../src/libnm-core-impl/nm-setting-team.c: In function nm_team_link_watcher_new_ethtool:
../src/libnm-core-impl/nm-setting-team.c:127:13: error: allocation of insufficient size 16 for type NMTeamLinkWatcher with size 48 [-Werror=alloc-size]
127 | watcher = g_malloc(nm_offsetofend(NMTeamLinkWatcher, ethtool));
| ^
Section 4.9 of RFC 4594 specifies that DHCP should use the standard
(CS0 = 0) service class. Section 3.2 says that class CS6 is for
"transmitting packets between network devices (routers) that require
control (routing) information to be exchanged between nodes", listing
"OSPF, BGP, ISIS, RIP" as examples of such traffic. Furthermore, it
says that:
User traffic is not allowed to use this service class. By user
traffic, we mean packet flows that originate from user-controlled
end points that are connected to the network.
Indeed, we got reports of some Cisco switches dropping DHCP packets
because of the CS6 marking.
For these reasons, change the default value to the recommended one,
CS0.
(cherry picked from commit d8b33e2a97)
Currently the internal DHCP client sets traffic class "CS6" in the DS
field of the IP header for outgoing packets.
dhclient sets the field according to the definition of TOS (RFC 1349),
which was was deprecated in 1998 by RFC 2474 in favor of DSCP.
Introduce a new property IPvX.dhcp-dscp (currently valid only for
IPv4) to specify a custom DSCP value for DHCP backends that support it
(currently, only the internal one).
Define the default value to CS0, because:
- section 4.9 of RFC 4594 specifies that DHCP should use the standard
(CS0 = 0) service class;
- section 3.2 says that class CS6 is for "transmitting packets
between network devices (routers) that require control (routing)
information to be exchanged between nodes", listing "OSPF, BGP,
ISIS, RIP" as examples of such traffic. Furthermore, it says that:
User traffic is not allowed to use this service class. By user
traffic, we mean packet flows that originate from user-controlled
end points that are connected to the network.
- we got reports of some Cisco switches dropping DHCP packets because
of the CS6 marking.
(cherry picked from commit fcd907e062)
The client currently always sets the DSCP value in the DS field
(formerly known as "TOS") to CS6. Some network equipment drops packets
with such DSCP value; provide a way to change it.
(cherry picked from commit 2f543f1154)
Section 4.9 of RFC 4594 specifies that DHCP should use the standard
(CS0 = 0) service class. Section 3.2 says that class CS6 is for
"transmitting packets between network devices (routers) that require
control (routing) information to be exchanged between nodes", listing
"OSPF, BGP, ISIS, RIP" as examples of such traffic. Furthermore, it
says that:
User traffic is not allowed to use this service class. By user
traffic, we mean packet flows that originate from user-controlled
end points that are connected to the network.
Indeed, we got reports of some Cisco switches dropping DHCP packets
because of the CS6 marking.
For these reasons, change the default value to the recommended one,
CS0.
Currently the internal DHCP client sets traffic class "CS6" in the DS
field of the IP header for outgoing packets.
dhclient sets the field according to the definition of TOS (RFC 1349),
which was was deprecated in 1998 by RFC 2474 in favor of DSCP.
Introduce a new property IPvX.dhcp-dscp (currently valid only for
IPv4) to specify a custom DSCP value for DHCP backends that support it
(currently, only the internal one).
Define the default value to CS0, because:
- section 4.9 of RFC 4594 specifies that DHCP should use the standard
(CS0 = 0) service class;
- section 3.2 says that class CS6 is for "transmitting packets
between network devices (routers) that require control (routing)
information to be exchanged between nodes", listing "OSPF, BGP,
ISIS, RIP" as examples of such traffic. Furthermore, it says that:
User traffic is not allowed to use this service class. By user
traffic, we mean packet flows that originate from user-controlled
end points that are connected to the network.
- we got reports of some Cisco switches dropping DHCP packets because
of the CS6 marking.
The client currently always sets the DSCP value in the DS field
(formerly known as "TOS") to CS6. Some network equipment drops packets
with such DSCP value; provide a way to change it.
