"nm-setting.c" (and property_to_dbus()) should stay independent of
actualy settings implementations. Instead, the property-info should
control the behavior.
What I like about this change is also that the generic handling is not a
flags "handle_secrets_for_vpn", but it just says to skip checking the
param-spec flags and directly call the to_dbus_fcn(). It's just a
generally useful thing to do, to let the to_dbus_fcn() function also
handle checking the property flags. The fact that only vpn.secrets
properties uses this for a certain pupose, is abstracted in a way that
makes sense.
The idea was that properties that are implemented via GENDATA still
could have a GObject property. As such, they would be marked with
this flag.
Currently, gendata properties are only implemented by NMSettingEthtool,
and there are no GObject properties where this flag is used. While it
might make sense in theory or in the future, it is unused.
Drop it.
We use the "properties_override" GArray to construct the list of property infos.
But as we append values to the GArray, the buffer grows exponentially and likely
is larger than the actually used number of values.
As this data is kept until the end of the program, let's not waste the over-allocated
memory and instead copy it to a buffer of the right size.
We have too many _properties_override_add*() variants. They basically are all the
same. Drop _properties_override_add_dbus_only() and use _properties_override_add_virt()
instead.
Also, I am always confused by the term "synth". We shouldn't treat
non-GObject-based properties as somehow odd that need to be synthesized.
In total, we register 447 property informations. Out of these,
326 are plain, GObject property based without special implementations.
The NMSettInfoProperty had all function pointers directly embedded,
currently this amounts to 5 function pointers and the "dbus_type" field.
That means, at runtime we have 326 times trivial implementations with
waste 326*6*8 bytes of NULL pointers. We can compact these by moving
them to a separate structure.
Before:
447 * 5 function pointers
447 * "dbus_type" pointer
= 2682 pointers
After:
447 * 1 pointers (for NMSettInfoProperty.property_type)
89 * 6 pointers (for the distinct NMSettInfoPropertType data)
= 981 pointers
So, in total this saves 13608 byes of runtime memory (on 64 bit arch).
The 89 NMSettInfoPropertType instances are the remaining distinct instances.
Note that every NMSettInfoProperty has a "property_type" pointer, but most of them are
shared. That is because the underlying type and the operations are the same.
Also nice is that the NMSettInfoPropertType are actually constant,
static fields and initialized very early.
This change also makes sense form a design point of view. Previously,
NMSettInfoProperty contained both per-property data (the "name") but
also the behavior. Now, the "behavioral" part is moved to a separate
structure (where it is also shared). That means, the parts that are
concerned with the type of the property (the behavior) are separate
from the actual data of the property.
property_to_dbus() returns NULL when called with
NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED and the property is
not an agent-owned secrets. The function doesn't handle VPN secrets
correctly, since they are all stored as a hash in the vpn.secrets
property and the flag for each of them is a matching '*-flags' key in
the vpn.data property. VPN secrets must be handled differently; do it
in the VPN setting to_dbus_fcn() function.
Fixes: 71928a3e5c ('settings: avoid cloning the connection to maintain agent-owned secrets')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/230https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/280
This is a complete refactoring of the bluetooth code.
Now that BlueZ 4 support was dropped, the separation of NMBluezManager
and NMBluez5Manager makes no sense. They should be merged.
At that point, notice that BlueZ 5's D-Bus API is fully centered around
D-Bus's ObjectManager interface. Using that interface, we basically only
call GetManagedObjects() once and register to InterfacesAdded,
InterfacesRemoved and PropertiesChanged signals. There is no need to
fetch individual properties ever.
Note how NMBluezDevice used to query the D-Bus properties itself by
creating a GDBusProxy. This is redundant, because when using the ObjectManager
interfaces, we have all information already.
Instead, let NMBluezManager basically become the client-side cache of
all of BlueZ's ObjectManager interface. NMBluezDevice was mostly concerned
about caching the D-Bus interface's state, tracking suitable profiles
(pan_connection), and moderate between bluez and NMDeviceBt.
These tasks don't get simpler by moving them to a seprate file. Let them
also be handled by NMBluezManager.
I mean, just look how it was previously: NMBluez5Manager registers to
ObjectManager interface and sees a device appearing. It creates a
NMBluezDevice object and registers to its "initialized" and
"notify:usable" signal. In the meantime, NMBluezDevice fetches the
relevant information from D-Bus (although it was already present in the
data provided by the ObjectManager) and eventually emits these usable
and initialized signals.
Then, NMBlue5Manager emits a "bdaddr-added" signal, for which NMBluezManager
creates the NMDeviceBt instance. NMBluezManager, NMBluez5Manager and
NMBluezDevice are strongly cooperating to the point that it is simpler
to merge them.
This is not mere refactoring. This patch aims to make everything
asynchronously and always cancellable. Also, it aims to fix races
and inconsistencies of the state.
- Registering to a NAP server now waits for the response and delays
activation of the NMDeviceBridge accordingly.
- For NAP connections we now watch the bnep0 interface in platform, and tear
down the device when it goes away. Bluez doesn't send us a notification
on D-Bus in that case.
- Rework establishing a DUN connection. It no longer uses blocking
connect() and does not block until rfcomm device appears. It's
all async now. It also watches the rfcomm file descriptor for
POLLERR/POLLHUP to notice disconnect.
- drop nm_device_factory_emit_component_added() and instead let
NMDeviceBt directly register to the WWan factory's "added" signal.
Add test for checking the meta data for expected consistency.
This is also useful if you want to check something about the meta data
programatically.
For example, if you have the question which (if any) properties
are GObject based but also implement a to_dbus_fcn() function. Then you
can extend this code with some simple printf debugging to get a list of
those.
Or, if you want to find how many NMSettInfoProperty instances are in
static data (e.g. to determine how much memory is used). You can easily
modify this code to count them (and find 447 properties). Out of these,
326 are plain GObject based properties. Meaning, we could refactor the
code to create smaller NMSettInfoProperty instances for those, saving
thus (326 * 4 * sizeof (gpointer)) bytes (10K).
Such questions are interesting when refactoring the code.
NetworkManager treats "gsm.apn" %NULL as setting an empty APN ("").
At least with ModemManager. With oFono, a %NULL APN means not to set
the "AccessPointName", so oFono implementation treats %NULL different
from "".
Soon the meaning will change to allow %NULL to automatically
obtain the APN from the mobile-broadband-provider-info. That will be a
change in behavior how to treat %NULL.
Anyway, since %NULL is accepted and in fact means to actually use "",
the empty word should be also accepted to explicitly choose this
behavior. This is especially important in combination with changing the
meaning of %NULL.
This will make NetworkManager look up APN, username, and password in the
Mobile Broadband Provider database.
It is mutually exclusive with the apn, username and password properties.
If that is the case, the connection will be normalized to
auto-config=false. This makes it convenient for the user to turn off the
automatism by just setting the apn.
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🅱️c::e4:13/128;192.168.157.84/32;a🅱️c::1b:df/128;a🅱️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🅱️c::e4:13/128;192.168.157.84/32;a🅱️c::1b:df/128;a🅱️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: ddd148e02b ('keyfile: let keyfile writer serialize setting with all default values')
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.
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.
NM didn't support wpa-none for years because kernel drivers used to be
broken. Note that it wasn't even possible to *add* a connection with
wpa-none because it was rejected in nm_settings_add_connection_dbus().
Given that wpa-none is also deprecated in wpa_supplicant and is
considered insecure, drop altogether any reference to it.
Up until now, a default-route (with prefix length zero) could not
be configured directly. The user could only set ipv4.gateway,
ipv4.never-default, ipv4.route-metric and ipv4.route-table to influence
the setting of the default-route (respectively for IPv6).
That is a problematic limitation. For one, whether a route has prefix
length zero or non-zero does not make a fundamental difference. Also,
it makes it impossible to configure all the routing attributes that one can
configure otherwise for static routes. For example, the default-route could
not be configured as "onlink", could not have a special MTU, nor could it be
placed in a dedicated routing table.
Fix that by lifting the restriction. Note that "ipv4.never-default" does
not apply to /0 manual routes. Likewise, the previous manners of
configuring default-routes ("ipv4.gateway") don't conflict with manual
default-routes.
Server-side this all the pieces are already in place to accept a default-route
as static routes. This was done by earlier commits like 5c299454b4
('core: rework tracking of gateway/default-route in ip-config').
A long time ago, NMIPRoute would assert that the prefix length is
positive. That was relaxed by commit a2e93f2de4 ('libnm: allow zero
prefix length for NMIPRoute'), already before 1.0.0. Using libnm from
before 1.0.0 would result in assertion failures.
Note that the default-route-metric-penalty based on connectivity
checking applies to all /0 routes, even these static routes. Be they
added due to DHCP, "ipv4.gateway", "ipv4.routes" or "wireguard.peer-routes".
I wonder whether doing that unconditionally is desirable, and maybe
there should be a way to opt-out/opt-in for the entire profile or even
per-routes.
https://bugzilla.redhat.com/show_bug.cgi?id=1714438
... and nm_utils_fd_get_contents() and nm_utils_file_set_contents().
Don't mix negative errno return value with a GError output. Instead,
return a boolean result indicating success or failure.
Also, optionally
- output GError
- set out_errsv to the positive errno (or 0 on success)
Obviously, the return value and the output arguments (contents, length,
out_errsv, error) must all agree in their success/failure result.
That means, you may check any of the return value, out_errsv, error, and
contents to reliably detect failure or success.
Also note that out_errsv gives the positive(!) errno. But you probably
shouldn't care about the distinction and use nm_errno_native() either
way to normalize the value.
Otherwise, it just looks odd:
"not priority 31265 from 0.0.0.0/0 fwmark 0xcb87 table 52103"
Better is:
"priority 31265 not from 0.0.0.0/0 fwmark 0xcb87 table 52103"
The "not" specifier should come after the priority. It makes more sense
to read it that way. As far as parsing the string is concerned, the
order does not matter. So this change in behavior is no problem.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/228
I was aware that this code is not reachable. But for consistency, it
seems better to be explict about it (to avoid future bugs when refactoring).
Anyway, Coverity complains about it. So assert instead.
This confuses coverity. Just use MAX(). MAX() is usually not preferred
as it evaluates the arguments more than once. But in this case, it is of
course fine.
CID 202433 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1.
expr_not_constant: expression must have a constant value
