This doesn't really change much, but feels more correct to do, as it
ensures that all messages currently queued in the bus connections are
definitely unreffed and thus destryoing of the connection object will
follow immediately.
Strictly speaking this change is entirely unnecessary, since nothing
else could have acquired a ref to the connection and queued a message
in, however, now that we have the new sd_bus_close_unref() helper it
makes a lot of sense to use it here, to ensure that whatever happens
nothing that might have been queued fucks with us.
It's similar to sd_bus_flush_close_unref() but doesn't do the flushing.
This is useful since this will still discnnect the connection properly
but not synchronously wait for the peer to take our messages.
Primary usecase is within _cleanup_() expressions where synchronously
waiting on the peer is not OK.
Ideally, coccinelle would strip unnecessary braces too. But I do not see any
option in coccinelle for this, so instead, I edited the patch text using
search&replace to remove the braces. Unfortunately this is not fully automatic,
in particular it didn't deal well with if-else-if-else blocks and ifdefs, so
there is an increased likelikehood be some bugs in such spots.
I also removed part of the patch that coccinelle generated for udev, where we
returns -1 for failure. This should be fixed independently.
Let's be more careful with what we serialize: let's ensure we never
serialize strings that are longer than LONG_LINE_MAX, so that we know we
can read them back with read_line(…, LONG_LINE_MAX, …) safely.
In order to implement this all serialization functions are move to
serialize.[ch], and internally will do line size checks. We'd rather
skip a serialization line (with a loud warning) than write an overly
long line out. Of course, this is just a second level protection, after
all the data we serialize shouldn't be this long in the first place.
While we are at it also clean up logging: while serializing make sure to
always log about errors immediately. Also, (void)ify all calls we don't
expect errors in (or catch errors as part of the general
fflush_and_check() at the end.
These lines are generally out-of-date, incomplete and unnecessary. With
SPDX and git repository much more accurate and fine grained information
about licensing and authorship is available, hence let's drop the
per-file copyright notice. Of course, removing copyright lines of others
is problematic, hence this commit only removes my own lines and leaves
all others untouched. It might be nicer if sooner or later those could
go away too, making git the only and accurate source of authorship
information.
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.
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.
sd_bus_open/sd_bus_open_system/sd_bus_open_user are convenient, but
don't allow the description to be set. After they return, the bus is
is already started, and sd_bus_set_description() fails with -EBUSY.
It would be possible to allow sd_bus_set_description() to update the
description "live", but messages are already emitted from sd_bus_open
functions, so it's better to allow the description to be set in
sd_bus_open/sd_bus_open_system/sd_bus_open_user.
Fixes message like:
Bus n/a: changing state UNSET → OPENING
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)
We maintain a queue of units and jobs that we are supposed to generate
change/new notifications for because they were either just created or
some of their property has changed. Let's throttle processing of this
queue a bit: as soon as > 1K of bus messages are queued for writing
let's skip processing the queue, and then recheck on the next
iteration again.
Moreover, never process more than 100 units in one go, return to the
event loop after that. Both limits together should put effective limits
on both space and time usage of the function, delaying further
operations until a later moment, when the queue is empty or the the
event loop is sufficiently idle again.
This should keep the number of generated messages much lower than
before on busy systems or where some client is hanging.
Note that this also means a bad client can slow down message dispatching
substantially for up to 90s if it likes to, for all clients. But that
should be acceptable as we only allow trusted bus clients, anyway.
Fixes: #8166
This is an optimization: there's no point in enqueuing unit and job
change notificiation signal messages into bus connection that aren't
fully set up yet.
This doesn't fix#8166 but should lower the load of messages enqueued
but not processed yet a bit.
This corrects the control flow: when we reuse the API bus as system bus,
let's definitely invoke bus_init_system() too, so that it is called
regardless how we acquired the bus object.
(Note that this doesn't actually change anything, as we only inherit the
bus like this in system mode, and bus_init_system() doesn't do anything
in system bus, besides writing a log message)
Let's better be safe than sorry, and validate that api_bus is not NULL
before we send messages to it. Of course, strictly speaking this
shouldn't actually be necessary, as the tracker object should not exist
without the bus, but let's be extra sure.
Previously, we'd synchronize bus names immediately when we succeeded
connecting to the bus, potentially even before coldplugging the units.
This was problematic, as synchronizing bus names meant invoking the
per-unit name change handler function which might change the unit's
state — which will result in consistency when done before we coldplug
things.
With this change we instead enqueue a job for the event loop to resync
the names in a later loop iteration, i.e. at a point where we know
coldplugging has finished.
This removes the current bus_init() call, as it had multiple problems:
it munged handling of the three bus connections we care about (private,
"api" and system) into one, even though the conditions when which was
ready are very different. It also added redundant logging, as the
individual calls it called all logged on their own anyway.
The three calls bus_init_api(), bus_init_private() and bus_init_system()
are now made public. A new call manager_dbus_is_running() is added that
works much like manager_journal_is_running() and is a lot more careful
when checking whether dbus is around. Optionally it checks the unit's
deserialized_state rather than state, in order to accomodate for cases
where we cant to connect to the bus before deserializing the
"subscribed" list, before coldplugging the units.
manager_recheck_dbus() is added, that works a lot like
manager_recheck_journal() and is invoked in unit_notify(), i.e. when
units change state.
All in all this should make handling a bit more alike to journal
handling, and it also fixes one major bug: when running in user mode
we'll now connect to the system bus early on, without conditionalizing
this in anyway.
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)
The aim of this change is to make sure that we properly log about all
D-Bus connection problems. After all, we only ever attempt to get on the
bus if dbus-daemon is around, so any failure in the process should be
treated as an error.
bus_init_system() is only called from bus_init() and in
bus_init() we have a bool flag which governs whether we should attempt
to connect to the system bus or not.
Hence if we are in bus_init_system() then it is clear we got called from
a context where connection to the bus is actually required and therefore
shouldn't be treated as the "best effort" type of operation. Same
applies to bus_init_api().
We make use of those error codes in bus_init() and log high level
message that informs admin about what is going on (and is easy to spot
and makes sense to an end user).
Also "retrying later" bit is actually a lie. We won't retry unless we
are explicitly told to reconnect via SIGUSR1 or re-executed. This is
because bus_init() is always called from the context where dbus-daemon
is already around and hence bus_init() won't be called again from
unit_notify().
Fixes#7782
This way, clients can install the very same match on direct and broker
connections as in both cases the messages will originate from the o.f.s1
service.
Let's remove a number of synchronization points from our service
startups: let's drop synchronous match installation, and let's opt for
asynchronous instead.
Also, let's use sd_bus_match_signal() instead of sd_bus_add_match()
where we can.
We'd like to use inotify to get notified when AF_UNIX sockets become
connectable. That happens at the moment of listen(), but this is doesn't
necessarily create in a watchable inotify event. Hence, let's synthesize
one whenever we generically create a socket, or when we know we created
it for a D-Bus server.
Ideally we wouldn't have to do this, and the kernel would generate an
event anyway for this. Doing this explicitly isn't too bad however, as
the event is still nicely associated with the AF_UNIX socket node, and
we generate all D-Bus sockets in our code hence it's safe.
The advantage is that is the name is mispellt, cpp will warn us.
$ git grep -Ee "conf.set\('(HAVE|ENABLE)_" -l|xargs sed -r -i "s/conf.set\('(HAVE|ENABLE)_/conf.set10('\1_/"
$ git grep -Ee '#ifn?def (HAVE|ENABLE)' -l|xargs sed -r -i 's/#ifdef (HAVE|ENABLE)/#if \1/; s/#ifndef (HAVE|ENABLE)/#if ! \1/;'
$ git grep -Ee 'if.*defined\(HAVE' -l|xargs sed -i -r 's/defined\((HAVE_[A-Z0-9_]*)\)/\1/g'
$ git grep -Ee 'if.*defined\(ENABLE' -l|xargs sed -i -r 's/defined\((ENABLE_[A-Z0-9_]*)\)/\1/g'
+ manual changes to meson.build
squash! build-sys: use #if Y instead of #ifdef Y everywhere
v2:
- fix incorrect setting of HAVE_LIBIDN2
If dbus is already down during shutdown, we can't propagate the cgroup
release message anymore, but that's expected and nothing to warn about.
Hence let's downgrade the message from LOG_WARN to LOG_DEBUG.
Fixes: #6777
This moves pretty much all uses of getpid() over to getpid_raw(). I
didn't specifically check whether the optimization is worth it for each
replacement, but in order to keep things simple and systematic I
switched over everything at once.
manager_sync_bus_names() function retrieves the dbus names
and compares it with unit bus names. It could be right
after the list is retrieved, the dbus peer is disconnected.
In this case it is really not an ERROR print if
sd_bus_get_name_creds() or sd_bus_creds_get_unique_name()
fail.
src/core/dbus.c: In function 'find_unit':
src/core/dbus.c:334:15: warning: 'u' may be used uninitialized in this function [-Wmaybe-uninitialized]
*unit = u;
^
src/core/dbus.c:301:15: note: 'u' was declared here
Unit *u;
^
Ben Boeckel