Changing "ipv4.route-table" and "ipv6.route-table" was not allowed
during reapply.
The main difficulty for supporting that is changing the sync-mode.
With route-table 0, we don't sync all tables but only the main table.
So, when reapply changes from full-sync to no-full-sync, it's slightly
more complicated.
But it's probably not too complicated either. The change from
no-full-sync to full-sync is simple: we just start doing a full-sync.
The reverse change is slightly more complicated, because we need to
do one last full-sync, to get rid of routes that we configured on those
other tables.
This doesn't make any difference in practice, but it seems more correct.
It would cause issues if we decided to remove an interface from the
signal handler.
When an interface (other OVS device types can not fail) encounters an error
it indicates it by changing the error column. Watch for those changes so
that we can eventually communicate them to the OVS factory to deal with
them.
Don't crash in situations, where the bridge or a port has a child with
UUID we don't know. This could happen if we mess up the parsing of
messages from OVSDB, but could also theoretically happen in OVSDB sends
us bad data.
The values cached in the device may be stale when we start a new
activation because in a disconnected state we might have called
ip_config_merge_and_apply() which cached the main table value.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
While at it, rename the "addr" field to "l_address". The term "addr" is
used over and over. Instead we should use distinct names that make it
easier to navigate the code.
NM_MODEM_OPERATOR_CODE property is construct-only. Add our common
code comment to the property setter.
Construct-only setters are pretty simple. They run before the object is
constructed, hence their scope is clearer. As such, there is no need to
emit property changed notifications (also because that is already taken
care by the GObject property setter). Don't call _nm_modem_set_operator_code(),
just directly set the property.
Usually we aim to have only one place where we set state (_nm_modem_set_operator_code()).
But a construct-only property setter is trivial enough that we can affort having two
places to modify the property. In particular, because the property setter does not "modify"
the property, it merely initializes it before the object is fully
constructed.
If the AddAndActivate() caller didn't explicitely a MAC address, default
to pinpointing the connection to the device by the means of an interface
name. This makes more sense than a MAC address with stable device names.
If the AddAndActivate() caller didn't explicitely a MAC address, default
to pinpointing the connection to the device by the means of an interface
name. This makes more sense than a MAC address with stable device names.
If the AddAndActivate() caller didn't explicitely a MAC address, default
to pinpointing the connection to the device by the means of an interface
name. This makes more sense than a MAC address with stable device names.
When changing the number of VFs the kernel can block for very long
time in the write() to sysfs, especially if autoprobe-drivers is
enabled. Turn the nm_platform_link_set_sriov_params() into an
asynchronous function.
Remove the call-id from the requests hash before invoking the callback.
This prevents the user to cancel the request from within the callback.
Supporting such a use case is not necessary so prevent it and tighten
the callers up.
A guint value can wrap, so we would need to check that we don't allocate duplicate
IDs (which we currently don't, and it's likely never to actually hit).
Just expose the (opaque) pointer of the call-id.
We still keep a "request_id", but that is only for logging purpose.
If we created a software interface it is because we already decided
that it should be managed, and so there is no point in waiting udev to
check that the interface is not udev-unmanaged.
We still wait udev for software interfaces created externally.
First of all, all file names in our source-tree should be unique. We should
not have stuff like "libnm-core/tests/test-general.c" and "src/tests/test-general.c".
The problem here are the C source files, and consequently also the test
binaries have duplicate names. We should avoid that in general. However,
our binaries should have a matching name with the C source. If
"test-general.c" is not good enough, that needs renaming. Not building
"platform-test-general" out of it.
On the other hand, all our tests should have a filename "*/tests/test-*", like
they do for autotools.
Rename the meson platform tests.
It's also important because "tools/run-nm-test.sh" relies on the test
name to workaround valgrind warnings.
- use GDBusConnection instead of GDBusProxy.
- rename "call-id" to "conf-id". It's really not a "call" but
configuration that gets added and NMPacrunnerManager ensures that
the configuration is send to pacrunner.
- let "conf-id" keep a reference to NMPacrunnerManager. For one,
when we remove configurations we need to call DestroyProxyConfiguration
to remove it again. We cannot just abort the requests but must linger
around until our configuration is properly cleaned up. Hence, we
anyway cannot destroy the NMPacrunnerManager earlier.
With respect to fixing shutdown not to leak anything, this merely
means that we must wait (and iterate the main loop) as long as
NMPacrunnerManager singleton still exits (that is anyway the plan
how to fix shutdown).
With these considerations it's also clear that our D-Bus calls must
have a stricter timeout: NM_SHUTDOWN_TIMEOUT_MS.
This is also nice because nm_pacrunner_manager_remove() no longer
needs a manager parameter, it can just rely on having a reference
to the manager.
- for logging the configuration IDs, don't log pointer values.
Logging pointer values should be avoided as it defeats ASLR.
Instead, give them a "log_id" number.
- pacrunner is a D-Bus activatable service. D-Bus activatable services
needs special care. We don't want to start it over and over again.
Instead, we only try to "StartServiceByName" if
- we have any configuration to add
- if pacrunner is currently confirmed not to be running (by watching
name owner changes)
- we didn't try to start it already. That means, only start it
at the beginning and afterwards set a flag to block it. When
we see pacrunner appear on D-Bus we always clear that flag,
that means if pacrunner drops of, we will try to restart it
(once).
Consider the situation in which ipv4.method=auto and there is an
address configured. Also, the DHCP timeout is long and there is no
DHCP server. If the link is brought down temporarily, the prefix route
for the static address is lost and not restored by NM because we
reapply the IP configuration only when the IP state is DONE.
The same can happen also for IPv6, but in that case also static IPv6
addresses are lost.
We should always reapply the IP configuration when the link goes up.
In general, all fields of public NMPlatform* structs must be
plain/simple. Meaning: copying the struct must be possible without
caring about cloning/duplicating memory.
In other words, if there are fields which lifetime is limited,
then these fields cannot be inside the public part NMPlatform*.
That is why
- "NMPlatformLink.kind", "NMPlatformQdisc.kind", "NMPlatformTfilter.kind"
are set by platform code to an interned string (g_intern_string())
that has a static lifetime.
- the "ingress_qos_map" field is inside the ref-counted struct NMPObjectLnkVlan
and not NMPlatformLnkVlan. This field requires managing the lifetime
of the array and NMPlatformLnkVlan cannot provide that.
See also for example NMPClass.cmd_obj_copy() which can deep-copy an object.
But this is only suitable for fields in NMPObject*. The purpose of this
rule is that you always can safely copy a NMPlatform* struct without
worrying about the ownership and lifetime of the fields (the field's
lifetime is unlimited).
This rule and managing of resource lifetime is the main reason for the
NMPlatform*/NMPObject* split. NMPlatform* structs simply have no mechanism
for copying/releasing fields, that is why the NMPObject* counterpart exists
(which is ref-counted and has a copy and destructor function).
This is violated in tc_commit() for the "kind" strings. The lifetime
of these strings is tied to the setting instance.
We cannot intern the strings (because these are arbitrary strings
and interned strings are leaked indefinitely). We also cannot g_strdup()
the strings, because NMPlatform* is not supposed to own strings.
So, just add comments that warn about this ugliness.
The more correct solution would be to move the "kind" fields inside
NMPObjectQdisc and NMPObjectTfilter, but that is a lot of extra effort.
While nm_platform_link_get_ifindex() is documented to return 0 if the device
is not found, don't rely on it. Instead, check that a valid(!) ifindex was
returned, and only then set the ifindex. Otherwise leave it at zero. There
is of course no difference in practice, but we generally treat invalid ifindexes
as <= 0, so it's not immediately clear what nm_platform_link_get_ifindex()
returns to signal no device.
Kernel calls the netlink attribute TCA_FQ_CODEL_MEMORY_LIMIT. Likewise,
iproute2 calls this "memory_limit".
Rename because TC parameters are inherrently tied to the kernel
implementation and we should use the familiar name.
iproute2 uses the special value ~0u to indicate not to set
TCA_FQ_CODEL_CE_THRESHOLD in RTM_NEWQDISC. When not explicitly
setting the value, kernel treats the threshold as disabled.
However note that 0xFFFFFFFFu is not an invalid threshold (as far as
kernel is concerned). Thus, we should not use that as value to indicate
that the value is unset. Note that iproute2 uses the special value ~0u
only internally thereby making it impossible to set the threshold to
0xFFFFFFFFu). But kernel does not have this limitation.
Maybe the cleanest way would be to add another field to NMPlatformQDisc:
guint32 ce_threshold;
bool ce_threshold_set:1;
that indicates whether the threshold is enable or not.
But note that kernel does:
static void codel_params_init(struct codel_params *params)
{
...
params->ce_threshold = CODEL_DISABLED_THRESHOLD;
static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
{
...
if (tb[TCA_FQ_CODEL_CE_THRESHOLD]) {
u64 val = nla_get_u32(tb[TCA_FQ_CODEL_CE_THRESHOLD]);
q->cparams.ce_threshold = (val * NSEC_PER_USEC) >> CODEL_SHIFT;
}
static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
{
...
if (q->cparams.ce_threshold != CODEL_DISABLED_THRESHOLD &&
nla_put_u32(skb, TCA_FQ_CODEL_CE_THRESHOLD,
codel_time_to_us(q->cparams.ce_threshold)))
goto nla_put_failure;
This means, kernel internally uses the special value 0x83126E97u to indicate
that the threshold is disabled (WTF). That is because
(((guint64) 0x83126E97u) * NSEC_PER_USEC) >> CODEL_SHIFT == CODEL_DISABLED_THRESHOLD
So in kernel API this value is reserved (and has a special meaning
to indicate that the threshold is disabled). So, instead of adding a
ce_threshold_set flag, use the same value that kernel anyway uses.
