Add dbus-announce plugin #1255

ffesti · 2020-06-04T10:10:21Z

The plugin announces start and completion of transactions and
the installation and erasure of packages.

The patch still needs some polishing:

Names of the DBus enities may need a second thought
Man page is still missing a description of the dbus signal sent
DBus config is not properly integrated into Makefile

Resolves: #1249

pmatilai · 2020-06-04T10:35:39Z

I'm starting to think we might actually be better off without sending notifications for individual packages, because this will be noisy, and doubly so since the psm hooks can get called multiple times per package as you noted. Just announce that transaction is in progress and when it ends. Which makes me wonder if we couldn't actually merge this with the inhibit plugin somehow, as then beginning and end of the transaction is the most interesting thing there.

pmatilai · 2020-06-04T10:47:29Z

...noisy and like you said in the ticket, we don't want people to start building their own triggers outside rpm. Information about the contents of a transaction in progress is probably best limited to the invoking process.

ffesti · 2020-06-04T11:29:09Z

Yeah, I had the package info as a separate patch, but I figured it'd be easier to just delete the functions than splitting all the changes and fixes.
Otoh I could imaging some GUI tool to not blocking it's UI completely on an running transaction but instead just changing package by package.

pmatilai · 2020-06-05T06:20:35Z

Having slept over it: the thing is that the packages aren't actually installed/removed and the result code until the transaction is over, we really must not advertise the inconsistent state to others. The syslog plugin gets this wrong too...

I'd really prefer starting with just the transaction begin/end signals, the case for individual package signals is much muddier. It's always much easier to add than remove.

ffesti · 2020-06-05T11:45:59Z

Split the package signals into a separate patch as they need some deeper discussion on how the plugin hooks should actually work and what new we need to add (see #1254).

plugins/dbus_announce.c

cgwalters · 2020-06-08T17:54:39Z

Is there any coordination between this and the work to add dbus to libdnf in rpm-software-management/libdnf#941 for example?

For rpm-ostree we are already a DBus daemon, and having multiple other libraries in the stack also going out and talking to DBus is going to be a bit problematic.

Binding this to the systemd-inhibit plugin makes sense because we already turn that off for rpm-ostree (because it's transactional, there's no reason to inhibit).

doc/rpm-plugin-dbus-announce.8

ffesti · 2020-06-09T08:11:02Z

Is there any coordination between this and the work to add dbus to libdnf in rpm-software-management/libdnf#941 for example?

Well, we add this on request of the DNF team that need to be notified if something else (rpm cli) changes the rpmdb underneath their daemon. Note that this is very different from the lib dnf thing even if it also uses DBus. RPM will not offer a DBUS interface that can be queried or send commands to. All this does is sending out signals that notify who ever is interested that the rpmdb is changing.
I will probably even drop the second patch providing more detailed information until there are real use cases. So all this is going to do is sending out a ping at the start and the end of every transaction.

For rpm-ostree we are already a DBus daemon, and having multiple other libraries in the stack also going out and talking to DBus is going to be a bit problematic.
Binding this to the systemd-inhibit plugin makes sense because we already turn that off for rpm-ostree (because it's transactional, there's no reason to inhibit).

So this is going to be a plugin that can be disabled or just plainly not be installed in the first place. We will probably not integrate this into the systemd-inhibit plugin but have it as a separate plugin that is very similar and comes in it's own sub package.

Can you please elaborate on why sending some DBus signals should be problematic?

pmatilai · 2020-06-10T09:55:53Z

plugins/dbus_announce.c

+
+	    state->bus = dbus_bus_get_private(DBUS_BUS_SYSTEM, &err);
+	    if (dbus_error_is_set(&err)) {
+		fprintf(stderr, "Connection Error (%s)\n", err.message);


This should be filtered out in case dbus isn't running to begin with, similarly to what commit 708e613 does. Also, rpmlog() should be used instead of fprintf().

plugins/dbus_announce.c

pmatilai · 2020-06-10T10:03:03Z

plugins/dbus_announce.c

+        }
+    }
+
+    return RPMRC_NOTFOUND;


The return code logic is confusing, with multiple returns in space of just a few lines.

plugins/dbus_announce.c

pmatilai · 2020-06-10T10:14:56Z

plugins/dbus_announce.c

