| Commit message (Collapse) | Author | Age | Files | Lines |
|
|
|
| |
s/grater/greater/
|
|
|
|
|
|
|
|
|
|
|
|
| |
When the master AC becomes ready, activate_stage1_device_prepare() is
called in a idle handler. If the master AC fails in the meantime, it
will change state to deactivating or deactivated. We must check for
that condition before proceeding with slave activation. Note the the
'master_ready' flag of an AC is never cleared after it is set.
Fixes: 5b677d5a3bed ('device: move check for master from nm_device_activate_schedule_stage2_device_config() to end of stage1')
https://bugzilla.redhat.com/show_bug.cgi?id=1747998
|
|
|
|
| |
...to aid readability.
|
|
|
|
|
|
|
|
|
| |
We're dereferencing the info pointer in the argument list in the call to
nm_client_activate_connection_async(). Stealing it at that point causes
a crash.
This reverts a chunk of commit b298f2e6058a ('cli: use cleanup macro for
freeing AddAndActivateInfo').
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
failure
We want to print the [wireguard] section before printing sections of the
peers. It just looks nicer.
This also fixes a test failure:
/libnm/settings/roundtrip-conversion/wireguard/2: **
test:ERROR:./shared/nm-utils/nm-test-utils.h:2254:nmtst_keyfile_assert_data: assertion failed (d1 == data): ("[connection]\nid=roundtrip-conversion-2\nuuid=63376701-b61e-4318-bf7e-664a1c1eeaab\ntype=wireguard\ninterface-name=ifname2\npermissions=\n\n[wireguard-peer.uoGoXWWRxJvu4jDva8pPGA4nxau8B33S+YR+MfPFjxc=]\nendpoint=192.168.255.180:30429\npreshared-key-flags=2\n\n[wireguard-peer.BED73rH9j3OCHYAeXNrW5y5oia/Ngj+M04e9sG7DQOo=]\nendpoint=192.168.188.253:30407\npreshared-key-flags=1\npersistent-keepalive=5070\nallowed-ips=192.168.215.179/32;192.168.120.249/32;a:b:c::e4:13/128;192.168.157.84/32;a:b:c::1b:df/128;a:b:c::b0:84/128;192.168.168.17/32;\n\n[wireguard]\n\n[ipv4]\ndns-search=\nmethod=disabled\n\n[ipv6]\naddr-gen-mode=stable-privacy\ndns-search=\nmethod=ignore\n\n[proxy]\n" == "[connection]\nid=roundtrip-conversion-2\nuuid=63376701-b61e-4318-bf7e-664a1c1eeaab\ntype=wireguard\ninterface-name=ifname2\npermissions=\n\n[wireguard]\n\n[wireguard-peer.uoGoXWWRxJvu4jDva8pPGA4nxau8B33S+YR+MfPFjxc=]\nendpoint=192.168.255.180:30429\npreshared-key-flags=2\n\n[wireguard-peer.BED73rH9j3OCHYAeXNrW5y5oia/Ngj+M04e9sG7DQOo=]\nendpoint=192.168.188.253:30407\npreshared-key-flags=1\npersistent-keepalive=5070\nallowed-ips=192.168.215.179/32;192.168.120.249/32;a:b:c::e4:13/128;192.168.157.84/32;a:b:c::1b:df/128;a:b:c::b0:84/128;192.168.168.17/32;\n\n[ipv4]\ndns-search=\nmethod=disabled\n\n[ipv6]\naddr-gen-mode=stable-privacy\ndns-search=\nmethod=ignore\n\n[proxy]\n")
Fixes: ddd148e02b6b ('keyfile: let keyfile writer serialize setting with all default values')
|
|\
| |
| |
| |
| |
| | |
https://bugzilla.redhat.com/show_bug.cgi?id=1734470
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/247
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
With accept_ra set to 1, kernel sends its own router solicitation
messages and parses the advertisements. This duplicates what NM
already does in userspace and has unwanted consequences like [1] and
[2].
The only reason why accept_ra was re-enabled in the past was to apply
RA parameters like ReachableTime and RetransTimer [3]; but now NM
supports them and so accept_ra can be turned off again.
Also, note that previously the option was set in
addrconf6_start_with_link_ready(), and so this was done only when the
method was 'auto'. Instead, now we clear it for all methods except
'ignore'.
[1] https://mail.gnome.org/archives/networkmanager-list/2019-June/msg00027.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1734470
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1068673
|
|/
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
IPv6 router advertisement messages contain the following parameters
(RFC 4861):
- Reachable time: 32-bit unsigned integer. The time, in
milliseconds, that a node assumes a neighbor is reachable after
having received a reachability confirmation. Used by the Neighbor
Unreachability Detection algorithm. A value of zero means
unspecified (by this router).
- Retrans Timer: 32-bit unsigned integer. The time, in milliseconds,
between retransmitted Neighbor Solicitation messages. Used by
address resolution and the Neighbor Unreachability Detection
algorithm. A value of zero means unspecified (by this router).
Currently NM ignores them; however, since it leaves accept_ra=1, the
kernel parses RAs and applies those parameters for us [1].
In the next commit kernel handling of RAs will be disabled, so let NM
set those neighbor-related parameters.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ipv6/ndisc.c?h=v5.2#n1353
|
|
|
|
| |
The 'nettools' client doesn't support IPv6, fall back to 'internal'.
|
|\
| |
| |
| | |
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/245
|
| |
| |
| |
| |
| | |
We know we are ready and in a situation where we can handle state changes.
Don't schedule stage2 in an idle handler, just invoke it directly.
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
nm_device_activate_schedule_stage2_device_config() to end of stage1
Note that by now no callers of nm_device_activate_schedule_stage2_device_config()
are left. All previous callers now re-schedule stage1 instead of directly
scheduling stage2.
Note that if stage2 later also gets re-factored to re-enter itself
instead of scheduling stage3 right away, the function will be used
again.
That means, we can move the check for the master where it belongs: as
part (and at the end of) stage1.
Also, slightly simplify the code. The handler master_ready_cb()
no longer directly calls master_ready(). It's enough to always
enter stage1 again.
Also drop master_ready_handled. We don't need to remember that this
condition was satsified. We can just check it always when we reach
the place in activate_stage1_device_prepare().
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
I am about to change the when stage1 gets postponed, then the way to
proceed it is to schedule stage1 again (instead of scheduling stage2).
The reason is that stage1 handling should be reentrant and we should
keep entering it until there is no more reason to postpone it. If
a subclass postpones stage1 and then later progresses it by directly
scheduling stage2, then only the subclass is in control over postponing
stage 2.
Instead, anybody should be able to delay stage2 independently. That can
only work if everybody signals readyness to proceed by scheduling stage1
again.
|
| |
| |
| |
| | |
It has only one caller. It's clearer to do the cleanup right there.
|
| |
| |
| |
| |
| | |
We don't need this. The applied-connection is already remembered
and suitable.
|
| | |
|
| | |
|
| |
| |
| |
| |
| |
| |
| | |
for NMDeviceEthernet
There is a small change in the order of actions. Now we set the MAC address before
calling link_negotiation_set(). That shouldn't make a difference.
|
| | |
|
| |
| |
| |
| |
| |
| | |
NMDevice
This is so common, that NMDevice can handle it for us.
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
NMDevice's act_stage1_prepare() now does nothing. Calling it is not
useful and has no effect.
In general, when a subclass overwrites a virtual function, it must be
defined whether the subclass must, may or must-not call the parents
implementation. Likewise, it must be clear when the parents
implementation should be chained: first, as last, does it matter?
In any case, that very much depends on how the parent is implemented
and this can only be solved by documentation and common conventions.
It's a forgiving approach to have a parents implementation do nothing,
then the subclass may call it at any time (or not call it at all).
This is especially useful if classes don't know their parent class well.
But in NetworkManager code the relationship between classes are known
at compile time, so every of these classes knows it derives directly
from NMDevice.
This forgingin approach was what NMDevice's act_stage1_prepare() was doing.
However, it also adds lines of code resulting in a different kind of complexity.
So, it's not clear that this forgiving approach is really better. Note
that it also has a (tiny) runtime and code-size overhead.
Change the expectation of how NMDevice's act_stage1_prepare() should be
called: it is no longer implemented, and subclasses *MUST* not chain up.
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
Note that all subclasses of NMDevice that implement act_stage1_prepare(), call
the parents implementation as very first thing.
Previously, NMDevice's act_stage1_prepare() was setting up SR-IOV. But that is
problemantic. Note that it may have returned NM_ACT_STAGE_RETURN_POSTPONE, in which
case subclasses would just skip their action and return to the caller. Later,
sriov_params_cb() would directly call nm_device_activate_schedule_stage2_device_config(),
and thus act_stage1_prepare() was never executed for the subclass. That
is wrong.
First, I don't think it is good to let subclasses decide whether to call a
parents implementation (and when). It just causes ambiguity. In
the best case we do it always in the same order, in the worst case we
call the parent at the wrong time or never. Instead, we want to initialize
SR-IOV always and early in stage1, so we should just do it directly from
activate_stage1_device_prepare(). Now NMDevice's act_stage1_prepare() does
nothing.
The bigger problem is that when a device wants to resume a stage that
it previously postponed, that it would schedule the next stage!
Instead, it should schedule the same stage again instead. That allows
to postpone the completion of a stage for multiple reasons, and each
call to a certain stage merely notifies that something changed and
we need to check whether we can now complete the stage.
For that to work, stages must become re-entrant. That means we need to
remember whether an action that we care about needs to be started, is pending
or already completed.
Compare this to nm_device_activate_schedule_stage3_ip_config_start(),
which checks whether firewall is configured. That is likewise the wrong
approach. Callers that were in stage2 and postponed stage2, and later would
schedule stage3 when they are ready.
Then nm_device_activate_schedule_stage3_ip_config_start() would check whether
firewall is also ready, and do nothing if that's not the case (relying
that when the firewall code completes to call
nm_device_activate_schedule_stage3_ip_config_start().
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
- use a [2] array for IPv4/IPv6 variants and a IS_IPv4 variable,
like we do for other places that have similar implementations for
both address families.
- drop ActivationHandleData and use the fields directly. Also drop
activation_source_get_by_family().
- rename "act_handle*" field to "activation_source_*", to follow the
naming of the related accessor functions.
- downgrade the severity of some logging messages.
|
| |
| |
| |
| |
| |
| | |
"nm-device-{ethernet,macvlan}.c"
This variable is commonly called "device", not "dev". Rename.
|
| | |
|
| |
| |
| |
| | |
Restructure code to return early and free resources with nm_auto.
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
The only change in behavior is in act_stage1_prepare().
That changes compared to before that we also set the specific
object path if it was already set (and we looked up the AP by
specific object to start with).
Also, for existing APs that we found with nm_wifi_aps_find_first_compatible(),
it changes the order of calling set_current_ap() before nm_active_connection_set_specific_object().
That should not make a different though. I anyway wonder why we even bother to
set the specific object on the AC. Maybe that should be revisited.
|
| |
| |
| |
| |
| | |
Don't clear and reschedule finding of p2p peer if called multiple
times during (the same) activation.
|
| | |
|
| |
| |
| |
| |
| | |
act_stage1_prepare() should become re-entrant. That means, we should not clear the state
there. Instead, we clear it where necessary or on deactivate (which we do already).
|
| |
| |
| |
| | |
minor cleanups
|
| |
| |
| |
| |
| | |
Only reset "vlan_configured" when deactivating. stage1() should be
re-entrant.
|
| |
| |
| |
| | |
act_stage1_prepare()
|
| | |
|
| |
| |
| |
| |
| |
| | |
NM_DEVICE_MODEM_GET_PRIVATE() is based on _NM_GET_PRIVATE(), which has
some smarts to check the pointer type, but is fine with well-known parent
pointer types like "NMDevice *".
|
|/
|
|
|
| |
NMDeviceClass already has a function with this name. It's confusing
to have multiple virtual functions named the same. Rename.
|
|
|
|
|
|
|
| |
If the @append_force argument is set and the object is already in the
list, it must be moved at the end.
Fixes: 22edeb5b691b ('core: track addresses for NMIP4Config/NMIP6Config via NMDedupMultiIndex')
|
|
|
|
|
|
|
|
| |
Add test to show a wrong result of ip_ipX_config_replace() due to a
bug in _nm_ip_config_add_obj(). When an address is added to the tail
of the index and another address with the same id already exists, the
existing object is left at the same place, breaking the order of
addresses.
|
|
|
|
|
|
|
|
| |
Only happens with debug logging enabled. So, not a large problem.
Found by Coverity.
Fixes: d9a4b59c18e3 ('acd: adapt NM code and build options')
|
|
|
|
|
|
|
|
|
|
|
| |
Nowadays, we should prefer "/run" over "/var/run". When not specifying
during ./configure, autotools however still defaults to "/var/run".
This default is also visible in the pre-generated documenation, for
example `man NetworkManager.conf` says
Unless the symlink points to the internal file /run/NetworkManager/resolv.conf,
in which case the ...
|
|\
| |
| |
| | |
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/248
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
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"
|
| |
| |
| |
| |
| |
| |
| | |
I thought I would need this, but ended up not using it.
Anyway, it makes sense in general that the function can lookup
all relevant information, so merge it.
|
| |
| |
| |
| | |
settings that are all-default
|
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
disk and fail
First of all, keyfile writer (and reader) are supposed to be able to store
every profile to disk and re-read a valid profile back. Note that the profile
might be modified in the process, for example, blob certificates are written
to a file. So, the result might no be exactly the same, but it must still be
valid (and should only diverge in expected ways from the original, like mangled
certificates).
Previously, we would re-read the profile after writing to disk. If that failed,
we would only fail an assertion but otherwise proceeed. It is a bug
after all. However, it's bad to check only after writing to file,
because it results in a unreadable profile on disk, and in the first
moment it appears that noting went wrong. Instead, we should fail early.
Note that nms_keyfile_reader_from_keyfile() must entirely operate on the in-memory
representation of the keyfile. It must not actually access any files on disk. Hence,
moving this check before writing the profile must work. Otherwise, that would be
a separate bug. Actually, keyfile reader and writer violate this. I
added FIXME comments for that. But it doesn't interfere with this
patch.
|
| |
| |
| |
| | |
It's a bug either way, but let's log what exactly went wrong.
|
|/ |
|
| |
|
| |
|
|
|
|
| |
Newer systemd rejects messages without one.
|