subject:"\[Rpm\-maint\] \[rpm\-software\-management\/rpm\] Add dbus\-announce plugin \(#1255\)"

@ffesti commented on this pull request.



> +if (!state->bus) {
+   return RPMRC_OK;
+}
+
+msg = dbus_message_new_signal("/org/rpm/Transaction", /* object name */
+ "org.rpm.Transaction",  /* interface name */
+ name);  /* signal name */
+if (NULL == msg) {
+   goto err;
+}
+
+dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, )) {
+   goto err;
+}
+dbcookie = _free(dbcookie);

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r639731889___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-848774248___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> +  rpmts ts,
+int res)
+{
+struct dbus_announce_data * state = rpmPluginGetData(plugin);
+DBusMessage* msg;
+dbus_uint32_t serial = 0;
+char * dbcookie;
+
+if (!state->bus) {
+   return RPMRC_OK;
+}
+
+msg = dbus_message_new_signal("/org/rpm/Transaction", /* object name */
+ "org.rpm.Transaction",  /* interface name */
+ name);  /* signal name */
+if (NULL == msg) {

switched order

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r639728528___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> +dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, )) {
+   goto err;
+}
+dbcookie = _free(dbcookie);
+
+if (!dbus_connection_send(state->bus, msg, )) {
+   goto err;
+}
+dbus_connection_flush(state->bus);
+return RPMRC_OK;
+
+ err:
+rpmlog(RPMLOG_WARNING,
+  "dbus_announce plugin: Error sending message (%s)\n",
+  name);

At closer look there is no good reason to deal with the serial number here. 
Dropped it completely.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r639727856___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> + return RPMRC_OK;
+
+/* ...don't notify on test transactions */
+if (rpmtsFlags(ts) & (RPMTRANS_FLAG_TEST|RPMTRANS_FLAG_BUILD_PROBS))
+   return RPMRC_OK;
+
+/* ...don't notify on chroot transactions */
+if (!rstreq(rpmtsRootDir(ts), "/"))
+   return RPMRC_OK;
+
+dbus_error_init();
+
+state->bus = dbus_bus_get_private(DBUS_BUS_SYSTEM, );
+if (dbus_error_is_set()) {
+   goto err;
+}

Deleted lots of { and }.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r639728238___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti pushed 1 commit.

