The previous implementation of these macros simply relied on the
__VA_ARGS__ to be expended and joined with ','. That make that work
inside the switch statement, the macros expanded to
switch (val) {
(void) 0, (void) 0;
case 0x1:
s = " ""value" "";
break;
(void) 0, (void) 0;
};
Those NOP statements cause lgtm.com to complain "Dead code due to goto
or break statement".
Implement these macros differently using NM_VA_ARGS_JOIN().
We have variadic macros like NM_UTILS_ENUM2STR() that create a switch
statement. Their implementation relies on the way how __VA_ARGS__
gets expanded to a comma separated list. But that implementation is
not great. Let's instead add (and later use) NM_VA_ARGS_JOIN() which
can join variadic arguments by a configurable separator.
_NM_OBJECT_CLASS_INIT_FIELD_INFO() is a bit odd, because it defines a
static variable and initialized it at the moment when being "called".
This is in fact correct, because this code only gets called from inside
the _class_init() function, which is executed at most once.
Add an assertion to ensure that the static variables is not yet
initialized.
- always remember priv->last_state_dir that we received via
D-Bus. Only later, during get_config_path() we will check
whether the path is valid.
- remember in priv->warned_state_dir the full path for
which we warned. We want to print a warning for each
path once, if the path changes, then we also want a new
warning. A boolean flag cannot express that.
Otherwise we see a warning:
<warn> [1622790097.3601] config: unknown key firewall-backend in section [main] of file /etc/NetworkManager/NetworkManager.conf
Fixes: 1da1ad9c99 ('firewall: make firewall-backend configurable via "NetworkManager.conf"')
So far, we didn't verify the secondary connections at all.
But these really are supposed to be UUIDs.
As we now also normalize "connection.uuid" to be in a strict
format, the user might have profiles with non-normalized UUIDs.
In that case, the "connection.uuid" would be normalized, but
"connection.secondaries" no longer matches. We can fix that by
also normalizing "connection.secondaries". OK, this is not a very good
reason, because it's unlikely to affect any users in practice ('though
it's easy to reproduce).
A better reason is that the secondary setting really should be well
defined and verified. As we didn't do that so far, we cannot simply
outright reject invalid settings. What this patch does instead, is
silently changing the profile to only contain valid settings.
That has it's own problems, like that the user setting an invalid
value does not get an error nor the desired(?) outcome.
But of all the bad choices, normalizing seems the most sensible
one.
Note that in practice, most client applications don't rely on setting
arbitrary (invalid) "UUIDs". They simply expect to be able to set valid
UUIDs, which they still are. For example, nm-connection-editor presents
a drop down list of VPN profile, and nmcli also resolves connection IDs
to the UUID. That is, clients already have an intimate understanding of
this setting, and don't blindly set arbitrary values. Hence, this
normalization is unlikely to hit users in practice. But what it gives
is the guarantee that a verified connection only contains valid UUIDs.
Now all UUIDs will be normalized, invalid entries removed, and the list
made unique.
GSList requires an additional allocation for the container struct for each
element. Also, it does not have O(1) direct access. It's a pretty bad
data structure, especially if the underlying data is in form of a strv
array.
Use a GArray instead and the nm_strvarray_*() helpers.
For example for NM_SETTING_CONNECTION_SECONDARIES, the user can set
the GObject property to a string list that includes empty strings.
The C accessors (add/remove-by-value) should also accept any strings that
are accepted otherwise. Asserting against empty strings is wrong. If the
setting wants to reject empty strings, then it should use verify().
Since commit 207cf3d5d4 ('libnm: normalize "connection.uuid"') the
"connection.uuid" is normalized to be a valid UUID and all lower case.
That means, if we have .nmmeta files on disk with a previously valid,
but now invalid UUID, the meta file is no longer going to match.
Reject such file outright as invalid. If we really wanted to preserve
backward compatibility, then we would have to also normalize the
filename when we read it. However, that means, that suddenly we might
have any number of compatible .nmmeta files that normalize to the same
UUID, like the files
71088c75dec54119ab41be71bc10e736aaaabbbb.nmmeta
F95D40B4-578A-5E68-8597-39392249442B.nmmeta
f95d40b4-578a-5e68-8597-39392249442b.nmmeta
Having multiple places for the nmmeta file is complicated to handle.
Also, we often have the connection profile (and the normalized UUID)
first, and then check whether it has a .nmmeta file. If we would support
those unnormalized file names, we would have to visit all file names and
try to normalize it, to find those with a matching UUID.
Non-normalized UUIDs really should not be used and they already are not
working anymore for the .nmmeta file. This commit only outright rejects
them. This is a change in behavior, but the behavior change happened
earlier when we started normalizing "connection.uuid".
"uuid" is returned from nms_keyfile_nmmeta_check_filename(),
and contains "$UUID.nmmeta". We must compare only the first
"uuid_len" bytes.
Fixes: 064544cc07 ('settings: support storing "shadowed-storage" to .nmmeta files')
Most of the time, we care about whether we have a normalized UUID.
nm_uuid_is_valid_full() only exists for a particular case where we want
to use the function in a header, without including "nm-uuid.h". In that
case, we actually also care about normalized UUIDs.
If the TC setting contains no qdiscs and filters, it is lost after a
write-read cycle. Fix this by adding a new property to indicate the
presence of the (empty) setting.
NetworkManager supports a very limited set of qdiscs. If users want to
configure a unsupported qdisc, they need to do it outside of
NetworkManager using tc.
The problem is that NM also removes all qdiscs and filters during
activation if the connection doesn't contain a TC setting. Therefore,
setting TC configuration outside of NM is hard because users need to
do it *after* the connection is up (for example through a dispatcher
script).
Let NM consider the presence (or absence) of a TC setting in the
connection to determine whether NM should configure (or not) qdiscs
and filters on the interface. We already do something similar for
SR-IOV configuration.
Since new connections don't have the TC setting, the new behavior
(ignore existing configuration) will be the default. The impact of
this change in different scenarios is:
- the user previously configured TC settings via NM. This continues
to work as before;
- the user didn't set any qdiscs or filters in the connection, and
expected NM to clear them from the interface during activation.
Here there is a change in behavior, but it seems unlikely that
anybody relied on the old one;
- the user didn't care about qdiscs and filters; NM removed all
qdiscs upon activation, and so the default qdisc from kernel was
used. After this change, NM will not touch qdiscs and the default
qdisc will be used, as before;
- the user set a different qdisc via tc and NM cleared it during
activation. Now this will work as expected.
So, the new default behavior seems better than the previous one.
https://bugzilla.redhat.com/show_bug.cgi?id=1928078
Let's instead add a generic nm_device_get_ports() function.
Also, only adding new API is maybe not sufficient. We should
at the same time deprecate and alias the D-Bus API, like was done
for commit 067a3d6c08 ('nm-device: expose via D-Bus the 'hw-address'
property').
This reverts commit 754143f4e8.
lgtm.com doesn't like this:
Query pack:com.lgtm/cpp-queries
Query ID:cpp/duplicate-include-guard
Using the same include guard macro in more than one header file may
cause unexpected behavior from the compiler.
both for src/libnm-base/nm-ethtool-utils-base.h and
src/libnm-client-public/nm-ethtool-utils.h. But this is intentional,
because these two files are supposed to be identical (but compiled
twice, under different context).
Suppress the warning.
lgtm.com doesn't like this:
Query pack:com.lgtm/cpp-queries
Query ID:cpp/function-in-block
Functions should always be declared at file scope. It is confusing
to declare a function at block scope, and the visibility of the function
is not what would be expected.
