> * Does g-k-d aspire to be session-centric (and thus should stop with the session, not the user D-Bus), or user-centric (and thus run outside the logind session scope)?

This comes down to the prompting. When used with gnome-shell, a prompter is listening on DBus, and gnome-keyring-dameon uses that for prompting. If that's not available it spawns a GTK prompt process using DISPLAY, and talks to that process. It could be user-centric if the prompting fallback here was avoided.

> * If session-centric, could it listen to logind signals when the session is closing and shut down cleanly?

Yes, sounds possible. I'd be up for reviewing such a patch.

> * If user-centric, could it open a Unix socket and the PAM module feeds the password into that instead of stdin?

I think that already exists. Last I checked, the PAM module can either use stdin or a unix socket based unlock mechanism. The stdin mechanism works with available SELinux policy, the unix socket doesn't.

Created attachment 331814 [details] [review]
Die if the XDG session we were started under goes away

Here's a patch to watch the session we were launched under and then cleanup and exit if this closes (according to logind). Sorry for the indirection; logind avoids telling you about changes to 'State', so you have to use some other information to decide when to look at the property and then query it yourself directly.

With this patch, our test Ubuntu session with services launched from systemd --user closes its sessions fully on logout. Without it, g-k-d is left running.

Let me know if there's a better way to do this, or somewhere else to hook it up, or whatever.

Thanks!

Review of attachment 331814 [details] [review]:

Makes sense, and looks like this will work well. I have a few comments below.

::: daemon/gkd-main.c
@@ +836,3 @@
 }
 
