- use svGetValue() instead of svGetValueStr(). The difference is that
svGetValueStr() coerces "" to NULL. "" is not a valid value, but we
want to parse the value and print an warning message about it. Also,
the presence of the variable determines whether we add the bond-port
setting or not.
- don't use nm_clear_g_free(). @value_to_free is gs_free, it will be
cleared automatically.
- use g_object_set() instead of nm_g_object_set_property_uint(). The
latter is our own implementation that does error checking (e.g., that
the value is in range (0..2^16-1). But we already ensured that to
be the case. So just call g_object_set(), it cannot fail and if it
would, we want the assertion failure that it would cause.
- queue_id should be a "guint". It is always true on Linux/glib that
sizeof(guint) >= sizeof(guint32), the opposite theoretically might not
be true.
But later we use the variable in the variadic function g_object_set(),
where it should be guint.
- the errno from _nm_utils_ascii_str_to_uint64() isn't very useful for
logging. It's either ERANGE or EINVAL, and logging the numeric values
of these error codes isn't gonna help the user. We could stringify
with nm_strerror_native(errno), but that message is also not very
useful. Just say that the string is not a number.
No actual caller should use the API without providing either a filename
or the directory name. I don't think this can actually happen, hence
fail and assert in that case.
Have an ifcfg file loaded in NetworkManager, then move/remove the file and try
to modify it. That will fail with:
"failed to update connection: Could not read file '/etc/sysconfig/network-scripts/ifcfg-eth0': No such file or directory"
That is not right.
If the user didn't move/remove the file but merely modified it, NetworkManager
would silently overwrite it. There is no reason why move/remove should behave
differently and not just write a completely fresh file.
The reason why NetworkManager first loads the file before writing, is to
preserve comments and unrecognized shell variables. This is a certain effort
to play nice with users editing the file. It's not essential to load the file
first and a failure to do so should not result in a failure.
And of course, keyfile writer doesn't behave like this either.
This bug exists since 2009, but let's not add a "Fixes" comment for
commit 1974b257e0 ('ifcfg-rh: begin adding write support'), because
it seems not right to backport this patch to all the old releases.
Do "bother" to read/write settings.
For the umpteenth time, it's not up to the reader/writer to decide
what properties are valid for a profile or which makes sense.
Only nm_connection_verify() can decide that. For example nm_connection_verify()
has no problem with ipv6.method=disabled while also setting ipv6.addr-gen-mode.
We cannot just shortcut the parsing/writing.
The reader only ignores addresses, dns and dns-searches, so that we don't start
parsing invalid files, where the setting would have been ignore
previously.
In particular,
echo "DEVICE=eth0" > /etc/sysconfig/network-scripts/ifcfg-xxx
nmcli connection load /etc/sysconfig/network-scripts/ifcfg-xxx
nmcli -f ipv6.method,ipv6.addr-gen-mode connection show /etc/sysconfig/network-scripts/ifcfg-xxx
needs to show eui64 addr-gen-mode.
It's not the task of the ifcfg reader to pre-normalize profiles
to truncate the DNS server list. It's only nm_connection_verify()'s
task to indicate what is valid and what not.
Increase the number to something excessive. Note that the parsing
scales with O(n^2). So don't have it totally unbounded and have an
overall limit (of 10000 entries).
Now that NetworkManager on Fedora 33 and RHEL 9 no longer writes
ifcfg-rh files by default ([1]), ifup/ifdown became less useful.
Possibly users shouldn't use it and it would be fine that new-style profiles
(keyfile) no longer work with these commands. But this is deemed as too
disruptive for users.
Note that our previous ifup/ifdown compat scripts only honored the argument
to be part of the ifcfg filename. That was not what initscripts were doing,
which called `need_config()` function that searched also the contents of
the files. With this extension, ifup/ifdown gets smarter too, to better
guess what the user might have wanted.
Extend the script by making it smarter, and to work with connection profile
names.
With this extension we further solidify ifup/ifdown as part of NetworkManager
command line API. That is problematic, because these tools pollute the
$PATH, by not having a clear NM-specific name. Also, these scripts
should only exist on Fedora/RHEL, which makes their usage non-portable
to other distros.
Also, other distros already ship different tools with name ifup/ifdown.
Extending the use of these scripts is thus undesirable, as it furthers
distro-specific commands.
Still, these arguments seem to not hold and users need to be "helped".
As Fedora users cannot be expected to unlearn "ifup" today, there is no
reason to assume they could in a few years. This likely means we will
never get rid of these scripts.
Also, if we truly would make ifup/ifdown part of NetworkManager, then a better
implementation would be that nmcli honors being called with these names.
That is not done, because nmcli's implementation currently is not as
nice to make that extension trivial (as it should be). It also would
mean to embrace ifup/ifdown officially. A shell script works well enough
as a hack.
[1] https://fedoraproject.org/wiki/Changes/NetworkManager_keyfile_instead_of_ifcfg_rhhttps://bugzilla.redhat.com/show_bug.cgi?id=1954607https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/936
The "utils" part does not seem useful in the name.
Note that we also have NMStrBuf, which is named nm_str_buf_*().
There is an unfortunate similarity between the two, but it's still
distinct enough (in particular, because one takes an NMStrBuf and
the other not).
This is a normalization employed by NMSettingIPConfig.gateway.
Also rework NMSettingIPConfig.set_property() to no longer assert against
valid input. We want to pass there untrusted strings from D-Bus,
asserting is a horrible idea. Instead, either normalize the string or
keep the invalid text that will be rejected by verify().
This is like using nm_ascii_is_ctrl_or_del() instead of
nm_ascii_is_ctrl() in the previous version of the patch.
We thus now always will switch to ANSIC escaping if we see
a ASCII DEL character. That is probable desirable, but either
way should not make a big difference (because we can parse
the DEL character both in regular quotation and in ANSIC quotation).
The patch is however larger, to also take the opportunity to only check
for nm_ascii_is_regular() in the "fast path". The behavior is the same
as changing nm_ascii_is_ctrl() to nm_ascii_is_ctrl_or_del().
Problems of this patch:
- the code does not differentiate between an ifcfg file and an alias
file. Different shell variables are honored however depending on the
context and the warning should reflect that.
- there are no warnings about /etc/sysconfig/network. The main problem
is that we read this file for every ifcfg file we parse, and we would
need to ratelimit the number of warnings. Another problem is that
the file likely contains keys that we intentionally don't support.
We would need a new way to omit warnings about those lines.
Example:
TYPE=Ethernet
PROXY_METHOD=none
BROWSER_ONLY=no
BOOTPROTO=dhcp
DEFROUTE=yes
STABLE_ID=$'xxx\xF4yy'
IPV4_FAILURE_FATAL=no
IPV6INIT=yes
XX=foo
XX1=foo'
'
IPV6_AUTOCONF=yes xxxx
IPV6_DEFROUTE=yesx
IPV6_DEFROUTE=yes
IPV6_FAILURE_FATAL=no
IPV6_ADDR_GEN_MODE=stable-privacy
NAME=xxx
UUID=9d8ed7ff-3cdd-4336-9e26-3e978dc87102
ONBOOT=no
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:6: key STABLE_ID does not contain valid UTF-8 and is treated as ""
<debug> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:9: key XX is unknown and ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:10: key XX1 is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:11: invalid line ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:12: key IPV6_AUTOCONF is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:13: key IPV6_DEFROUTE is duplicated and the early occurrence ignored
https://bugzilla.redhat.com/show_bug.cgi?id=1959656
ifcfg files are a text format. It makes no sense to ever accept
non-UTF-8 blobs. If binary data is to be encoded in a ifcfg file, then
the upper layers must escape/encode it in valid UTF-8.
Let svUnescape() silently reject any binary "text". This will lead to treat such
strings as empty strings "". This is no different than some invalid
quoting: the string is not parsable as (UTF-8) text and will be treated
as such.
This is potentially a breaking change. But the benefit is that all the
upper layers can rely on only getting valid UTF-8 strings. For example,
a non-UTF-8 string cannot be converted to a "s" GVariant (of course not,
it's not a string). But our nm_connection_verify() commonly does not
check that all strings are in fact valid UTF-8. So a user who edits
an ifcfg file could inject non-valid strings, and cause assertion
failures later on.
It's actually easy to provoke a crash (or at least an assertion failure)
by writing an ifcfg file with certain keys as binary.
Note that you can either reproduce the binary files by writing non-UTF-8
"strings" dirctly, or by using \x, \u, or \U escape sequences.
Note that also '\0' gets rejected and renders the string as invalid
(i.e. as empty). Before the returned string would have been simply
truncated and the rest ignored. Such NUL bytes can only be produced
using the escape sequences, because the ifcfg reader already (silently)
truncates the file on the first binary NUL.
Note that previously the check
if (s[slen] < ' ') {
...
return (*to_free = _escape_ansic(s));
}
would be TRUE for all UTF-8 characters if `char` is signed. That means,
depending on the compiler, we would always ANSI escape all UTF-8
characters. With this patch, we no longer do that!
Instead, valid unicode gets now preserved (albeit quoted).
On the other hand, always ANSIC escape invalid UTF-8 (regardless of the
compiler). ifcfg-rh is really a text based format. If a caller wants to store
binary data, they need to escape it first, for example with some own escaping
scheme, base64 or bin2hexstr.
A caller passing a non-text to svEscape() is likely a bug already and
they should have not done that.
Still, let svEscape() handle that by using ANSIC escaping. That works
as far as escaping is concerned, but likely later will be a problem
during unescaping, when the reader expects a valid UTF-8 string.
svEscape() is in no place to signal a sensible error, so proceed the
best it can, by escaping.
Add a new property to specify the minimum time interval in
milliseconds for which dynamic IP configuration should be tried before
the connection succeeds.
This property is useful for example if both IPv4 and IPv6 are enabled
and are allowed to fail. Normally the connection succeeds as soon as
one of the two address families completes; by setting a required
timeout for e.g. IPv4, one can ensure that even if IP6 succeeds
earlier than IPv4, NetworkManager waits some time for IPv4 before the
connection becomes active.
We somehow need to encode an NMSettingEthtool instance that has all
options unset. Previously, that would result in no "$ETHTOOL_OPTS"
variable and thus the reader would loose a previously existing setting.
Hack it by writing a bogus
ETHTOOL_OPTS="-A $IFACE"
line.
It just seems ugly to call g_getenv() repeatedly. Environment variables
must not change (in a multi-threaded program after other threads start),
so determine the mode once and cache it.
For the umpteenth time: it is not ifcfg-rh writers decision to decide
what are valid configurations and only persist settings based on
some other settings.
If s390-options would only be allowed together with subchannels, then
this is alone nm_connection_verify()'s task to ensure.
Reproduce with
$ nmcli connection add type ethernet autoconnect no con-name zz ethernet.s390-options bridge_role=primary
Related: https://bugzilla.redhat.com/show_bug.cgi?id=1935842
Fixes: 16bccfd672 ('core: handle s390 options more cleanly')
Since commit 207cf3d5d4 ('libnm: normalize "connection.uuid"') the
"connection.uuid" is normalized to be a valid UUID and all lower case.
That means, if we have .nmmeta files on disk with a previously valid,
but now invalid UUID, the meta file is no longer going to match.
Reject such file outright as invalid. If we really wanted to preserve
backward compatibility, then we would have to also normalize the
filename when we read it. However, that means, that suddenly we might
have any number of compatible .nmmeta files that normalize to the same
UUID, like the files
71088c75dec54119ab41be71bc10e736aaaabbbb.nmmeta
F95D40B4-578A-5E68-8597-39392249442B.nmmeta
f95d40b4-578a-5e68-8597-39392249442b.nmmeta
Having multiple places for the nmmeta file is complicated to handle.
Also, we often have the connection profile (and the normalized UUID)
first, and then check whether it has a .nmmeta file. If we would support
those unnormalized file names, we would have to visit all file names and
try to normalize it, to find those with a matching UUID.
Non-normalized UUIDs really should not be used and they already are not
working anymore for the .nmmeta file. This commit only outright rejects
them. This is a change in behavior, but the behavior change happened
earlier when we started normalizing "connection.uuid".
"uuid" is returned from nms_keyfile_nmmeta_check_filename(),
and contains "$UUID.nmmeta". We must compare only the first
"uuid_len" bytes.
Fixes: 064544cc07 ('settings: support storing "shadowed-storage" to .nmmeta files')
If the TC setting contains no qdiscs and filters, it is lost after a
write-read cycle. Fix this by adding a new property to indicate the
presence of the (empty) setting.
Previously, we would allocate a buffer of the worst case, that is,
4 times the number of bytes, in case all of them require octal escaping.
Coverity doesn't like _escape_ansic() for another reason:
Error: NULL_RETURNS (CWE-476): [#def298]
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: returned_null: "g_malloc" returns "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: alias: Assigning: "q" = "dest = g_malloc(strlen(source) * 4UL + 1UL + 3UL)". Both pointers are now "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:163: dereference: Incrementing a pointer which might be null: "q".
# 161| q = dest = g_malloc(strlen(source) * 4 + 1 + 3);
# 162|
# 163|-> *q++ = '$';
# 164| *q++ = '\'';
# 165|
It doesn't recognize that g_malloc() shouldn't return NULL (because
we never request zero bytes).
I am not sure how to avoid that, but let's rework the code to first count
how many characters we exactly need. It think that should also help with
the coverity warning.
Doing exact allocation requires first to count the number of required
bytes. It still might be worth it, because we might keep the allocated
strings a bit longer around.
When the ACCEPT_ALL_MAC_ADDRESSES key is found by the wired reader, the
wired setting was not being created.
Fixes: d946aa0c50 ('wired-setting: add support to accept-all-mac-addresses')
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Introducing ethtool PAUSE support with:
* ethtool.pause-autoneg on/off
* ethtool.pause-rx on/off
* ethtool.pause-tx on/off
Limitations:
* When `ethtool.pause-autoneg` is set to true, the `ethtool.pause-rx`
and `ethtool.pause-tx` will be ignored. We don't have warning for
this yet.
Unit test case included.
Signed-off-by: Gris Ge <fge@redhat.com>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/829
It's not the task of the writer to mangle/normalize profiles. If a profile
for a virtual device can have an [ethernet] setting, then unsuitable values
like s390 options must be either rejected by nm_connection_verify() or normalized
by nm_connection_normalize(). In no way it's right that the writer simple
pretends they are not set.
"string" is leaked in the error case. But in practice, this cannot
happen because nm_bridge_vlan_to_str() cannot fail.
While at it, replace GString by NMStrBuf.
Thanks Coverity:
Error: RESOURCE_LEAK (CWE-772):
NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1565: alloc_fn: Storage is returned from allocation function "g_string_new".
NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1565: var_assign: Assigning: "string" = storage returned from "g_string_new("")".
NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1572: leaked_storage: Variable "string" going out of scope leaks the storage it points to.
# 1570| vlan_str = nm_bridge_vlan_to_str(vlan, error);
# 1571| if (!vlan_str)
# 1572|-> return FALSE;
# 1573| if (string->len > 0)
# 1574| g_string_append(string, ",");
This patch is introducing the wired setting accept-all-mac-addresses
property. The value corresponds to the kernel flag IFF_PROMISC.
When accept-all-mac-address is enabled, the interface will accept all
the packets without checking the destination mac address.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
