The library should not print to stdout/stderr. This function is used to
convert untrusted(!!) input to a normalized and sanitized strv array.
g_warning() is essentially an assertion, and it's wrong to do that
for untrusted data. If the caller had to pre-validate the array, then
having this function would be pointless.
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.
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.
Rejecting %NULL for a "is-a" function can be annoying. Of course, %NULL is
not a valid name. But it's sufficient that the function just returns
%FALSE in that case, and not assert against the input not being %NULL.
Asserting might be useful to catch bugs, but rejecting %NULL as input
is more cumbersome to the caller than helping with catching bugs.
Something similar was also recently done for nm_utils_is_uuid().
We usually want to combine the fields from "struct timespec" to
have one timestamp in either nanoseconds or milliseconds.
Use nm_utils_clock_gettime_*() util for that.
For a "is" check, it's inconvenient to assert against the parameter
being %NULL. We should accept %NULL and just say that it's not a valid
uuid.
This relaxes previous API.
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.
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
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.
The memory-limit is an unsigned integer. It is ugly (if not wrong) to compare unsigned
values with "-1". When comparing with the default value we must also use an u32 type.
Instead add a define NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET.
Note that like iproute2 we treat NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
to indicate to not set TCA_FQ_CODEL_MEMORY_LIMIT in RTM_NEWQDISC. This
special value is entirely internal to NetworkManager (or iproute2) and
kernel will then choose a default memory limit (of 32MB). So setting
NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET means to leave it to kernel to
choose a value (which then chooses 32MB).
See kernel's net/sched/sch_fq_codel.c:
static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
{
...
q->memory_limit = 32 << 20; /* 32 MBytes */
static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
...
if (tb[TCA_FQ_CODEL_MEMORY_LIMIT])
q->memory_limit = min(1U << 31, nla_get_u32(tb[TCA_FQ_CODEL_MEMORY_LIMIT]));
Note that not having zero as default value is problematic. In fields like
"NMPlatformIP4Route.table_coerced" and "NMPlatformRoutingRule.suppress_prefixlen_inverse"
we avoid this problem by storing a coerced value in the structure so that zero is still
the default. We don't do that here for memory-limit, so the caller must always explicitly
set the value.
I think initializing structs should (almost) be always done with designated
initializers, because otherwise it's easy to get the order wrong. The
problem is that otherwise the order of fields gets additional meaning
not only for the memory layout, but also for the code that initialize
the structs.
Add a macro NM_VARIANT_ATTRIBUTE_SPEC_DEFINE() that replaces the other
(duplicate) macros. This macro also gets it right to mark the struct as
const.
This actually allows the compiler/linker to mark the memory as read-only and any
modification will cause a segmentation fault.
I would also think that it allows the compiler to put the structure directly
beside the outer constant array (in which this pointer is embedded). That is good
locality-wise.
- g_ascii_strtoll() accepts leading spaces, but it leaves
the end pointer at the first space after the digit. That means,
we accepted "1: 0" but not "1 :0". We should either consistently
accept spaces around the digits/colon or reject it.
- g_ascii_strtoll() accepts "\v" as a space (just like `man 3 isspace`
comments that "\v" is a space in C and POSIX locale.
For some reasons (unknown to me) g_ascii_isspace() does not treat
"\v" as space. And neither does NM_ASCII_SPACES and
nm_str_skip_leading_spaces().
We should be consistent about what we consider spaces and what not.
It's already odd to accept '\n' as spaces here, but well, lets do
it for the sake of consistency (so that it matches with our
understanding of ASCII spaces, albeit not POSIX's).
- don't use bogus error domains in "g_set_error (error, 1, 0, ..."
That is a bug and we have NM_UTILS_ERROR exactly for error instances
with unspecified domain and code.
- as before, accept a trailing ":" with omitted minor number.
- reject all unexpected characters. strtoll() accepts '+' / '-'
and a "0x" prefix of the numbers (and leading POSIX spaces). Be
strict here and only accepts NM_ASCII_SPACES, ':', and hexdigits.
In particular, don't accept the "0x" prefix.
This parsing would be significantly simpler to implement, if we could
just strdup() the string, split the string at the colon delimiter and
use _nm_utils_ascii_str_to_int64() which gets leading/trailing spaces
right. But let's save the "overhead" of an additional alloc.
Merge the function pointer get_func() into to_dbus_fcn().
Previously, get_func() as handled separately from to_dbus_fnc()
(formerly synth_func()). The notion was that synth-func would syntetize
properties that are D-Bus only. But that distinction does not seem
very helpful to me.
Instaed, we want to convert a property to D-Bus. Period. The
implementation should be handled uniformly. Hence, now that is
all done by property_to_dbus().
Note that property_to_dbus() is also called as default implementation
for compare-property. At least, for properties that are backed by a
GObject property.
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
(cherry picked from commit af07ed01c0)
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
- if there is only one vlan in the list, then we can return success
early. That is, because one NMBridgeVlan instance is always valid
due to the way how users must use the API to construct the element.
- the implementation for check_normalizable is only correct, if there
are no duplicate or overlapping ranges. Assert for that. In fact,
all callers first check for errors and then for normalizable errors.
- avoid duplicate calls to nm_bridge_vlan_get_vid_range(). There are
duplicate assertions that we don't need.
- only check for pvid once per range.
- combine calls to g_hash_table_contains() and g_hash_table_add().
(cherry picked from commit a358da096f)
In some cases it is convenient to specify ranges of bridge vlans, as
already supported by iproute2 and natively by kernel. With this commit
it becomes possible to add a range in this way:
nmcli connection modify eth0-slave +bridge-port.vlans "100-200 untagged"
vlan ranges can't be PVIDs because only one PVID vlan can exist.
https://bugzilla.redhat.com/show_bug.cgi?id=1652910
(cherry picked from commit 7093515777)
The call to nm_utils_parse_variant_attributes() is useless. The following
_tc_read_common_opts() call does the same thing. This was probably left
in place by accident.
When we delete the runner.name property, the runner object itself gets
deleted if that was the only property, and @runner becomes invalid.
==13818== Invalid read of size 1
==13818== at 0x55EAF4: nm_streq (nm-macros-internal.h:869)
==13818== by 0x55EAF4: _json_team_normalize_defaults (nm-utils.c:5573)
==13818== by 0x566C89: _nm_utils_team_config_set (nm-utils.c:6057)
==13818== by 0x5498A6: _nm_utils_json_append_gvalue (nm-utils-private.h:228)
==13818== by 0x5498A6: set_property (nm-setting-team.c:1622)
==13818== Address 0x182a9330 is 0 bytes inside a block of size 13 free'd
==13818== at 0x4839A0C: free (vg_replace_malloc.c:530)
==13818== by 0x4857868: json_delete_string (value.c:763)
==13818== by 0x4857868: json_delete (value.c:975)
==13818== by 0x4851FA1: UnknownInlinedFun (jansson.h:129)
==13818== by 0x4851FA1: hashtable_do_del (hashtable.c:131)
==13818== by 0x4851FA1: hashtable_del (hashtable.c:289)
==13818== by 0x55DFDD: _json_del_object (nm-utils.c:5384)
==13818== by 0x55EA70: _json_delete_object_on_string_match (nm-utils.c:5532)
==13818== by 0x55EADB: _json_team_normalize_defaults (nm-utils.c:5549)
==13818== by 0x566C89: _nm_utils_team_config_set (nm-utils.c:6057)
==13818== by 0x5498A6: _nm_utils_json_append_gvalue (nm-utils-private.h:228)
==13818== by 0x5498A6: set_property (nm-setting-team.c:1622)
==13818== Block was alloc'd at
==13818== at 0x483880B: malloc (vg_replace_malloc.c:299)
==13818== by 0x4852E8C: lex_scan_string (load.c:389)
==13818== by 0x4852E8C: lex_scan (load.c:620)
==13818== by 0x4853458: parse_object (load.c:738)
==13818== by 0x4853458: parse_value (load.c:862)
==13818== by 0x4853466: parse_object (load.c:739)
==13818== by 0x4853466: parse_value (load.c:862)
==13818== by 0x4853655: parse_json.constprop.7 (load.c:899)
==13818== by 0x48537CF: json_loads (load.c:959)
==13818== by 0x566780: _nm_utils_team_config_set (nm-utils.c:5961)
==13818== by 0x5498A6: _nm_utils_json_append_gvalue (nm-utils-private.h:228)
==13818== by 0x5498A6: set_property (nm-setting-team.c:1622)
Fixes: a5642fd93a ('libnm-core: team: rework defaults management on runner properties')
The VFs already can be parsed as plain number (to indicate the
ifindex). We should not also support accepting the plain number
as index to be removed.
Fixes: a2f12994b7 ('cli: add support for configuring SR-IOV')
- use nm_auto_decref_json for "json_value" to indicate ownership
transfer.
- don't reuse variable json_element and json_link to construct
watchers list. It's confusing. In general, use different variables
for different purposes.