+static void on_state_property_get (GObject *connection,

Lets give this a more descriptive name. Something like on_logind_session_property_get().

@@ +860,3 @@
+
+	/* yes, the session is closing, so we'll quit now */
+	if (g_strcmp0 (state, "closing") == 0)

The string 'state' is accessed after the owner 'resultv' has been freed.

@@ +887,3 @@
+				object_path,
+				"org.freedesktop.DBus.Properties",
+				"Get",

Why do we have to call "Get" here? The property in question should be included in changed_properties a{sv} above, right?

@@ +900,3 @@
+}
+
+static void on_object_path_get (GObject *connection,

This should be named something like on_logind_session_object_path_get() (or something more descriptive.

Also a new line immediately before the function name would bring it in line with the rest of teh code.

@@ +912,3 @@
+
+	if (error) {
+		if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED) ||

Shouldn't this be && instead of ||?

@@ +919,3 @@
+			 * silently do nothing, otherwise issue a critical */
+			if (!remote_error ||
+			    g_strcmp0 (remote_error, "org.freedesktop.DBus.Error.NameHasNoOwner") != 0) {

If using g_strcmp0() you shouldn't need to check for !remote_error additionally. g_strcmp0 accepts NULL.

@@ +1089,3 @@
+	/* if we can get a connection to the system bus, watch it and then kill
+	 * ourselves when our session closes */
+	connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, NULL);

Lets break this out into its own function. main() is already too long.

@@ +1090,3 @@
+	 * ourselves when our session closes */
+	connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, NULL);
+

Lets not use the same 'connection' variable for both the system and session bus. I guess if we break this out into its own function, that problem should be solved, right?

(In reply to Stef Walter from comment #3)
> @@ +887,3 @@
> +				object_path,
> +				"org.freedesktop.DBus.Properties",
> +				"Get",
> 
> Why do we have to call "Get" here? The property in question should be
> included in changed_properties a{sv} above, right?

If it's the standard PropertiesChanged signal: not necessarily, it might only be in invalidated_properties (without a value).

Created attachment 333503 [details] [review]
Die if the XDG session we were started under goes away

Hey Stef, thanks for the detailed review - much appreciated.

Fixed everything which I've cut from this reply.

(In reply to Stef Walter from comment #3)
> @@ +887,3 @@
> +				object_path,
> +				"org.freedesktop.DBus.Properties",
> +				"Get",
> 
> Why do we have to call "Get" here? The property in question should be
> included in changed_properties a{sv} above, right?

You get told about 'Active' changing, but we need to know 'State', so another
call is needed. Indeed I do look in changed_properties a little bit above here.

From <https://freedesktop.org/wiki/Software/systemd/logind/>

> Whenever Active or the idle state changes PropertyChanged signals are sent
> out to which clients can subscribe.

> @@ +912,3 @@
> +
> +	if (error) {
> +		if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED) ||
> 
> Shouldn't this be && instead of ||?

I thought about this for far too long - but I think you're right. I've broken
the checks out into booleans and simplified it a bit to try to make this easier
to reason about. Hope that helps.

Review of attachment 333503 [details] [review]:

> Hey Stef, thanks for the detailed review - much appreciated.
> 
> Fixed everything which I've cut from this reply.

Nice!

> > Why do we have to call "Get" here? The property in question should be
> > included in changed_properties a{sv} above, right?
> 
> You get told about 'Active' changing, but we need to know 'State', so another
> call is needed. Indeed I do look in changed_properties a little bit above
> here.

So just to double check. The 'State' property value is not included in the PropertiesChanged signal changed_properties a{sv}?

If that's the case, and the following issues are fixed, looks good to merge.

::: daemon/gkd-main.c
@@ +882,3 @@
+
+	if (g_variant_lookup (changed_properties, "Active", "b", &active, NULL))
+		if (!active)

Could you include braces around both of these. In the case of multiple lines (even though they are semantically one "line"), we include braces.

@@ +960,3 @@
+	const gchar *xdg_session_id;
+
+	connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, NULL);

This variable leaks. Won't g_dbus_connection_call() hold a reference to connection? I think we can g_object_unref(connection) at the end of this function.

(In reply to Stef Walter from comment #6)
> Review of attachment 333503 [details] [review] [review]:
> 
> > Hey Stef, thanks for the detailed review - much appreciated.
> > 
> > Fixed everything which I've cut from this reply.
> 
> Nice!
> 
> > > Why do we have to call "Get" here? The property in question should be
> > > included in changed_properties a{sv} above, right?
> > 
> > You get told about 'Active' changing, but we need to know 'State', so another
> > call is needed. Indeed I do look in changed_properties a little bit above
> > here.
> 
> So just to double check. The 'State' property value is not included in the
> PropertiesChanged signal changed_properties a{sv}?
> 
> If that's the case, and the following issues are fixed, looks good to merge.

Yes, that's right. I suppose properties are changing all the time, and so logind wants to avoid being too chatty on the bus.

> ::: daemon/gkd-main.c
> @@ +882,3 @@
> +
> +	if (g_variant_lookup (changed_properties, "Active", "b", &active, NULL))
> +		if (!active)
> 
> Could you include braces around both of these. In the case of multiple lines
> (even though they are semantically one "line"), we include braces.

Sure.

> @@ +960,3 @@
> +	const gchar *xdg_session_id;
> +
> +	connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, NULL);
> 
> This variable leaks. Won't g_dbus_connection_call() hold a reference to
> connection? I think we can g_object_unref(connection) at the end of this
> function.

It doesn't, I'm afraid. I actually tried this when I made the function and it broke the patch. I suppose you're supposed to hold your own reference until you know you won't need that connection any more. With that in mind, I could unref the connection once we know we're going to quit - that's when it is definitely not going to be wanted any more. It's a bit academic at that point, of course, but I could still do it. Would that be okay?

> With that in mind, I could unref the connection once we know we're going to quit - that's when it is definitely not going to be wanted any more.

Lets make it global then. That makes it much clearer reasoning about the code.

Created attachment 333557 [details] [review]
Die if the XDG session we were started under goes away

(In reply to Stef Walter from comment #8)
> > With that in mind, I could unref the connection once we know we're going to quit - that's when it is definitely not going to be wanted any more.
> 
> Lets make it global then. That makes it much clearer reasoning about the
> code.

Ok, here we go. Hope this is what you had in mind.

Great. Merged. Thanks!

Attachment 333557 [details] pushed as a6e9129 - Die if the XDG session we were started under goes away