9a51049918edca7572ab6efd27a45ee2ea9d98b8  Add dbus-announce plugin


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255/files/a1e7a31155a3053e745473e477e1b755ebc5cdbd..9a51049918edca7572ab6efd27a45ee2ea9d98b8
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> @@ -0,0 +1,144 @@
+#include "system.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "lib/rpmplugin.h"
+
+struct dbus_announce_data {
+DBusConnection * bus;
+int old_dboffset;

Done.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r639727122___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +  rpmts ts,
+int res)
+{
+struct dbus_announce_data * state = rpmPluginGetData(plugin);
+DBusMessage* msg;
+dbus_uint32_t serial = 0;
+char * dbcookie;
+
+if (!state->bus) {
+   return RPMRC_OK;
+}
+
+msg = dbus_message_new_signal("/org/rpm/Transaction", /* object name */
+ "org.rpm.Transaction",  /* interface name */
+ name);  /* signal name */
+if (NULL == msg) {

Not that it matters technically but the comparison is usually written the other 
way around, so this kinda jumps at you...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-667580573___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> + return RPMRC_OK;
+
+/* ...don't notify on test transactions */
+if (rpmtsFlags(ts) & (RPMTRANS_FLAG_TEST|RPMTRANS_FLAG_BUILD_PROBS))
+   return RPMRC_OK;
+
+/* ...don't notify on chroot transactions */
+if (!rstreq(rpmtsRootDir(ts), "/"))
+   return RPMRC_OK;
+
+dbus_error_init();
+
+state->bus = dbus_bus_get_private(DBUS_BUS_SYSTEM, );
+if (dbus_error_is_set()) {
+   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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-667576193___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, )) {
+   goto err;
+}
+dbcookie = _free(dbcookie);
+
+if (!dbus_connection_send(state->bus, msg, )) {
+   goto err;
+}
+dbus_connection_flush(state->bus);
+return RPMRC_OK;
+
+ err:
+rpmlog(RPMLOG_WARNING,
+  "dbus_announce plugin: Error sending message (%s)\n",
+  name);

Maybe log the serial number here too since we already have it?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-667571166___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +if (!state->bus) {
+   return RPMRC_OK;
+}
+
+msg = dbus_message_new_signal("/org/rpm/Transaction", /* object name */
+ "org.rpm.Transaction",  /* interface name */
+ name);  /* signal name */
+if (NULL == msg) {
+   goto err;
+}
+
+dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, )) {
+   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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-667568293___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> @@ -0,0 +1,144 @@
+#include "system.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "lib/rpmplugin.h"
+
+struct dbus_announce_data {
+DBusConnection * bus;
+int old_dboffset;

old_dboffset isn't used so should be removed

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-667564433___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2021-05-03 Thread Florian Festi

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-831321667___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> @@ -0,0 +1,25 @@
+'\" t
+.TH "RPM-DBUS-ANNOUNCE" "8" "03 Jun 2020" "Red Hat, Inc."
+.SH NAME
+rpm-plugin-dbus-announce \- DBus plugin for the RPM Package Manager
+
+.SH Description
+
+The plugin writes basic information about rpm transactions to the
+system dbus - like packages installed or removed. Other programms can

Fixed

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r576608360___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-779651408___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> + fprintf(stderr, "Connection Error (%s)\n", err.message);
+   dbus_error_free();
+   }
+   if (state->bus) {
+   rc = dbus_bus_request_name(state->bus, "org.rpm.announce",
+ DBUS_NAME_FLAG_REPLACE_EXISTING , );
+   }
+   if (dbus_error_is_set()) {
+   //fprintf(stderr, "Name Error (%s)\n", err.message);
+   dbus_error_free();
+   }
+   if (DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER != rc) {
+   dbus_connection_close(state->bus);
+   dbus_connection_unref(state->bus);
+   state->bus = NULL;
+   state->logging = 0;

Done

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r576606951___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> +static rpmRC dbus_announce_init(rpmPlugin plugin, rpmts ts)
+{
+struct dbus_announce_data * state = rcalloc(1, sizeof(*state));
+rpmPluginSetData(plugin, state);
+return RPMRC_OK;
+}
+
+static rpmRC open_dbus(rpmPlugin plugin, rpmts ts)
+{
+struct stat st;
+DBusError err;
+int rc = 0;
+struct dbus_announce_data * state = rpmPluginGetData(plugin);
+
+/* Assume we are logging but... */
+state->logging = 1;

Cleaned up.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r576606699___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@ffesti commented on this pull request.



> +state->logging = 1;
+
+/* ...don't log test transactions */
+if (rpmtsFlags(ts) & (RPMTRANS_FLAG_TEST|RPMTRANS_FLAG_BUILD_PROBS))
+   state->logging = 0;
+
+/* ...don't log chroot transactions */
+if (!rstreq(rpmtsRootDir(ts), "/"))
+   state->logging = 0;
+
+/* Don't open */
+if (!state->logging || state->bus)
+   return RPMRC_OK;
+
+if (lstat("/run/systemd/system/", ) == 0) {
+if (S_ISDIR(st.st_mode)) {

Fixed.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r576606032___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2021-02-05 Thread Florian Festi

@ffesti commented on this pull request.



> +return RPMRC_OK;
+}
+
+static rpmRC dbus_announce_tsm_pre(rpmPlugin plugin, rpmts ts)
+{
+int rc;
+
+rc = open_dbus(plugin, ts);
+if (rc != RPMRC_OK)
+   return rc;
+return send_ts_message(plugin, "StartTransaction", ts, RPMRC_OK);
+}
+
+static rpmRC dbus_announce_tsm_post(rpmPlugin plugin, rpmts ts, int res)
+{
+return send_ts_message(plugin, "CompleteTransaction", ts, res);

Using EndTransaction instead.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r570881196___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2021-02-02 Thread Panu Matilainen

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-771564353___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2021-02-02 Thread Florian Festi

@ffesti pushed 1 commit.

c434c333e37f86fb0182c29c2c10c993201c5423  Add dbus-announce plugin


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255/files/b38344a88b25019a3d048ce5d51be10cccfbcc56..c434c333e37f86fb0182c29c2c10c993201c5423
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-11-25 Thread Marek Blaha

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-733608943___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-11-24 Thread Marek Blaha

>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!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-732722116___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-06-26 Thread Panu Matilainen

> > imposes its own signal handling on users when the db is open
> 
> Can turn that off since 
> [56f49d7](https://github.com/rpm-software-management/rpm/commit/56f49d7f5af7c1c8a3eb478431356195adbfdd25)
>  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](https://pagure.io/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, doing 
rpmdb query for debuginfo discovery.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-650015111___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-06-25 Thread Colin Walters

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

Can turn that off since 56f49d7f5af7c1c8a3eb478431356195adbfdd25 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](https://pagure.io/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.  



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-649535595___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-06-25 Thread Panu Matilainen

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-649251979___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-06-17 Thread Michael Schroeder

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-645250342___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-06-17 Thread Panu Matilainen

@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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-645185825___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-06-16 Thread Colin Walters

> 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 `, 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.

--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-645103007___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-642408715___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

2020-06-10 Thread Colin Walters

> 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 https://github.com/coreos/rpm-ostree/pull/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 :smile: 

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)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#issuecomment-642022674___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> + fprintf(stderr, "Connection Error (%s)\n", err.message);
+   dbus_error_free();
+   }
+   if (state->bus) {
+   rc = dbus_bus_request_name(state->bus, "org.rpm.announce",
+ DBUS_NAME_FLAG_REPLACE_EXISTING , );
+   }
+   if (dbus_error_is_set()) {
+   //fprintf(stderr, "Name Error (%s)\n", err.message);
+   dbus_error_free();
+   }
+   if (DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER != rc) {
+   dbus_connection_close(state->bus);
+   dbus_connection_unref(state->bus);
+   state->bus = NULL;
+   state->logging = 0;

Unify this with the code in cleanup hook, maybe close_dbus() helper or such.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427927188___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +return RPMRC_OK;
+}
+
+static rpmRC dbus_announce_tsm_pre(rpmPlugin plugin, rpmts ts)
+{
+int rc;
+
+rc = open_dbus(plugin, ts);
+if (rc != RPMRC_OK)
+   return rc;
+return send_ts_message(plugin, "StartTransaction", ts, RPMRC_OK);
+}
+
+static rpmRC dbus_announce_tsm_post(rpmPlugin plugin, rpmts ts, int res)
+{
+return send_ts_message(plugin, "CompleteTransaction", ts, res);

This is perhaps in the bike-shedding department, but "complete" by definition 
seems to indicate success, whereas this can have an error condition as well. So 
maybe "EndTransaction" would be closer to pairing with Start in these context.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427924424___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +char * dbcookie;
+
+if (!state->logging) {
+   return RPMRC_OK;
+}
+
+msg = dbus_message_new_signal("/org/rpm/Transaction", // object name
+ "org.rpm.Transaction", // interface name
+ name); // name of the signal
+if (NULL == msg) {
+   return RPMRC_FAIL;
+}
+
+dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, )) {
+   return RPMRC_FAIL;

Probably should also add the transaction ID to these messages.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#discussion_r438021380___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +char * dbcookie;
+
+if (!state->logging) {
+   return RPMRC_OK;
+}
+
+msg = dbus_message_new_signal("/org/rpm/Transaction", // object name
+ "org.rpm.Transaction", // interface name
+ name); // name of the signal
+if (NULL == msg) {
+   return RPMRC_FAIL;
+}
+
+dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, )) {
+   return RPMRC_FAIL;

This leaks memory in the failure case.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427912875___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +
+msg = dbus_message_new_signal("/org/rpm/Transaction", // object name
+ "org.rpm.Transaction", // interface name
+ name); // name of the signal
+if (NULL == msg) {
+   return RPMRC_FAIL;
+}
+
+dbcookie = rpmdbCookie(rpmtsGetRdb(ts));
+if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, )) {
+   return RPMRC_FAIL;
+}
+dbcookie = _free(dbcookie);
+
+if (!dbus_connection_send(state->bus, msg, )) {
+   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... 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427912576___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +static rpmRC dbus_announce_init(rpmPlugin plugin, rpmts ts)
+{
+struct dbus_announce_data * state = rcalloc(1, sizeof(*state));
+rpmPluginSetData(plugin, state);
+return RPMRC_OK;
+}
+
+static rpmRC open_dbus(rpmPlugin plugin, rpmts ts)
+{
+struct stat st;
+DBusError err;
+int rc = 0;
+struct dbus_announce_data * state = rpmPluginGetData(plugin);
+
+/* Assume we are logging but... */
+state->logging = 1;

This logging member seems redundant to me, we simply shouldn't open the bus at 
all for test- and chroot-transactions. The less state there is to worry about, 
the better.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427909419___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> + if (dbus_error_is_set()) {
+   //fprintf(stderr, "Name Error (%s)\n", err.message);
+   dbus_error_free();
+   }
+   if (DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER != rc) {
+   dbus_connection_close(state->bus);
+   dbus_connection_unref(state->bus);
+   state->bus = NULL;
+   state->logging = 0;
+   return RPMRC_NOTFOUND;
+   }
+return RPMRC_OK;
+}
+}
+
+return RPMRC_NOTFOUND;

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427904266___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +
+/* ...don't log test transactions */
+if (rpmtsFlags(ts) & (RPMTRANS_FLAG_TEST|RPMTRANS_FLAG_BUILD_PROBS))
+   state->logging = 0;
+
+/* ...don't log chroot transactions */
+if (!rstreq(rpmtsRootDir(ts), "/"))
+   state->logging = 0;
+
+/* Don't open */
+if (!state->logging || state->bus)
+   return RPMRC_OK;
+
+if (lstat("/run/systemd/system/", ) == 0) {
+if (S_ISDIR(st.st_mode)) {
+   // initialise the error value

Please use `/* ... */` for comments, // is not in the rpm coding style even if 
the compiler permits it.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427900196___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)

@pmatilai commented on this pull request.



> +/* ...don't log chroot transactions */
+if (!rstreq(rpmtsRootDir(ts), "/"))
+   state->logging = 0;
+
+/* Don't open */
+if (!state->logging || state->bus)
+   return RPMRC_OK;
+
+if (lstat("/run/systemd/system/", ) == 0) {
+if (S_ISDIR(st.st_mode)) {
+   // initialise the error value
+   dbus_error_init();
+
+   state->bus = dbus_bus_get_private(DBUS_BUS_SYSTEM, );
+   if (dbus_error_is_set()) {
+   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 708e61307bc3fd027b016fdf5a1d1a5274c1843c does. Also, rpmlog() 
should be used instead of fprintf().

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1255#pullrequestreview-427899061___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] Add dbus-announce plugin (#1255)