inet_ntop() is not documented to be thread-safe, so it should not
be used in the DHCP library. Arguably, glibc uses a thread local
buffer, so indeed there is no problem with a suitable libc. Anyway,
just avoid it.
The DHCP client should not pre-filter addresses beyond what RFC
requires. If a client's user (like networkd) wishes to skip/filter
certain addresses, it's their responsibility.
The point of this is that the DHCP library does not hide/abstract
information that might be relevant for certain users. For example,
NetworkManager exposes DHCP options in its API. When doing that, the
options should be close to the actual lease.
This is related to commit d9ec2e632d
(dhcp4: filter bogus DNS/NTP server addresses silently).
deserialize_in_addrs() allocates the buffer before trying to parse
the IP address. Since a parsing error is silently ignored, the returned
size might be zero. In such a case we shouldn't return any buffer.
Anyway, there was no leak, because there are only two callers like
r = deserialize_in_addrs(&lease->dns, dns);
which both keep the unused buffer and later release it.
Note that deserialize_in_addrs() doesn't free the pointer before
reassigning the new output. The caller must take care to to pass
"ret" with an allocated buffer that would be leaked when returning
the result.
Fixes#3374. The problem is that we set MACPolicy=persistent (i.e. we would
like to generate persistent MAC addresses for interfaces which don't have a
fixed MAC address), but various virtual interfaces including bridges, tun/tap,
bonds, etc., do not not have the necessary ID_NET_NAME_* attributes and udev
would not assing the address and warn:
Could not generate persistent MAC address for $name: No such file or directory
Basic requirements which I think a solution for this needs to satisfy:
1. No changes to MAC address generation for those cases which are currently
handled successfully. This means that net_get_unique_predictable_data() must
keep returning the same answer, which in turn means net_get_name() must keep
returning the same answer. We can only add more things we look at with lower
priority so that we start to cover cases which were not covered before.
2. Like 1, but for IPvLL seed and DHCP IAD. This is less important, but "nice
to have".
3. Keep MACPolicy=persistent. If people don't want it, they can always apply
local configuration, but in general stable MACs are a good thing. I have never
seen anyone complain about that.
== Various approaches that have been proposed
=== https://github.com/systemd/systemd/issues/3374#issuecomment-223753264 (tomty89)
if !ID_BUS and INTERFACE, use INTERFACE
I think this almost does the good thing, but I don't see the reason to reject ID_BUS
(i.e. physical hardware). Stable MACs are very useful for physical hardware that has
no physical MAC.
=== https://github.com/systemd/systemd/issues/3374#issuecomment-224733069 (teg)
if (should_rename(device, true))
This means looking at name_assign_type. In particular for
NET_NAME_USER should_rename(..., true) returns true. It only returns false
for NET_NAME_PREDICTABLE. So this would cover stuff like br0, bond0, etc,
but would not cover lo and other devices with predictable names. That doesn't
make much sense.
But did teg mean should_rename() or !should_rename()?
=== https://github.com/systemd/systemd/issues/3374#issuecomment-234628502 (tomty89):
+ if (!should_rename(device, true))
+ return udev_device_get_sysname(device)
This covers only devices with NET_NAME_PREDICTABLE. Since the problem applies as
much to bridges and such, this isn't neough.
=== https://github.com/systemd/systemd/issues/3374#issuecomment-281745967 (grafi-tt)
+ /* if the machine doesn't provide data about the device, use the ifname specified by userspace
+ * (this is the case when the device is virtual, e.g., bridge or bond) */
+ s = udev_device_get_sysattr_value(device, "name_assign_type");
+ if (s && safe_atou(s, &type) >= 0 && type == NET_NAME_USER)
+ return udev_device_get_sysname(device);
This does not cover bond0, vnet0, tun/tap and similar.
grafi-tt also proposes patching the kernel, but *not* setting name_assign_type
seems intentional in those cases, because the device name is a result of
enumeration, not set by the userspace.
=== https://github.com/systemd/systemd/issues/3374#issuecomment-288882355 (tomty89)
(also PR #11372)
- MACAddressPolicy=persistent
This break requirement 3. above. It would solve the immediate problem, but I
think the disruption is too big.
=== This patch
This patch means that we will set a "stable" MAC for pretty much any virtual
device by default, where "stable" means keyed off the machine-id and interface
name.
It seems like a big change, but we already did this for most physical devices.
Doing it also for virtual devices doesn't seem like a big issue. It will make
the setup and monitoring of virtualized networks slightly nicer. I don't think
anyone is depending on having the MAC address changed when those devices are
destoryed and recreated. If they do, they'd have to change MACAddressPolicy=.
== Implementation
net_get_name() is called from dhcp_ident_set_iaid() so I didn't change
net_get_name() like in grafi-tt's patch, but net_get_unique_predictable_data().
net_get_unique_predictable_data() is called from get_mac() in link-config.c
and sd_ipv4ll_set_address_seed(), so both of those code paths are affected
and will now get data in some cases where they errored out previously.
The return code is changed to -ENODATA since that gives a nicer error string.
An earlier commit 0e408b82b (dhcp6-client: handle IAID with value zero)
introduced a flag to sd_dhcp6_client to distinguish between an unset
IAID and a value set to zero.
However, that was not sufficient and broke leaving the setting
uninitialized in networkd configuration. The configuration parsing
also must distinguish between the default, unset value and an
explict zero configuration.
Fixes: 0e408b82b8
Prior to this commit, a .link file with a [Match] section containing
MACAddress= would match any device without a MAC. This restores the
matching logic prior to e90d037.
This part of the copyright blurb stems from the GPL use recommendations:
https://www.gnu.org/licenses/gpl-howto.en.html
The concept appears to originate in times where version control was per
file, instead of per tree, and was a way to glue the files together.
Ultimately, we nowadays don't live in that world anymore, and this
information is entirely useless anyway, as people are very welcome to
copy these files into any projects they like, and they shouldn't have to
change bits that are part of our copyright header for that.
hence, let's just get rid of this old cruft, and shorten our codebase a
bit.
Previously we were a bit sloppy with the index and size types of arrays,
we'd regularly use unsigned. While I don't think this ever resulted in
real issues I think we should be more careful there and follow a
stricter regime: unless there's a strong reason not to use size_t for
array sizes and indexes, size_t it should be. Any allocations we do
ultimately will use size_t anyway, and converting forth and back between
unsigned and size_t will always be a source of problems.
Note that on 32bit machines "unsigned" and "size_t" are equivalent, and
on 64bit machines our arrays shouldn't grow that large anyway, and if
they do we have a problem, however that kind of overly large allocation
we have protections for usually, but for overflows we do not have that
so much, hence let's add it.
So yeah, it's a story of the current code being already "good enough",
but I think some extra type hygiene is better.
This patch tries to be comprehensive, but it probably isn't and I missed
a few cases. But I guess we can cover that later as we notice it. Among
smaller fixes, this changes:
1. strv_length()' return type becomes size_t
2. the unit file changes array size becomes size_t
3. DNS answer and query array sizes become size_t
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=76745
Double newlines (i.e. one empty lines) are great to structure code. But
let's avoid triple newlines (i.e. two empty lines), quadruple newlines,
quintuple newlines, …, that's just spurious whitespace.
It's an easy way to drop 121 lines of code, and keeps the coding style
of our sources a bit tigther.
Files which are installed as-is (any .service and other unit files, .conf
files, .policy files, etc), are left as is. My assumption is that SPDX
identifiers are not yet that well known, so it's better to retain the
extended header to avoid any doubt.
I also kept any copyright lines. We can probably remove them, but it'd nice to
obtain explicit acks from all involved authors before doing that.
This macro will read a pointer of any type, return it, and set the
pointer to NULL. This is useful as an explicit concept of passing
ownership of a memory area between pointers.
This takes inspiration from Rust:
https://doc.rust-lang.org/std/option/enum.Option.html#method.take
and was suggested by Alan Jenkins (@sourcejedi).
It drops ~160 lines of code from our codebase, which makes me like it.
Also, I think it clarifies passing of ownership, and thus helps
readability a bit (at least for the initiated who know the new macro)
log.h really should only include the bare minimum of other headers, as
it is really pulled into pretty much everything else and already in
itself one of the most basic pieces of code we have.
Let's hence drop inclusion of:
1. sd-id128.h because it's entirely unneeded in current log.h
2. errno.h, dito.
3. sys/signalfd.h which we can replace by a simple struct forward
declaration
4. process-util.h which was needed for getpid_cached() which we now hide
in a funciton log_emergency_level() instead, which nicely abstracts
the details away.
5. sys/socket.h which was needed for struct iovec, but a simple struct
forward declaration suffices for that too.
Ultimately this actually makes our source tree larger (since users of
the functionality above must now include it themselves, log.h won't do
that for them), but I think it helps to untangle our web of includes a
tiny bit.
(Background: I'd like to isolate the generic bits of src/basic/ enough
so that we can do a git submodule import into casync for it)
This adds a simple condition/assert/match to the service manager, to
udev's .link handling and to networkd, for matching the kernel version
string.
In this version we only do fnmatch() based globbing, but we might want
to extend that to version comparisons later on, if we like, by slightly
extending the syntax with ">=", "<=", ">", "<" and "==" expressions.
* networkd: condition_test() can return a negative error, handle that
If a condition check fails with an error we should not consider the check
successful. Fix that.
We should probably also improve logging in this case, but for now, let's just
unbreak this breakage.
Fixes: #3236
* condition: handle unrecognized architectures nicer
When we encounter a check for an architecture we don't know we should not
let the condition check fail with an error code, but instead simply return
false. After all the architecture might just be newer than the ones we know, in
which case it's certainly not our local one.
Fixes: #3236
1. Replace strtol with unhexchar, verified with valid and invalid DUID strings.
2. Fix logging to use log_syntax instead of log_error.
3. On error reading DUID, ignore read and preserve previous state.
4. Fix man-pages to use markup, remove options not yet implemented.
5. Remove spurious header line in new files.
Throughout the tree there's spurious use of spaces separating ++ and --
operators from their respective operands. Make ++ and -- operator
consistent with the majority of existing uses; discard the spaces.
At the moment sd_dhcp_lease_get_routes() returns an array of structs
which are not defined in public headers. Instead, change the function
to return an array of pointers to opaque sd_dhcp_route objects.
