The existing limit for the reports was a global METRICS_MAX cap.
However on systems with a verbose socket or with many sockets
that is easily hit and because sockets are contacted in parallel
random parts of the following report generators are skipped.
It seems better to instead make the limit per-source so that one
that is overly verbose cannot starve the others. Because we already
have METRICS_LINKS_MAX this does not add a global limit - we could add
one but it would have the same problem as METRICS_MAX before, some
report would be cut that might be important (or we need to be explicit
about ordering but even then its problematic if we have two
"important" ones and one is buggy and generates too much data).
Note that this keeps the METRICS_PER_SOURCE_MAX at 4096 because
io.systemd.Manager reports a lot, on my idle test VM already
~3k.
We also now report any issue with the metric instead of just the
first one in verb_metrics.
Originally https://github.com/systemd/systemd/pull/43595 was
targeted for v263 and the NEWS entry was for v263 too.
However it got merged now for v262 (for the right reasons) so
this commit updates the NEWS file.
The json mode was returning the string `none` when unmerged and
an array for merged extensions. Return an empty array instead.
Instead of branching on the output format, always add an empty strv
for an unmerged hierarchy: the table code renders an empty strv as
the ersatz string ("-") in text mode and as an empty array in json
mode. This also changes the human-readable output from "none" to "-".
It also updated the TEST-50-DISSECT.sysext.sh test. Technically
this is a compatbility break but its also arguably a bugfix.
The petnames wordlist added in [1] is Apache-2.0 and this
causes problems with the distribution of systemd as the
GPL-2 and Apache-2.0 are considered incompatible by the FSF.
So this commit replaces the previous wordlist with
a new one:
- weather (basic english weather adjectives)
- colors (basic english color names)
- worlds (planets, moons, minor bodies of the solar system)
All common vocabulary and compiled for this project, not taken
from anywhere.
An example with the new list:
- wintry-rust-dione-9393
- warm-yellow-saturn-39f9
The "worlds" are picked on the following principles:
- well-known
- memorable
- short(ish)
- cool
This is why e.g. we have "thetis" (an astroid) instead of
"tethys" (a moon): easier to spell and more well known.
[1] https://github.com/systemd/systemd/pull/42566
When encrypting with the `--with-key=null` option systemd-creds
is currently doing the encryption via IPC. This is not needed
for the null key, no privs are required so we can just do the
in-process operation. So instead simply check for the null-key and
if its requested use the in-process path.
This commit tweaks the in_first_boot() code to make it
a proper shared helper. It tweaks the comments about the
caching policy. This is important because while this is
simialr to `in_initrd()` there is a suble difference:
we never cache the result of access() because other parts
of systemd will use this file to signal if anything about
firstboot changes.
This also tweaks it to return an int instead of a bool.
So any error is returned as -errno. This allows other
parts of systemd (like the random-seed) that need to make
decisions about "unknown" first-boot state.
This also means that the consumers need to get updated
and we can now also use the new helper in random-seed-tool.c
and have only a single helper to check for first-boot.
For the new systemd.credentials_boot_policy= setting we need
to be able to differenciate if its a firstboot or not early.
But `manager_set_first_boot` is run after `initialize_runtime()`
which imports the credentials. So we need to touch the file
so that the credentials_boot_policy setting can work.
Unfortunately we cannot just move manager_set_first_boot() earlier
because there is no manager yet at this point.
This commit adds a new `systemd.credentials_boot_policy=` kernel
commandline that allows to control if credentials with
a `null` key are accepted.
The possible options are:
* strict: always insist on tpm encryption
* tofu: allow null encryption in firstboot mode and when no tpm is available
* relaxed: allow null encryption when sb is off, or no tpm is available
* off: allow null encryption always
The default is currently `relaxed` which is the same behavior
as before.
This replaces the initial idea of using plaintext credentials
at firstboot (thanks to Lennart for this nicer and simpler design).
Note that this also moves `in_first_boot()` to `basic/initrd-util`
which is a better fit now.
By chance (because I had this mistake in my own commit) I noticed
that there are a bunch of duplicated works like "the the" in the
code and man-pages.
This commit fixes them and some similar issues with "and and" etc.
This commit implements `journalctl -f` like behavior for the
varlink API of journalctl. It is used via:
```
$ varlinkctl call -E \
/run/systemd/io.systemd.JournalAccess \
io.systemd.JournalAccess.GetEntries '{"follow": true, "limit": 10}'
```
This gives the last 10 message and then it keeps the connection
open and output each new log line that matches the set filters.
The code is modeled after `journalctl -f`. It seems there is little
to extract into shared code here so I left it for now.
This adds a new systemd.firstboot=headless mode. It differs from the
existing systemd.firstboot=no mode in that it still performs the
non-interactive auto-configuration that requires no user input (such
as selecting the only installed locale, or applying settings provided
via credentials), and only skips the prompts that would otherwise
block waiting for user input. In contrast, =no disables that
auto-configuration along with the prompts.
The option is also honoured by homectls firstboot logic and
systemd-cryptenroll, where headless behaves the same as no
(because the is no auto-configuration).
Similar to PR#42360 this commit adds missing properties for
absolute path handling in io.systemd.Unit.StartTransient for
the `Exec` context and a macro helper to share the common code.
The properties added are:
IPCNamespacePath, NetworkNamespacePath, RootDirectory, RootHashPath,
RootHashSignaturePath, RootImage, RootVerity, UserNamespacePath,
RootMStack.
Note that RootHashPath, RootHashSignaturePath need custom apply
functions because the varlink name "RootHashPath" differs from the
name that needs to be written into the unit file ("RootHash=")
and the iovec must be cleared.
Lennart reminded me in [1] that we need to add assert() in functions
that do pointer access. For the simple `*p` pointer dereferences
we even have an automatic coccinelle script that ensures that as
part of the automatic code checks.
However for deref in the `p->` style this is not supported right
now and adding it to coccinelle is hard because its too slow for
this kind of check. So I created a (slightly messy) tree-sitter
python script to see how many asserts we are currently missing.
This commit is the result of running it over the `src/basic`
dir and fixing the flagged issues. I plan to tidy it up and
add it to the checks too but this is orthogonal to this commit.
[1] https://github.com/systemd/systemd/pull/42360#discussion_r3426964562
This commit exposes the last 10 high priority logs as metrics
so that the systemd-report reports them. The entries are
reported as `io.systemd.Journal.HighPriorityMessage` and
include all field as the new METRIC_FAMILY_TYPE_OBJECT.
Individual fields from a journal entry that are unprintable
(invalid utf-8) are skipped.
This is archived via a new socket-activated unit listens on
/run/systemd/report/io.systemd.Journal
This commit adds a new OUTPUT_SKIP_UNPRINTABLE to the OutputFlags
and adds code in `update_json_data` and `json_escape` to honor it.
When set all json fields that have unprintable data will be skipped
and `null` is send instead.
We will need a way to send journal entries as metrics. Those are already
json objects. So Lennart suggested to introduce a new type
METRIC_FAMILY_TYPE_OBJECT that does this. This commit implements
his suggestion.
This commit extracts the initialization of the transient parameters
for io.systemd.Unit.StartTransient into a set of helpers that follow
the _parameters_init() pattern. This way the code is more uniform
and easier to extend and less fragile. It also means there is a
single (logical) place to init the fields.
This commit uses the abstractions added in the previous commit to
add a bunch more properties to the io.systemd.StartTransient()
to showcase how straightforward this is now.
New helpers for tristate bools and an init helper are added. A
dedicated dispatcher for LogLevelMax parses the string-form name
("info", "debug" etc.) declared in the varlink IDL.
The new properties are: DynamicUser, IgnoreSIGPIPE, LockPersonality,
MemoryDenyWriteExecute, NoNewPrivileges, OOMScoreAdjust, RemoveIPC,
RestrictRealtime, RestrictSUIDSGID, RootEphemeral, UMask.
The remaining ProtectKernel*, Private*, ProtectClock properties are
declared as STRING in the varlink IDL (matching the modern *Ex/enum
form) so a bool dispatcher does not pass schema validation. Those
need a string-parsing dispatcher and will be added in a follow-up.
This brings us closer to parity with the D-Bus code (still a long
way to go though).
The handling of the `Exec` parameters for the varlink
`io.systemd.Unit.StartTransient()` became a bit unwieldy. So
this commit creates another abstraction to handle the various
fields in the `Exec` part of the StartTransient code.
Each Exec property is now described by a single TransientExecProperty
entry and adding a new property is just a single entry there plus
an apply function.
Thanks to Ivan Kruglov for many useful suggestions.
Lennart suggested to use a more uniform algorithm for
the picking of the hostname words that is not biased
for long words by just (predictably) randomly going over
the offsets until we land on a word boundary. This is a
very nice suggestion so this commit implements it with
a fallback to the "old" behavior if we do not find a
word boundary within a reasonable amount of attempts.
A small python script shows that 64 iterations plus
fallback is a good number:
```
$ python3 simulate-hostname-pick.py 64
hostname-wordlist/adverbs
words=261 p_accept=0.1119 avg_bytes/word=1/p=8.94
max_iterations=64, n_trials=1000000
fallback rate : 0.051000% (510/1_000_000)
mean seeks per word : 8.93
hostname-wordlist/adjectives
words=449 p_accept=0.1380 avg_bytes/word=1/p=7.24
max_iterations=64, n_trials=1000000
fallback rate : 0.007500% (75/1_000_000)
mean seeks per word : 7.25
hostname-wordlist/nouns
words=449 p_accept=0.1472 avg_bytes/word=1/p=6.79
max_iterations=64, n_trials=1000000
fallback rate : 0.002700% (27/1_000_000)
mean seeks per word : 6.79
```
Combined with the fallback to the previous method if
we can't find anything within the 64 attemps this seems
to be the best tradeoff and give us very good uniformity.
Similar to the support for ?/$ in /etc/hostname and the credentials
we now add this to the kernel commandline systemd.hostname= option.
If the expansion fails, e.g. in the initrd where the word lists (and
possibly the machine ID) are not available yet, the option is ignored
and the usual default hostname logic applies. Once the host system is
up the expansion succeeds and the intended name is applied.
Now that we support the `$` we want to also make this available
inside the system.hostname and firstboot.hostname credentials and
the firstboot --hostname option. This commit adds it (and also `?`).
This commit adds support to /etc/hostname for substitution
of $ wordlists from {/etc,/run,/usr/lib}/systemd/hostname-wordlist.
The first $ will lookup hostname-wordlist/1, the next
hostname-wordlist/2 and so on.
With that we can do a petname [1] style hostname in systemd, e.g.
below a possible expansion for a hostname template:
$-$-$-???? -> wildly-happy-octopus-92a9
The substitution of words is stable (based on machine-id) but
not persisted, it is picked on every boot via a stable file
offset so the operation is cheap. But this means that if the
wordlist changes the hostname would change. The next commit
will add the pattern to the firstboot.hostname credential which
is persistet with the resolved names to avoid this issue.
This also includes a wordlist from the "petname" project
that can be optionally installed.
Thanks to Dustin Kirkland for this wonderful project.
[1] https://github.com/dustinkirkland/petname
We already support exponential backoff for automatic restarts via
RestartSec=/RestartSteps=/RestartMaxDelaySec=, but there is no way to
randomize the restart delay. When many instances of a service fail at
the same time (e.g. because a shared resource briefly went away) they
are all restarted in lockstep, creating a thundering herd problem.
So this commit adds a simple `RestartRandomizedDelaySec=` service
option which is similar to the timer `RandomizedDelaySec=` and
adds a randomized restart delay.
test_wildcard() was never executed: run_testcases() only picks up
functions named testcase_* so this test never ran. This commmit
makes it run and fixes two issues in the test:
1. /etc/hostname is absent in the test image so we need to guard
for that.
2. The pattern check was written as [[ "$P" == "$H" ]] with both
sides quoted, but we need to one side unquoted as otherwise
the comparison will always be false.
The networkd metrics interface already reports a lot of interesting
metrics. With this commit it also report the network addresses too.
Each ready address is emitted as one record per (interface, address)
pair:
- object: ifname
- value: address in CIDR notation
- fields: { family: "ipv4"|"ipv6", scope: "global"|"link"|"host"|... }
The loopback addresses are not reported as its just noise.
Example output:
```
root@localhost:~# varlinkctl --more --json=short call /run/systemd/report/io.systemd.Network io.systemd.Metrics.List '{}'
{"name":"io.systemd.Network.Address","object":"enp0s1","value":"fe80::5054:ff:fe12:3456/64","fields":{"family":"ipv6","scope":"link"}}
{"name":"io.systemd.Network.Address","object":"enp0s1","value":"fec0::5054:ff:fe12:3456/64","fields":{"family":"ipv6","scope":"site"}}
{"name":"io.systemd.Network.Address","object":"enp0s1","value":"10.0.2.15/24","fields":{"family":"ipv4","scope":"global"}}
```
This commit adds the boot timeline (MANAGER_TIMESTAMP_KERNEL/USERSPACE/FINISH) as
metrics. The kernel CLOCK_MONOTONIC value is 0 by definition, so only its
.Realtime is reported. For userspace and finish report both .Realtime and
.Monotonic. The naming follows D-Bus.
This commit moves the io.systemd.Unit.StartTransient tests into
the right place in TEST-74-AUX-UTILS.varlinkctl-unit.sh.
Thanks to Ivan Kruglov for suggesting this.
This commit move the erros in StartTransient from
VARLINK_ERROR_UNIT_BAD_SETTING to SD_VARLINK_ERROR_INVALID_PARAMETER
and it also ensures we have the bad field in the error.
Thanks to Ivan Kruglov for suggesting this.
This commit adds more writable fields to the io.systemd.Unit.StartTransient
varlink method. With this its possible to set:
User,Group,SupplementaryGroups,Nice values.
Plus tests for them.
Just like for the unsupported/bad exec_fields we should show
a message about what field is bad for service parameters. This
commit adds it using the same pattern. The JSON parser works in
fail-fast mode so we only display the first bad field (and
it depends on the parser what it finds first).
The existing error reporting for the varlink `StartTransient` code
was converting all errors into `VARLINK_ERROR_UNIT_BAD_SETTING`.
This is not correct in some cases, we need to have a more targted
pattern here, i.e. only convert EINVAL to VARLINK_ERROR_UNIT_BAD_SETTING
and otherwise return the matching varlink error from the errno instead.
This commit fixes this issue. Thanks to Ivan Kruglov for raising
this.
Extract the SetCredential{,Encrypted} logic out of
bus_exec_context_set_transient_property() into a new helper.
No functional changes.
This will be used in the varlink Unit.StartTransient SetCredential
implementation.
This commit adds support to set `Environment` in the
`io.systemd.Unit.StartTransient` varlink call.
The behavior is similar to D-Bus, i.e. a `null` or `[]` clears the
environment. This is not needed for StartTransient() as there the env
always starts empty but it seems a good property to have if this is
reused.
This commit adds setting the WorkingDirectory to the
`io.systemd.Unit.StartTransient` varlink call. This is a
first step towards more complete StartTransient in varlink.
The goal is to be as close as possible to the D-Bus parameters.
The exception is WorkingDirectory which is an object here so that
we avoid the `-` prefixes and use a more type-safe approach by
making it an explicit `missingOK` parameter.
The key names stay the same as the D-Bus properties (PascalCase).
If there are no equivalent D-Bus properties the native varlink
convention of camelCase is used.
We want to allow partial inputs for the ExecContext when we pass
that to io.systemd.Unit.StartTransient. So this commit makes the
fields nullable. Without this the varlink input validation will
reject partial ExecContext objects.
This is similar to 913b81735e for UnitContext.
So far we had the pattern to check first if any property needs setting
and then have a function to set it. The downside is that when we add
a new property two different places need to change (once the `if`
in vl_method_start_transient_unit() and once in the specific helper
to do the actual setting). So instead this commit moves everything to
the helpers and tweaks the code so that we can always call the function.
In https://github.com/systemd/systemd/pull/41688 we merged metrics
and facts for systemd-report. However while some metric sources
are enabled by default (like `io.systemd.{Manager,Network}`) the
`io.systemd.Basic` service is not enabled by default.
This commit changes this and enables it by default.
We could also enable the systemd-report-cgroup.socket but that sends
a lot more data not sure that is a good default.
We removed the concept of facts, so we need to update the existing
io.systemd.Basic facts provider to metrics. This commit does just
that. Its mostly mechanical.
This also means that facts.{c,h} and varlink-io.systemd.Facts.{c,h}
are gone now.
When a report upload fails the backend often provides useful
details via the varlink error. Show them as part of the upload
error message. For now we just dump the json because we have
no structure that the backends should follow. We may want to
consider adding one (like check for an "error_message" key in
the json). But for now this is a nice step forward.
This commit adds a simple version of io.systemd.Unit.StartTransient
for varlink. It is similar to the dbus version, but there is a key
difference:
1. Instead of building the unit from key/value properties it
takes a structured json object "UnitContext" with a "Service" field
inside.
It is also only implementing a minimal set of what can be done with a
service.
2. No aux units (for now)
3. When called with --more the varlink socket can notify about
state changes depending on the notify{Job,Unit}Changes parameter
This aligns to the json objects/format from
https://github.com/systemd/systemd/pull/39391
and to show how the format can be shared it adds a new
(minimal) `ServiceContext` that is now part of
`io.systemd.Unit.List()`.
When a job reaches the job_uninstall() stage we used to set the
state to JOB_WAITING. However now that we have a JOB_FINISHED state [1]
we should use that instead. This is more accurate so when
varlink_job_send_removed_signal() is called the job is in the expected
state and that is what the user will see.
Note that this does not change the D-Bus API because there
bus_job_send_removed_signal() doesn't send the state, it only sends
the result.
[1] https://github.com/systemd/systemd/pull/41811
This commit adds a simple version of io.systemd.Unit.StartTransient
for varlink. It is similar to the dbus version, but there is a key
difference:
1. Instead of building the unit from key/value properties it
takes a json object in the "service" parameter. It is also
only implementing a minimal set of what can be done with a
service (for now)
2. No aux units (for now)
3. When called with --more the varlink socket can notify about
unit job and state changes controlled via a bool on the
varlink call inputs: notify{Job,Unit}Changes
We use the new io.systemd.Job interface when outputing the
io.systemd.Unit.StartTransient result as it makes the output
nice and mirrors the input.
Note that the property names follow the D-Bus naming to make a
future "systemctl show" transition from D-Bus -> varlink easier.
Because UnitContext is now also used for the inputs we need
to make a bunch of fields `SD_VARLINK_NULLABLE` so that the
input is even accepted. This does not affect the output, it
is still fully populated, just the schema. The ID of UnitContext
is still required.
Thanks to ikruglov and Lennart for their excellent feedback on
this.
Extract the transient unit setup logic from transient_unit_from_message()
in dbus-manager.c into a shared helper.
This prepares for reuse by the varlink StartTransient implementation.
This commit creates a new varlink-io.systemd.Job.c file and puts
the job related varlink types into this file. Those will be
used by the upcoming io.systemd.Unit.StartTransient.
Note that the property names follow the D-Bus naming to make a
future "systemctl show" transition from D-Bus -> varlink easier.
Thanks to @ikruglov for suggesting this preparation commit and
to Lennart for suggesting the D-Bus compatibility considerations.
We have code like `size_t max_size = sizeof(*private)` in three
places. This is evaluated at compile time so its safe to use. However
the new pointer-deref checker in coccinelle is not smart enough to know
this and will flag those as errors. To avoid these false positives
we have some options:
1. Reorder so that we do:
```C
size_t max_size = 0;
assert(private);
max_size = sizeof(*private);
```
2. Use something like `size_t max_size = sizeof(*ASSERT_PTR(private));`
3. Place the assert before the declaration
4. Workaround coccinelle via SIZEOF(*private) that we can then hide
via parsing_hacks.h
5. Fix coccinelle (OCaml, hard)
6. ... somehting I missed?
None of these is very appealing. I went for (4) but happy about
suggestions.
We have a test failure where the testsuite is calling
sd_varlink_idl_parse() with *ret being NULL. This is now an
assert error. So we could either fix the test or fix the code
Given that it seems genuinely useful to run sd_varlink_idl_parse()
without *ret to e.g. just check if the idl is valid I opted to
fix the code.
The updated parsing_hacks.h file uncovered a bunch of extra
things that the check-pointer-deref coccinelle script flags.
This commit fixes them to make the tree check-pointer-deref clean.
This commit runs the check-coccinelle checker scripts with the
parsing_hacks.h. Because this was missing before there were some
issues that did not get flagged.
While at it it also adds some missing cleanup attributes and
iterators to get better results. Its a bit sad that there is no
(easy/obvious) way to detect when new things are needed for
parsing_hacks.h
When there is a potential protocol upgrade we need to be careful that
we do not read beyond our json message as the custom protocol may be
anything. This was archived via a byte-by-byte read. This is of course
very inefficient. So this commit moves to use MSG_PEEK to find the
boundary of the json message instead. This makes the performance hit
a lot smaller.
Thanks to Lennart for suggesting this.
When the server side of a varlink connection supports connection
upgrades we need to go into single byte-read mode to avoid the
risk of a client that sends the json to protocol upgrade and then
immediately the custom protocol payload. This commit implements
this.
The next step is using MSG_PEEK to avoid the single-byte overhead.
This commit tweaks the API of sd_varlink_call_and_upgrade and
sd_varlink_reply_and_upgrade to return two independent fds even
if the internal {input,output}_fd are the same (e.g. a socket).
This makes the external API easier as there is no longer the risk
of double close. The sd_varlink_call_and_upgrade() is not in a
released version of systemd yet so I presume it is okay to update
it still.
This also allowed some simplifications in varlinkctl.c now that
the handling is easier.
With the new protocol upgrade support in varlinkctl client we can
now do the equivalent for the server side. This commit adds a new
`serve` verb that will serve any command that speaks stdin/stdout
via varlink and its protocol upgrade feature. This is the
"inetd for varlink".
This is useful for various reasons:
1. Allows to e.g. provide a heavily sandboxed io.myorg.xz.Decompress
varlink endpoint, c.f. xz CVE-2024-3094)
2. Allow sftp over varlink which is quite useful with the
varlink-http-bridge (that has more flexible auth mechanism than
plain sftp).
3. Makes testing the varlinkctl client protocol upgrade simpler.
4. Because we can.
Extract the fd-handling logic from sd_varlink_call_and_upgrade() into a
shared static helper so that it can be reused by the upcoming server-side
sd_varlink_reply_and_upgrade().
The io.systemd.Manager.{PowerOff,SoftReboot,Halt,Kexec} manager
varlink and bus methods log the peer ID when calling shutdown.
The logind code is missing this, so this commit adds a similar
logging now. The code is quite similar to the one in existing in
src/core/manager.c but its hard to share code so this adds a bit
of duplication.
To properly support a shutdown interface with varlink we need
the functionality of verify_shutdown_creds(). This is currently
dbus only. There are some options:
1. refactor and abstract so that verify_shutdown_creds() is agnostic
2. provide a equivalent function with varlink
3. allow to call it with either a dbus or a varlink message.
The most elegant of course is (1) but it makes reviewing harder
and has a higher regression risk. It will also be more code.
Doing (2) has the risk of drift, i.e. we will need to keep
the two functions in sync and not forget about it ever.
So this commit opts for (3): allowing either dbus or varlink.
This is quite ugly, however the big advantage is that its very
simple to review as the dbus/varlink branches mirror each other.
And there is no risk of drift the dbus/varlink options are
close to each other. It unlikely we get a third bus, so it will
most likely stay this way. It is still ugly though so I can
understand if this is undesired and I can look into (1) if its
too ugly.
With this function avaialble logind-varlink.c is now updated to
use it.
Move verify_shutdown_creds() and its helper have_multiple_sessions()
from logind-dbus.c to logind-shutdown.c so that they can be reused
by the varlink transport. No functional changes.
Also prefix both with `manager_` now that they are public.
The shutdown interface is currently only exposed via dbus. This commit
adds a comparable varlink implementation. It is inspired by the existing
dbus methods and implements PowerOff, Reboot, Halt, Kexec, SoftReboot.
It is (intentional) simpler than the dbus for now, i.e. strictly root
only. To match dbus we will need to the functionality
of verify_shutdown_creds() which is dbus-ish right now and would
need some refactor.
For the same reason it does not do the Can* methods - we will need
the verify_shutdown_creds() equivalent first.
This function operates on generic Manager state and will be needed
by the varlink shutdown interface too. Move it out of logind-dbus.c
into a new logind-shutdown.c, alongside the SHUTDOWN_SCHEDULE_FILE
define and use `manager_reset_scheduled_shutdown() as the new
name.
This commit tweaks the TEST-74-AUX-UTILS.varlinkctl.sh code
to use `systemd-notify --fork $UPGRADE_SERVER` instead of
the (ugly) timeout.
This also fixes a stale comment in around
`Test --upgrade with stdin redirected from a regular file`.
Thanks to Daan for suggesting this!
Instead of exiting in exec_with_listen_fds() just return an error
and do the actual _exit() in the caller. Much nicer this way.
Thanks for Lennart for suggesting this.
This commit fixes an issue with the fd handling in
sd_varlink_call_and_upgrade() when one direction of the output
FDs is unset.
Thanks to Lennart for spotting this and suggesting the fix.
The shared/ dns_packet_read_uint*() functions used to be a bit
inconsistent when it comes to the handling of NULL in *ret. The
helpers dns_packet_read_{uint16,name} accepted NULL but the
dns_packet_read_uint{8,32} did not.
This commit makes them all symmetric in accepting NULL.
Some functions are in the hot path of a sort. While it is more
correct to also use assert() here for the passed pointers it is
also a runtime cost that should never be needed when e.g. qsort()
is used as it will always pass non-NULL pointers.
So for these critical functions use POINTER_MAY_BE_NULL() which
is compiled out and add a comment to them. Not all _compare()
functions are changed, it was a bit of a judgement call because
some (like ether_addr_compare) are not called with that much data.
But happy to get advise here and tweak accordingly.
Add the needed assert changes to make the code clean
for the new check-pointer-deref script.
Note that for `_cmp()` functions its a bit of a question
if we want `assert()` or `POINTER_MAY_BE_NULL()` here. The
former is more correct but it adds a (small) runtime hit. If
we use POINTER_MAY_BE_NULL() it is compiled out but strictly
speaking it is wrong (so at least we would need a comment).
There are some unnecessary() if() checks for variables
that are already guarded by assert()s. Drop those.
Thanks to keszybz for clarifying the direction of this commit.
The previous code used strv_join() when it generated the log
message for `varlinkctl --exec`. However this can lead to
inaccurate logging so use `quote_command_line()` instead.
Thanks to Lennart for suggesting this.
Having support for `--exec` when using `--upgrade` is nice so this
commit adds it. It does it by extracting a shared helper called
`exec_with_listen_fds()` and then use that in the `verb_call()`
and `varlink_call_and_upgrade()` calls.
The varlink spec supports protocol upgrades and they are very
useful to e.g. transfer binary data directly via varlink. So
far varlinkctl/sd-varlink was not supporting this. This commit
adds support for it in varlinkctl by using the new code in
sd-varlink and the generalized socket-forward code.
The SimplexForwader was using the naming of the SocketForwarder
for bi-directional sockets. This was to keep the diff small and
to make it easier to follow what changed and what was reused.
However the name "client/server" for the SimplexForwader does
no longer make much sense. The SimplexForwader is no longer
about client/server but really just read/write. So this commit
adjusts the naming.
Now that the socket forward code is extracted we can
extend it to not just support bidirectional sockets
but also input/output fd-pairs. This will be needed
for e.g. the varlinkctl protocol upgrade support where
one side of the connection is a fd-pair (stdin/stdout).
This is done by creating two half-duplex forwarders
that operate independantly. This also allows to simplify
some state tracking, e.g. because each fd serves only one
direction we don't need to dynamically create the event mask
with EPOLLIN etc, its enough to set it once. It also handles
non-pollable FDs transparently.
Thanks to Lennart for his excellent suggestions here.
The varlink spec supports protocol upgrades and they are very
useful to e.g. transfer binary data directly via varlink. So
far sd-varlink was not supporting this.
This commit adds a new public sd_varlink_call_and_upgrade()
that sends a method call, waits for the reply, then steals
the connection fds for raw I/O. It returns separate input_fd
and output_fd to support both bidirectional sockets and pipe
pairs.
A helper is extracted and shared between sd_varlink_call_full()
and sd_varlink_call_and_upgrade(). A new `protocol_upgrade`
bool in `struct sd_varlink` ensures that on a protocol upgrade
request we only exactly read the varlink protocol bytes and
leave anything beyond that to the caller that speaks the upgraded
protocol.
Note that this is the client side of the library implementation
only for now. The server side needs work but this is already
useful as it allows to talk to varlink servers that speak protocol
upgrades (like the rust implemenations of varlink).
Trivial ordering/modernizing change that got highlighted by
claude and refined by keszybz to move to the modern systemd
style.
Thanks to keszybz for suggesting this.
For some of the directories it makes more sense to keep them
excluded from the coccinelle check. Specifically:
- libc: compatibility, no asserts or systemd headers yet
- test: uses NUL internally to test crashes etc
Our coccinelle/check-pointer-deref.cocci checker has a special
case for `assert(pidref_is_set(param))`. It turns out we can
generalize this and catch the following:
- iovec_is_set
- sd_dhcp_duid_is_set
- sd_dhcp_client_id_is_set
The fix in 8f1751a111 made me wonder if we could automatically detect
when pointers are accessed but when this might not be safe. Systemd
is already using a lot of `assert(dst)` and this change now forces
us to use them.
So this commit (ab)uses coccinelle to flag any pointer parameter
dereference not preceded by assert(param), ASSERT_PTR(param), or an
explicit NULL check. It adds integration into meson as a new "coccinelle"
test suite (just like clang-tidy) and is run in CI. The check is not
perfect but seems a reasonable heuristic.
For this RFC commit it is scoped to a subset, it excludes 25 dirs right
now and includes around 100. About 300 warnings left. Busywork that I am
happy to do if there is agreement that it is worth it.
With this in place we would have caught the bug from 8f1751a111 in CI:
```
FAIL: check-pointer-deref.cocci found issues in systemd/src/boot:
diff -u -p systemd/src/boot/measure.c /tmp/nothing/measure.c
--- systemd/src/boot/measure.c
+++ /tmp/nothing/measure.c
@@ -312,7 +312,6 @@ EFI_STATUS tpm_log_tagged_event(
if (err != EFI_SUCCESS)
return err;
- *ret_measured = true;
return EFI_SUCCESS;
}
```
This also adds a new POINTER_MAY_BE_NULL() for the cases when the
called function will do the NULL check (like `iovec_is_set()`).
The comment about `SD_BUS_VTABLE_CAPABILITY(CAP_SYS_BOOT)` is
incorrect. To quote @YHNdnzj:
```
nah, the capability-based permission model is a legacy from kdbus. We cannot do it race-freely without it. Please simply drop the comment.
```
Thanks to @YHNdnzj for suggesting this.
The manager_do_set_objective() was doing a bunch of work to
check the `root` path that can already be done via
`json_dispatch_path` so instead of duplicating use the helper.
Thanks to @YHNdnzj for suggesting this.
Drop the _manager suffix for vl_method_{poweroff,{soft,}reboot,
halt,kexec}_manager.
They're unambiguous enough and the power mgmt operations are
not manager-wide but system-wide.
Thanks to @YHNdnzj for suggesting this.
External tools that use the systemd varlink ecosystem require
to know if a specific varlink method supports/requires the
"more" flag from the IDL. This is tracked upstream in
https://github.com/varlink/varlink.github.io/issues/26
As an intermediate step systemd adds the (very nice) comments
```
# [Requires 'more' flag]
or
# [Supports 'more' flag]
```
to the various methods.
This commit extends the comment around the code that adds the
comment to clarify that this should be considered API and that
the comment should not be changed as external tools (like e.g.
the varlink-http-bridge) rely on it.
Michael Vogt