+    dbcookie = _free(dbcookie);
+
+    if (!dbus_connection_send(state->bus, msg, &serial)) {
+	return RPMRC_FAIL;


Hmm, failing the entire transaction if dbus announce fails seems a bit dramatic. Then again, if people are actually relying on these signals...

pmatilai · 2020-06-10T10:15:25Z

plugins/dbus_announce.c

+
+    dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+    if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, &dbcookie)) {
+	return RPMRC_FAIL;


This leaks memory in the failure case.
In general, the "goto err" idiom with a single exit point for cleanup is preferred over multiple returns, exactly for reasons like this.

Probably should also add the transaction ID to these messages.

plugins/dbus_announce.c

cgwalters · 2020-06-10T13:48:03Z

Well, we add this on request of the DNF team that need to be notified if something else (rpm cli) changes the rpmdb underneath their daemon.

Ah, I see. Well, nothing else can/should be changing the rpmdb on rpm-ostree based systems (see also coreos/rpm-ostree#1789 where rpm-ostree will start wrapping /usr/bin/rpm).

So this is going to be a plugin that can be disabled or just plainly not be installed in the first place. We will probably not integrate this into the systemd-inhibit plugin but have it as a separate plugin that is very similar and comes in it's own sub package.

OK that seems fine, then we can not install it in the container image and rpm-ostree based systems.

Can you please elaborate on why sending some DBus signals should be problematic?

A few things. First, rpm-ostree updates are offline (generating a new root) by default - so anything listening and responding to these DBus signals would be confused because it's not affecting the current rpm database but a new one it can't see.

Second, and related to that - which exact component would be listening to these signals? Would it be dnf or libdnf? I really hope it's not the library because then rpm-ostree would be talking to itself over DBus which...yeah I would like not to have that happen 😄

Do we really need DBus for this versus just e.g. having interested processes use inotify() on the path to the rpmdb's directory, and ensuring that RPM does touch(/path/to/rpmdb) whenever it makes a change? This is basically what we do for ostree changes. (And yes in some cases rpm-ostreed does catch inotify updates from its own changes but that's kind of intentional and much less heavy-weight than talking to oneself over dbus)

pmatilai · 2020-06-11T05:03:59Z

This isn't just for dnf's benefit. It's a mechanism that anybody who needs to do so is free to listen, and everybody else can ignore.

cgwalters · 2020-06-17T02:05:21Z

This isn't just for dnf's benefit. It's a mechanism that anybody who needs to do so is free to listen, and everybody else can ignore.

Speaking as a former DBus maintainer upstream here and I have written many DBus APIs...

It seems dramatically simpler to me to go with the inotify approach, the requirement is basically for librpm to touch the toplevel directory of the database path. It's near zero cost if there is nothing listening.

And most crucially it avoids creating a new API for things to try to query/monitor the rpm database state. There's already an API for that via librpm. This signal isn't extensible - if e.g. a new field (say something related to modules) becomes interesting then we'd have to define a new one. Handling that requires going down to a "bag of properties" i.e. a{sv} in DBus type code model. (We do this in many places in rpm-ostree) - but it loses some of the elegance.

A DBus API is a serious commitment, particularly once you have things listening to it.

That said a particular strength of DBus (really the reason it was created) is to be able to to cross privilege boundaries by having unprivileged processes (originally the desktop) sanely monitor and interact with system services. But in this case since the consumer is (theoretically) dnf they're in the same privilege level (and in fact the same process!), so that doesn't apply.

FWIW here's the libostree code to bump mtime: https://github.com/ostreedev/ostree/blob/62632511f149adfc186da7f0e976006845591c8f/src/libostree/ostree-sysroot.c#L357

And here's where rpm-ostree uses it: https://github.com/coreos/rpm-ostree/blob/e6907d209b258388339b26e58a6da8cd022d1081/src/daemon/rpmostreed-sysroot.c#L788

That's how rpm-ostreed gets notified if e.g. an admin directly runs ostree admin <foo>, which is a lot like the situation where someone is using a traditional rpm system with a DBus daemon like dnfdragora/PackageKit or whatever and an admin uses rpm -ivh or so.

pmatilai · 2020-06-17T06:47:23Z

@cgwalters , the use-case is not dnf itself but a daemon which will need to refresh it's view of rpmdb whenever someone else changes the rpmdb. See #1124 for some background. Also one thing worth remembering is that inotify is a Linux-specific interface, but rpm is portable software. While adding system-specific features is okay, exposing them via public API creates its own set of problems.

Thanks for the insights on DBus.

mlschroe · 2020-06-17T09:02:19Z

(Colin, see issue #1124 for a solution using a named pipe)

pmatilai · 2020-06-25T06:25:49Z

One important aspect of dbus vs librpm API is that not everybody wants to link to librpm, which imposes its own signal handling on users when the db is open, and our ABI hasn't been particularly stable historically, dbus library itself is much more stable AIUI.

cgwalters · 2020-06-25T13:18:35Z

imposes its own signal handling on users when the db is open

Can turn that off since 56f49d7 right?

and our ABI hasn't been particularly stable historically

Maintaining C shared libraries is indeed extremely hard. I've messed up in the past and only just barely caught it before releases etc. That said there are nice tools now like libabigail - should be easy to set that up on CI here right?
(I still need to do that for ostree)

dbus library itself is much more stable AIUI.

Yeah, libdbus as a C shared library has been fine and will continue to be so, but exposing an API over DBus is a separate thing with long term commitments.

If we're effectively saying most projects shouldn't be using librpm to read the rpm database effectively (that seems like what you're implying) that's a rather radical thing right? Maybe I am misunderstanding though.

pmatilai · 2020-06-26T07:05:57Z

imposes its own signal handling on users when the db is open

Can turn that off since 56f49d7 right?

API users turning off signal handling is not a solution, it's a problem in its own right.

and our ABI hasn't been particularly stable historically

Maintaining C shared libraries is indeed extremely hard. I've messed up in the past and only just barely caught it before releases etc. That said there are nice tools now like libabigail - should be easy to set that up on CI here right?
(I still need to do that for ostree)

librpm ABI stability or lack of thereof has little to do about it being difficult, it's merely that rpm has historically exported all manner of crap that it never should have, and it's not possible to get rid off it all in one go, unfortunately. So for 12+ years we've done staged "exit accumulated crap, bump soname" releases every now and then, rpm soname bumps are an absolute PITA.

dbus library itself is much more stable AIUI.

Yeah, libdbus as a C shared library has been fine and will continue to be so, but exposing an API over DBus is a separate thing with long term commitments.

Eh, no kidding?

If we're effectively saying most projects shouldn't be using librpm to read the rpm database effectively (that seems like what you're implying) that's a rather radical thing right? Maybe I am misunderstanding though.

I said no such thing. Different users have different priorities and needs, projects should (be able to) use what works best for them.

Linking to librpm comes with heavy and hard to understand babbage and is way too intimate (think "bare metal") for various "casual" users. A DBus API puts a much needed insulation layer between the process that actually accesses rpmdb and the unsuspecting piece of software wanting to know something about packages on the system. With the existing librpm API, very weird things happen in dark corners such as gdb running as root on a 32bit process on a 64bit system does an rpmdb query for debuginfo discovery.

m-blaha · 2020-11-24T07:54:40Z

From dnf-daemon point of view this plugin looks good and definitely useful. I've just build rpm with this plugin enabled and I'm going to test how it works. Thanks Florian!

m-blaha · 2020-11-25T10:11:34Z

I did some testing in dnfdaemon and the patch is working for us. Just (as Panu mentioned) for consistency I'd like to change CompleteTransaction signal name to EndTransaction - the signal is emitted regardless the transaction was successful and "Complete" in the name suggests the success.
I was also thinking whether the result of the transaction should be part of the signal but having rpmdb cookie is perfectly fine with us to decide whether there actually was any change.
I suppose the plugin will be released as a separate sub-package so that the dnfdaemon could depend on it.

pmatilai · 2021-02-02T11:17:32Z

There are quite a few unaddressed review issues from the previous round still present.
Please mention what changed when pushing new versions, the way GH works makes it quite impossible to track from the code itself.

ffesti · 2021-02-16T07:45:36Z

While many smaller things are fixed now. This still needs a run through the error handling.

ffesti · 2021-05-03T15:07:04Z

OK, now there should be all issues raised so far being addressed. The plugin now only gives a warning via rpmlog if the connection to dbus cannot be obtained. From my POV this can be merged by squashing all commit into the first one - or I can squash them first if that is prefered.

plugins/dbus_announce.c

pmatilai · 2021-05-25T08:57:07Z

plugins/dbus_announce.c

+    if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, &dbcookie)) {
+	goto err;
+    }
+    dbcookie = _free(dbcookie);


dbcookie will leak if dbus_message_append_args() fails. The generally fail-safe strategy is to initialize to NULL and free at central exit point than try be clever with local allocations.

Plugged the leak. Still not a text book example of the central exit point pattern...

plugins/dbus_announce.c

pmatilai · 2021-05-25T09:04:28Z

plugins/dbus_announce.c

+    state->bus = dbus_bus_get_private(DBUS_BUS_SYSTEM, &err);
+    if (dbus_error_is_set(&err)) {
+	goto err;
+    }


There's no strict guideline whether to use {} on a one-liner block, but please pick a strategy and use it consistently. There are multiple cases where braces are not used (eg a few lines above), and many cases where they are not, throughout this file.

I personally tend to avoid the the excess {} unless it's needed for readability.

Deleted lots of { and }.

plugins/dbus_announce.c

ffesti · 2021-05-26T13:34:02Z

Rebased to current master
Switched man page over to new docs pattern (markdown, doc->docs move)
Squashed commits into one
Addressed above comments.

pmatilai · 2021-06-02T07:54:26Z

plugins/dbus_announce.c

+	goto err;
+
+    dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+    if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, &dbcookie))


Sorry for not realizing this earlier: we really should add transaction ID to these messages.

pmatilai · 2021-06-02T07:57:04Z

plugins/dbus_announce.c

+{
+    int rc;
+
+    rc = open_dbus(plugin, ts);


Just cosmetics, but you could save a couple of lines by initializing on the declaration line.

The plugin announces start and end of transactions

ffesti · 2021-07-13T11:39:26Z

Added transaction id to the message and added a description of the data send in the man page.

pmatilai

I guess we've stared at the code long enough, lets get some real-world experience next...

ffesti force-pushed the dbus_announce branch from 7391911 to 060eaf8 Compare June 4, 2020 10:13

ffesti added the RFC label Jun 4, 2020

ffesti force-pushed the dbus_announce branch from 060eaf8 to 46897a3 Compare June 5, 2020 11:44

mlschroe reviewed Jun 8, 2020

View reviewed changes