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.
- drops nm_dispatcher_init(), which was called early in the main loop
and created a proxy synchronously. Instead, the GDBusConnection is
always ready.
- reuse the GDBusConnection of NMDBusManager. This means, we won't even
try from in "initrd" configure-and-quit mode.
- as before, there is no "manager" instance. Instead, the data is stored
in a global variable. That's ok. What is not OK is how the entire
shutdown is handled (calling dispatcher blockingly, not waiting for
requests to complete). That is fixable, but a lot of work. It is
independent of whether we use a manager object or not.
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.
This way, we avoid code duplication of how to print the request-id
("(%u) "), but it also will allow up to attach the interface name
and connection name to the call-id, so that we can use it for structured
logging.
We don't need the two callers both unpack the GVariant and pass a GVariantIter.
Let dispatcher_results_process() do the unpacking.
Also, use some cleanup attributes.
Completely refactor the team/JSON handling in libnm's NMSettingTeam and
NMSettingTeamPort.
- team handling was added as rh#1398925. The goal is to have a more
convenient way to set properties than constructing JSON. This requires
libnm to implement the hard task of parsing JSON (and exposing well-understood
properties) and generating JSON (based on these "artificial" properties).
But not only libnm. In particular nmcli and the D-Bus API must make this
"simpler" API accessible.
- since NMSettingTeam and NMSettingTeamPort are conceptually the same,
add "libnm-core/nm-team-utils.h" and NMTeamSetting that tries to
handle the similar code side-by-sdie.
The setting classes now just delegate for everything to NMTeamSetting.
- Previously, there was a very fuzzy understanding of the provided
JSON config. Tighten that up, when setting a JSON config it
regenerates/parses all other properties and tries to make the
best of it. When modifying any abstraction property, the entire
JSON config gets regenerated. In particular, don't try to merge
existing JSON config with the new fields. If the user uses the
abstraction API, then the entire JSON gets replaced.
For example note that nm_setting_team_add_link_watcher() would not
be reflected in the JSON config (a bug). That only accidentally worked
because client would serializing the changed link watcher to
GVariant/D-Bus, then NetworkManager would set it via g_object_set(),
which would renerate the JSON, and finally persist it to disk. But
as far as libnm is concerned, nm_setting_team_add_link_watcher() would
bring the settings instance in an inconsistent state where JSON and
the link watcher property disagree. Setting any property must
immediately update both the JSON and the abstraction API.
- when constucting a team setting from D-Bus, we would previously parse
both "config" and abstraction properties. That is wrong. Since our
settings plugins only support JSON, all information must be present
in the JSON config anyway. So, when "config" is present, only the JSON
must be parsed. In the best case, the other information is redudant and
contributes nothing. In the worse case, they information differs
(which might happen if the client version differs from the server
version). As the settings plugin only supports JSON, it's wrong to
consider redundant, differing information from D-Bus.
- we now only convert string to JSON or back when needed. Previously,
setting a property resulted in parsing several JSON multiple times
(per property). All operations should now scale well and be reasonably
efficient.
- also the property-changed signals are now handled correctly. Since
NMTeamSetting knows the current state of all attributes, it can emit
the exact property changed signals for what changed.
- we no longer use libjansson to generate the JSON. JSON is supposed
to be a machine readable exchange format, hence a major goal is
to be easily handled by applications. While parsing JSON is not so
trivial, writing a well-known set of values to JSON is.
The advantage is that when you build libnm without libjansson support,
then we still can convert the artificial properties to JSON.
- Requiring libjansson in libnm is a burden, because most of the time
it is not needed (as most users don't create team configurations). With
this change we only require it to parse the team settings (no longer to
write them). It should be reasonably simple to use a more minimalistic
JSON parser that is sufficient for us, so that we can get rid of the
libjansson dependency (for libnm). This also avoids the pain that we have
due to the symbol collision of libjansson and libjson-glib.
https://bugzilla.redhat.com/show_bug.cgi?id=1691619
We have nm_setting_verify() for a purpose.
The checks that ifcfg-rh reader does are either
- redundant (and thus unnecessary)
- wrong (and thus we cannot read valid settings)
- should belong to libnm's nm_setting_verify().
NMSettingTeam/NMSettingTeamPort are already very libraral and don't do
almost any strict validation. Previously, ifcfg reader would be even more
liberal. If there is totally invalid data in the profile, reading the profile
should fail.
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.
An active connection started by user could be queued in state UNKNOWN
which means the device hasn't started activating yet. Eventually it
will, and we don't want to cancel the user activation because of an
internal autoconnection attempt.
There was a special case for ensuring that the device's configuration
doesn't disappear when some factory is too late at recognizing the
device is just a component of another one.
It was always a bad idea. If the device already had an active
connection (such as for a generated default wired connection), it would
remain around, with a dangling reference to the device.
This effectively reverts commit 5ad69cb29b ('core: remove child devices
without deconfiguring them (bgo #738479)'). It's okay to do so, because
we now wouldn't deconfigure the device upon its removal anyway.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/151/
The pid-file is private to NetworkManager. It should reside in NetworkManager's
run directory instead of "/var/run".
I don't think that changing this location can break existing uses. Why
would somebody outside of NetworkManager care about this file?
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/157
For the most part, we only have one main .gitignore file.
There were a few nested files, merge them into the main file.
I find it better to have only one gitignore file, otherwise the
list of ignored files is spread out through the working directory.
We already have "libnm-core/tests/test-keyfile.c" from which we build
"test-keyfile".
Our test binaries should be named the following:
- "*/tests/test-*"
- the test binary "*/tests/test-*" should be build from a source file
"*/tests/test-*.c". Meaning: the source's and executable's name should
correspond.
- test binaries should be named uniquely. Also, because older meson
versions don't like having the same binary name more than once.
Rename to avoid the duplicate name.
This will be the default for Slackware 15.0 and on.
This should be safe for both master and 1.12.x stable branch, as
no existing Slackware releases are expected to run NM-1.12.x or
later.
Signed-off-by: Robby Workman <rworkman@slackware.com>
https://mail.gnome.org/archives/networkmanager-list/2019-May/msg00011.html
Older versions of meson don't like building multiple artifacts
with the same name (even if they are in different directories). We
have multiple tests called "test-general.c", and it would be natural
to compile a test binary of the same name.
Meson encountered an error in file src/tests/meson.build, line 14, column 2:
Tried to create target "test-general", but a target of that name already exists.
It's generally a bad idea to have in our source tree multiple files with the
same name. Rename the test.
Fixes: 16cd84d346 ('build/meson: rename platform tests to use same name as autotools'):
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.
There should be no difference in pratice. But we should not mention "/var/run"
as /var might not be mounted in early boot or rescue environment. Of course,
in those cases ConsoleKit is not running either. But still...
"RUNDIR" is set to "$runstatedir/NetworkManager". That is not correct,
we must use "$runstatedir".
I don't understand how this could have ever worked. Commit e2ecf5b808
('dhcp: dhcpcd uses a fixed path for PID files') seems to address this issue,
but already then "RUNDIR" was set to "$(localstatedir)/run/NetworkManager".
NM_LOG_DOMAINS is a comma-separated list of the selected logging domains.
As the number of all logging domains is fixed at compile-time, the maximum
length of the buffer is known.
$ git grep $'^\t{ LOGD_' | sed 's/.*"\(.*\)" .*/\1/' | xargs | sed 's/ */,/g' | sed 's/^/NM_LOG_DOMAINS=/'
NM_LOG_DOMAINS=PLATFORM,RFKILL,ETHER,WIFI,BT,MB,DHCP4,DHCP6,PPP,WIFI_SCAN,IP4,IP6,AUTOIP4,DNS,VPN,SHARING,SUPPLICANT,AGENTS,SETTINGS,SUSPEND,CORE,DEVICE,OLPC,INFINIBAND,FIREWALL,ADSL,BOND,VLAN,BRIDGE,DBUS_PROPS,TEAM,CONCHECK,DCB,DISPATCH,AUDIT,SYSTEMD,VPN_PLUGIN,PROXY
Note that the static buffer "_all_logging_domains_to_str" is known
to be large enough to contain these domain names (it's even longer,
as it also contains "ALL", "IP", and "DHCP" alises). We can use that
to define the array of suitable size.
Don't create a heap allocated GString to hold the static
result of nm_logging_all_domains_to_string().
Instead, use a static buffer of the exactly required size.
The main reason to do this, is to get the exact size of
"_all_logging_domains_to_str" buffer. This is the upper
boundary for the size of a string buffer to hold all domain
names.
We will need that boundary in the next commit. The attractive
thing here is that we will have a unit-test failure if this
boundery no longer matches (--with-more-asserts). That means,
this boundary is guarded by unit tests and we don't accidentally
get it wrong when the domains change.
Also, take care to initialize the buffer in a thread-safe manner.
It's easy enough to get right, so there is no excuse for having
non-thread-safe code in logging.
Syslog's "facility" is a well defined thing and must be
one of a few well-known numbers. Don't re-use it for our
own purposes.
Fixes: 1b808d3b25 ('logging: add native systemd-journald support to nm-logging')
https://bugzilla.redhat.com/show_bug.cgi?id=1709741
This overflow could only happen when we would try to log a message
with "NM_DEVICE=", "NM_CONNECTION=", and more than 8 logging domains
(_NUM_MAX_FIELDS_SYSLOG_FACILITY - 2).
The latter is never the case. While we sometimes log messages with
more than one logging domain, there are no logging statements that
use most as 8 different logging domains. So, this overflow is not
actually reachable from current code (I think).
Fixes: ed552c732c ('logging: log device and connection along with the message'):
Always ensure that the entire buffer is initialized with padding NULs.
For example, valgrind checks whether we access uninitalized memory,
so leaving this uninitalized can be unexpected and cause valgrind
failures. In general, one might be tempted to copy the ifname buffer (of
well known size IFNAMSIZ) with memcpy(). In that case, we should not
have trailing garbage there.
We could use strncpy() for that (which guarantees NUL padding), but
then we still would have to ensure NUL termination. But strncpy() is
frowned upon, so let's not use it here.
Note that g_strlcpy() does not guarantee NUL padding, so it's
unsuitable.
We could also implement this with a combination of memcpy() and
memset(). But in this case, it just seems simpler to iterate over the
16 bytes and do it manually.
==6207== Syscall param ioctl(SIOCETHTOOL) points to uninitialised byte(s)
==6207== at 0x514603B: ioctl (syscall-template.S:78)
==6207== by 0x19FC2F: _ioctl_call (nm-platform-utils.c:183)
==6207== by 0x1A026B: _ethtool_call_handle (nm-platform-utils.c:319)
==6207== by 0x1A031F: ethtool_get_stringset (nm-platform-utils.c:378)
==6207== by 0x1A03BC: ethtool_get_stringset_index (nm-platform-utils.c:414)
==6207== by 0x1A181E: nmp_utils_ethtool_supports_vlans (nm-platform-utils.c:912)
==6207== by 0x1756D7: link_supports_vlans (nm-linux-platform.c:6508)
==6207== by 0x1A81D8: nm_platform_link_supports_vlans (nm-platform.c:1536)
==6207== by 0x14B96B: test_internal (test-link.c:602)
==6207== by 0x4F5C18D: test_case_run (gtestutils.c:2597)
==6207== by 0x4F5C18D: g_test_run_suite_internal (gtestutils.c:2685)
==6207== by 0x4F5BF33: g_test_run_suite_internal (gtestutils.c:2697)
==6207== by 0x4F5C679: g_test_run_suite (gtestutils.c:2772)
==6207== by 0x4F5C694: g_test_run (gtestutils.c:2007)
==6207== by 0x166B4D: main (test-common.c:2092)
==6207== Address 0x1ffeffeecf is on thread 1's stack
==6207== in frame #1, created by _ioctl_call (nm-platform-utils.c:110)
==6207==
"ifname" is the stack-allocated array "known_ifnames" of suitable
IFNAMSIZ bytes. But it may not be fully initialized, so using memcpy()
to copy the string leads to unintialized warning.
We really should only copy the valid bytes, either with strcpy() or our
nm_utils_ifname_cpy() wrapper.
Fixes: 856322562e ('platform/ethtool,mii: retry ioctl when interface name was renamed for ehttool/mii')
"struct ifreq" contains a union field, and initalizing the struct is not
guaranteed to fill all bytes with zero (it only sets the first union
member to zero).
Since we later return the entire struct, ensure that it's initialized to
all zero by using memset().
Ooherwise, the file has wrong permissions:
# ls -la /var/lib/NetworkManager/secret_key
----r-xr-x. 1 root root 50 May 14 13:52 /var/lib/NetworkManager/secret_key
Luckily, /var/lib/NetworkManager should be already
# ls -lad /var/lib/NetworkManager
drwx------. 2 root root 8192 May 14 13:57 /var/lib/NetworkManager
which mitigates this a bit.
Fixes: dbcb1d6d97 ('core: let nm_utils_secret_key_read() handle failures internally')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/175
- use GDBusConnection instead of GDBusProxy.
- namespace global variables with a "gl" struct.
- don't log __func__. If a log line should have a certain
topic/tag, the tag should be set manually, not based on the
function name. It just looks odd. Also, it's unnecessary.
