We should ensure this order: database dump() -> callouts -> dbus signal Today the dump action occurs in the manager handling the dbus signals. A simple proposal is to just decouple the dump from the signals by having the session/seat set a needs-dump property to true. The manager would see this change, call dump, which would then clear the property.
Not much tested, but should do the job: http://0pointer.de/public/0001-database-write-the-console-database-to-disk-before-s.patch Jon, any chance you could review and merge this before the F12 alpha freeze?
Hmm, turns out that this patch is not sufficient. The callouts happen from Session.ActiveChanged, not from Seat.ActiveSessionChanged. These two signals are completely redundant as it seems. I really don't get why both are generated. Jon, what's the point? The amount of redundant (and race-inducing) signals triggered by CK doesn't exactly make me happy.
Here's a second patch (on top of the first one), that also makes sure we dump the database before doing callouts. Those two patches together should make sure that we always dump the db first, then do the callouts and finally send the signals. http://0pointer.de/public/0001-database-write-database-before-doing-callouts.patch There are a lot of unnecessary signals and database dumps happening here, but to fix that we'd have to rework ck and especially its dbus iface quite substantially.
Both patches are verified to have the desired effect.
I would probably ditch the database-dump signal and instead connect to active-changed. If you went that route you'd need to move ck_session_run_programs (session, "session_active_changed"); to the (not yet defined) default handler for the active-changed signal.
I've now reworked this considerable. It's now a series of patches: http://git.0pointer.de/?p=ConsoleKit git://git.0pointer.de/ConsoleKit I decided no to drop session.d/ entirely, since I believe it is actually useful. What I did drop however is the session_active_changed type of callout for it. I added seat.d/ instead with a callout seat_active_session_changed as a replacement. This is much cleaner in many many ways. Only ugliness I could understand anyone might see here is that I had to duplicate the 'session-remove' signals of CkSeat for the reason that dbus-glib needs the argument as id string, while we internally need it as CkSession. I saw no way around duplicating the signal due to that, since there is no easy workaround possible, since the CkSession object is otherwise unaccessible at that point (since it isn't listed in the sessions hashtable anymore). So there's now 'session-removed' that is forwarded via dbus-glib, and there is 'sesson-removed-full' for own use. (series also includes a few unrelated patches, but I think those should be merged, too) Please review! Would be nice if we could get this quickly into Rawhide. This should allows us to fix all outstanding problems with CK usage by udev-acl.
Created attachment 28496 [details] [review] [PATCH] dbus-monitor: get rid of SIGINT madness The current SIGINT handling of dbus-monitor ain't making too many people happy since it defers the exit to the next msg received -- which might be quite some time away often enough. This patch replaces the SIGINT handling by simply enabling line-buffered IO for STDOUT so that even if you redirect dbus-monitor into a file no lines get accidently lost and the effect of C-c is still immediate. halfline came up with the great idea to use setvbuf here instead of fflush()ing after each printf(). --- tools/dbus-monitor.c | 24 +++++++----------------- 1 files changed, 7 insertions(+), 17 deletions(-)
Created attachment 28497 [details] [review] [PATCH] move desktop file parser from bus/ to dbus/ We want to make use of it for reading the ConsoleKit database which will need to be implemented in dbus/dbus-userdb-util.c, so let's move this to dbus/. --- bus/Makefile.am | 4 - bus/activation-helper.c | 61 ++-- bus/activation.c | 464 ++++++++++++++-------------- bus/bus.h | 16 +- bus/desktop-file.c | 800 ---------------------------------------------- bus/desktop-file.h | 56 ---- dbus/Makefile.am | 18 +- dbus/dbus-desktop-file.c | 799 +++++++++++++++++++++++++++++++++++++++++++++ dbus/dbus-desktop-file.h | 49 +++ 9 files changed, 1131 insertions(+), 1136 deletions(-)
Created attachment 28498 [details] [review] [PATCH] Enable automake 1.11 silent mode by default Silent mode yay! --- configure.in | 83 +++++++++++++++++++++++++++++---------------------------- 1 files changed, 42 insertions(+), 41 deletions(-)
Created attachment 28499 [details] [review] [PATCH] desktop-file: relax validity checks a bit Mnimally change the validity checks for deskto files so that the ConsoleKit database can pass as valid. --- dbus/dbus-desktop-file.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
Created attachment 28500 [details] [review] [PATCH] desktop-file: implement _dbus_desktop_file_get_section() This allows us to iterate through the sections of a desktop file. --- dbus/dbus-desktop-file.c | 12 ++++++++++++ dbus/dbus-desktop-file.h | 3 +++ 2 files changed, 15 insertions(+), 0 deletions(-)
Created attachment 28501 [details] [review] [PATCH] check ConsoleKit database for detecting if user is on console In addtion to Solaris style /dev/console permission checking and pam_console style /var/run/console file existance checking add support for checking console status via the ConsoleKit database. This adds very basic support and will read the console database on every single read. These needs optimization. --- configure.in | 62 +++++++++++++++-- dbus/dbus-sysdeps-util-unix.c | 150 +++++++++++++++++++++-------------------- dbus/dbus-userdb-util.c | 98 +++++++++++++++++++++------ 3 files changed, 210 insertions(+), 100 deletions(-)
Created attachment 28502 [details] [review] [PATCH] desktop-file: implement _dbus_desktop_file_changed() _dbus_desktop_file_changed() can be used if the file changed since it was read. (by comparing the mtime back then and now) --- dbus/dbus-desktop-file.c | 14 ++++++++++++++ dbus/dbus-desktop-file.h | 3 +++ 2 files changed, 17 insertions(+), 0 deletions(-)
Created attachment 28503 [details] [review] [PATCH] consolekit: cache ckit database file Speed up ckit database checks a little by keeping the parsed database in memory and mantaining an extra hashtable-based cache. --- dbus/dbus-userdb-util.c | 107 +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 98 insertions(+), 9 deletions(-)
Created attachment 28504 [details] [review] [PATCH] desktop-file: fix stat() race _dbus_desktop_file_load() used to stat the desktop file before reading it, to verify its size. This is both racy and unnecessary since _dbus_file_get_contents() which it uses stats the file anyway -- does that however in a race-free fashion with fstat() instead of stat(). This patch gets rid of the redundant stat(). Also, since the desktop file change logic requires the mtime of the file it read we now export that in _dbus_file_get_contents(). This patch probably breaks Win32 builds, but afaik those are broken anyway. --- bus/config-loader-expat.c | 8 +- dbus/dbus-auth-script.c | 76 +++++----- dbus/dbus-desktop-file.c | 19 +-- dbus/dbus-internals.c | 90 ++++++------ dbus/dbus-keyring.c | 136 ++++++++-------- dbus/dbus-message-util.c | 34 ++-- dbus/dbus-sha.c | 64 ++++---- dbus/dbus-sysdeps-unix.c | 385 +++++++++++++++++++++++---------------------- dbus/dbus-sysdeps.h | 29 ++-- 9 files changed, 415 insertions(+), 426 deletions(-)
Created attachment 28505 [details] [review] [PATCH] Complain whenever someone uses at_console at_console should be considered obsolete. Applications should use PolicyKit instea for verifying that a process is on the (active) console. --- bus/config-parser.c | 319 ++++++++++++++++++++++++++------------------------- 1 files changed, 160 insertions(+), 159 deletions(-)
OK, I have now used git-bugzilla to serialize the patch set here, on Colin's request. Sorry that this includes unrelated patches.
Oh, sorry the noise, this was the wrong bug where I attached this.
That said, on request I can of course attach the git patch series for this patch here too. In case anyone is interested in even more bugzilla spam... And sorry again for the noise.
Created attachment 28523 [details] [review] [PATCH] database: write the console database to disk before signalling via dbus We simply change the order how the signal handlers for D-Bus and the database dumping are registered. According to the gobject docs it is guaranteed that the signal handlers are run in the same order as they are registered, so this should be safe and have the desired effect. --- src/ck-manager.c | 29 +++++++++++++++++++---------- src/ck-seat.c | 19 ++----------------- src/ck-seat.h | 2 ++ 3 files changed, 23 insertions(+), 27 deletions(-)
Created attachment 28524 [details] [review] [PATCH] get rid of session.d's session_active_changed callout The session_active_changed callout is an invitaton to race misuses since it splits up the session switches into two events. Get rid of it. --- src/ck-session.c | 2 -- 1 files changed, 0 insertions(+), 2 deletions(-)
Created attachment 28525 [details] [review] [PATCH] Add seat.d/ callout directory and guarantee we dump the database before callout invocation This adds a callout directory called seat.d/ that follows the basic session.d/ semantics but works on seats instead of sessions. As replacement for the old session.d/ 'session_active_changed' semantics seat.d/ knows 'seat_active_session_changed'. Which combines the two callouts necessary for session_active_changed into one. This has various advantages: it's not as racy, allows the suppressing of ACL permission changes when switching between sessions of the same user, reduces the amount of disk IO and finally is less ugly. This patch also moves all callout invocations into the CkManager. This has the advantage that we can guarantee to have fully dumped the CK database before the callout. In summary, the session.d/ directory will now get two types of callout invocations: session_added session_removed In contrast, seat.d/ gets three types: seat_added seat_removed seat_active_session_changed The 'seat_active_session_changed' callout type gets two sets of environment variables describing the old resp. the new session that is active. Either set can be left out if no session was active before, or no session will be active after the switch. This is similar to the logic behind D-Bus' NameOwnerChanged. This patch duplicates CkSeat's 'session-removed' signal into 'session-removed-full' (and friends). Reason for that is that the signal forwarded via D-Bus needs the session id as string while the callout code needs the session itself as object. Since at the time of invocation the CkSeat is no longer in the seats hashtable it is hence necessary to pass the object in as argument to the signal handlers, which makes this duplification necessary to not confuse dbus-glib. For a similar reason 'active-session-changed' is duplicated as well. --- src/Makefile.am | 2 + src/ck-manager.c | 53 +++++++++++++++------ src/ck-marshal.list | 1 + src/ck-seat.c | 131 +++++++++++++++++++++++++++++++++++++++++++++++---- src/ck-seat.h | 5 ++ 5 files changed, 167 insertions(+), 25 deletions(-)
Comment on attachment 28523 [details] [review] [PATCH] database: write the console database to disk before signalling via dbus This looks pretty good. Relying on the ordering seems a little magic to me but if it works and has been tested to work then I'm ok with it. However, I do like the making register explicit and separate from _new. That seems like a good change. Please fix the spacing issues - we require spaces before "(". Otherwise looks good.
Comment on attachment 28524 [details] [review] [PATCH] get rid of session.d's session_active_changed callout This looks OK I guess. Please add some note about this to the NEWS and a generous explanation in the commit message.
Comment on attachment 28525 [details] [review] [PATCH] Add seat.d/ callout directory and guarantee we dump the database before callout invocation >64b6b1d12c016dfd4e68a4b8a41d293b86154802 >diff --git a/src/Makefile.am b/src/Makefile.am >index cbe8a09..6ab05c8 100644 >--- a/src/Makefile.am >+++ b/src/Makefile.am >@@ -205,5 +205,7 @@ MAINTAINERCLEANFILES = \ > install-data-local: > -mkdir -p $(DESTDIR)$(sysconfdir)/ConsoleKit/run-session.d > -mkdir -p $(DESTDIR)$(prefix)/lib/ConsoleKit/run-session.d >+ -mkdir -p $(DESTDIR)$(sysconfdir)/ConsoleKit/run-seat.d >+ -mkdir -p $(DESTDIR)$(prefix)/lib/ConsoleKit/run-seat.d > -mkdir -p $(DESTDIR)$(localstatedir)/run/ConsoleKit > -mkdir -p $(DESTDIR)$(localstatedir)/log/ConsoleKit >diff --git a/src/ck-manager.c b/src/ck-manager.c >index 787f259..449f0f6 100644 >--- a/src/ck-manager.c >+++ b/src/ck-manager.c >@@ -1228,29 +1228,47 @@ ck_manager_can_stop (CkManager *manager, > } > > static void >-on_seat_active_session_changed (CkSeat *seat, >- const char *ssid, >- CkManager *manager) >+on_seat_active_session_changed_full (CkSeat *seat, >+ CkSession *old_session, >+ CkSession *session, >+ CkManager *manager) > { >+ char *ssid = NULL; >+ >+ if (session) >+ ck_session_get_id(session, &ssid, NULL); >+ All blocks should have braces. Please use explicit NULL comparisons. > ck_manager_dump (manager); >+ ck_seat_run_programs (seat, old_session, session, "seat_active_session_changed"); >+ > log_seat_active_session_changed_event (manager, seat, ssid); > } Should we change the logged message too then? > > static void >-on_seat_session_added (CkSeat *seat, >- const char *ssid, >- CkManager *manager) >+on_seat_session_added_full (CkSeat *seat, >+ CkSession *session, >+ CkManager *manager) > { >+ char *ssid = NULL; >+ ck_session_get_id(session, &ssid, NULL); >+ Needs a blank line after declarations. Space before "(" > ck_manager_dump (manager); >+ ck_session_run_programs (session, "session_added"); >+ > log_seat_session_added_event (manager, seat, ssid); > } > Seems like a nice way to do it. > static void >-on_seat_session_removed (CkSeat *seat, >- const char *ssid, >- CkManager *manager) >+on_seat_session_removed_full (CkSeat *seat, >+ CkSession *session, >+ CkManager *manager) > { >+ char *ssid = NULL; >+ ck_session_get_id(session, &ssid, NULL); >+ spacing as above > ck_manager_dump (manager); >+ ck_session_run_programs (session, "session_removed"); >+ > log_seat_session_removed_event (manager, seat, ssid); > } > >@@ -1276,9 +1294,9 @@ static void > connect_seat_signals (CkManager *manager, > CkSeat *seat) > { >- g_signal_connect (seat, "active-session-changed", G_CALLBACK (on_seat_active_session_changed), manager); >- g_signal_connect (seat, "session-added", G_CALLBACK (on_seat_session_added), manager); >- g_signal_connect (seat, "session-removed", G_CALLBACK (on_seat_session_removed), manager); >+ g_signal_connect (seat, "active-session-changed-full", G_CALLBACK (on_seat_active_session_changed_full), manager); >+ g_signal_connect (seat, "session-added-full", G_CALLBACK (on_seat_session_added_full), manager); >+ g_signal_connect (seat, "session-removed-full", G_CALLBACK (on_seat_session_removed_full), manager); How are > g_signal_connect (seat, "device-added", G_CALLBACK (on_seat_device_added), manager); > g_signal_connect (seat, "device-removed", G_CALLBACK (on_seat_device_removed), manager); > } >@@ -1287,9 +1305,9 @@ static void > disconnect_seat_signals (CkManager *manager, > CkSeat *seat) > { >- g_signal_handlers_disconnect_by_func (seat, on_seat_active_session_changed, manager); >- g_signal_handlers_disconnect_by_func (seat, on_seat_session_added, manager); >- g_signal_handlers_disconnect_by_func (seat, on_seat_session_removed, manager); >+ g_signal_handlers_disconnect_by_func (seat, on_seat_active_session_changed_full, manager); >+ g_signal_handlers_disconnect_by_func (seat, on_seat_session_added_full, manager); >+ g_signal_handlers_disconnect_by_func (seat, on_seat_session_removed_full, manager); > g_signal_handlers_disconnect_by_func (seat, on_seat_device_added, manager); > g_signal_handlers_disconnect_by_func (seat, on_seat_device_removed, manager); > } >@@ -1325,7 +1343,9 @@ add_new_seat (CkManager *manager, > g_debug ("Added seat: %s kind:%d", sid, kind); > > ck_manager_dump (manager); >+ ck_seat_run_programs (seat, NULL, NULL, "seat_added"); > >+ g_debug ("Emitting seat-added: %s", sid); > g_signal_emit (manager, signals [SEAT_ADDED], 0, sid); > > log_seat_added_event (manager, seat); >@@ -1366,6 +1386,7 @@ remove_seat (CkManager *manager, > } > > ck_manager_dump (manager); >+ ck_seat_run_programs (seat, NULL, NULL, "seat_removed"); > > g_debug ("Emitting seat-removed: %s", sid); > g_signal_emit (manager, signals [SEAT_REMOVED], 0, sid); >@@ -2429,7 +2450,9 @@ add_seat_for_file (CkManager *manager, > g_debug ("Added seat: %s", sid); > > ck_manager_dump (manager); >+ ck_seat_run_programs (seat, NULL, NULL, "seat_added"); > >+ g_debug ("Emitting seat-added: %s", sid); > g_signal_emit (manager, signals [SEAT_ADDED], 0, sid); > > log_seat_added_event (manager, seat); >diff --git a/src/ck-marshal.list b/src/ck-marshal.list >index f9eed10..7f60efc 100644 >--- a/src/ck-marshal.list >+++ b/src/ck-marshal.list >@@ -1,2 +1,3 @@ > VOID:UINT,STRING > BOOLEAN:POINTER >+VOID:OBJECT,OBJECT >diff --git a/src/ck-seat.c b/src/ck-seat.c >index 27ccf14..2acc307 100644 >--- a/src/ck-seat.c >+++ b/src/ck-seat.c >@@ -67,8 +67,11 @@ struct CkSeatPrivate > > enum { > ACTIVE_SESSION_CHANGED, >- SESSION_ADDED, >+ ACTIVE_SESSION_CHANGED_FULL, >+ SESSION_ADDED, /* Carries the session as path for D-Bus */ >+ SESSION_ADDED_FULL, /* Carries the session as CkSession for other uses */ > SESSION_REMOVED, >+ SESSION_REMOVED_FULL, > DEVICE_ADDED, > DEVICE_REMOVED, > LAST_SIGNAL >@@ -481,15 +484,16 @@ change_active_session (CkSeat *seat, > CkSession *session) > { > char *ssid; >+ CkSession *old_session; Align variables in declarations please. > if (seat->priv->active_session == session) { > return; > } > >- if (seat->priv->active_session != NULL) { >- ck_session_set_active (seat->priv->active_session, FALSE, NULL); >- g_object_unref (seat->priv->active_session); >- } >+ old_session = seat->priv->active_session; >+ >+ if (old_session) >+ ck_session_set_active (old_session, FALSE, NULL); Braces on all blocks. Explicit NULL comparisons. > seat->priv->active_session = session; > >@@ -502,8 +506,12 @@ change_active_session (CkSeat *seat, > > g_debug ("Active session changed: %s", ssid ? ssid : "(null)"); > >+ g_signal_emit (seat, signals [ACTIVE_SESSION_CHANGED_FULL], 0, old_session, session); > g_signal_emit (seat, signals [ACTIVE_SESSION_CHANGED], 0, ssid); Maybe a comment here that the order of these signals matters and full must be first and why.. >+ if (old_session) >+ g_object_unref (old_session); >+ Braces and NULL comparison. > g_free (ssid); > } > >@@ -586,9 +594,8 @@ ck_seat_remove_session (CkSeat *seat, > * unref until the signal is emitted */ > g_hash_table_steal (seat->priv->sessions, ssid); > >- ck_session_run_programs (session, "session_removed"); >- > g_debug ("Emitting session-removed: %s", ssid); >+ g_signal_emit (seat, signals [SESSION_REMOVED_FULL], 0, session); > g_signal_emit (seat, signals [SESSION_REMOVED], 0, ssid); Another comment about why order matters might be good. > /* try to change the active session */ >@@ -624,10 +631,9 @@ ck_seat_add_session (CkSeat *seat, > g_signal_connect_object (session, "activate", G_CALLBACK (session_activate), seat, 0); > /* FIXME: attach to property notify signals? */ > >- ck_session_run_programs (session, "session_added"); >- > g_debug ("Emitting added signal: %s", ssid); > >+ g_signal_emit (seat, signals [SESSION_ADDED_FULL], 0, session); > g_signal_emit (seat, signals [SESSION_ADDED], 0, ssid); Another comment about why order matters might be good. > maybe_update_active_session (seat); >@@ -920,6 +926,15 @@ ck_seat_class_init (CkSeatClass *klass) > g_cclosure_marshal_VOID__STRING, > G_TYPE_NONE, > 1, G_TYPE_STRING); >+ signals [ACTIVE_SESSION_CHANGED_FULL] = g_signal_new ("active-session-changed-full", >+ G_TYPE_FROM_CLASS (object_class), >+ G_SIGNAL_RUN_LAST, >+ 0, >+ NULL, >+ NULL, >+ ck_marshal_VOID__OBJECT_OBJECT, >+ G_TYPE_NONE, >+ 2, CK_TYPE_SESSION, CK_TYPE_SESSION); > signals [SESSION_ADDED] = g_signal_new ("session-added", > G_TYPE_FROM_CLASS (object_class), > G_SIGNAL_RUN_LAST, >@@ -929,6 +944,15 @@ ck_seat_class_init (CkSeatClass *klass) > g_cclosure_marshal_VOID__BOXED, > G_TYPE_NONE, > 1, DBUS_TYPE_G_OBJECT_PATH); >+ signals [SESSION_ADDED_FULL] = g_signal_new ("session-added-full", >+ G_TYPE_FROM_CLASS (object_class), >+ G_SIGNAL_RUN_LAST, >+ 0, >+ NULL, >+ NULL, >+ g_cclosure_marshal_VOID__OBJECT, >+ G_TYPE_NONE, >+ 1, CK_TYPE_SESSION); > signals [SESSION_REMOVED] = g_signal_new ("session-removed", > G_TYPE_FROM_CLASS (object_class), > G_SIGNAL_RUN_LAST, >@@ -938,7 +962,15 @@ ck_seat_class_init (CkSeatClass *klass) > g_cclosure_marshal_VOID__BOXED, > G_TYPE_NONE, > 1, DBUS_TYPE_G_OBJECT_PATH); >- >+ signals [SESSION_REMOVED_FULL] = g_signal_new ("session-removed-full", >+ G_TYPE_FROM_CLASS (object_class), >+ G_SIGNAL_RUN_LAST, >+ 0, >+ NULL, >+ NULL, >+ g_cclosure_marshal_VOID__OBJECT, >+ G_TYPE_NONE, >+ 1, CK_TYPE_SESSION); > signals [DEVICE_ADDED] = g_signal_new ("device-added", > G_TYPE_FROM_CLASS (object_class), > G_SIGNAL_RUN_LAST, >@@ -1134,6 +1166,85 @@ ck_seat_new_from_file (const char *sid, > } > > static void >+env_add_session_info(CkSession *session, >+ const char *prefix, >+ char **extra_env, >+ int *n) >+{ Space before "(" >+ char *s; >+ gboolean b; >+ guint u; Align variables in declarations. >+ if (!session) >+ return; Braces and explicit NULL comparison. >+ if (ck_session_get_id(session, &s, NULL) && s && *s) { >+ extra_env[(*n)++] = g_strdup_printf ("%sID=%s", prefix, s); >+ g_free(s); >+ } >+ >+ if (ck_session_get_session_type(session, &s, NULL) && s && *s) { >+ extra_env[(*n)++] = g_strdup_printf ("%sTYPE=%s", prefix, s); >+ g_free(s); >+ } >+ >+ if (ck_session_get_unix_user(session, &u, NULL)) { >+ extra_env[(*n)++] = g_strdup_printf ("%sUSER_UID=%u", prefix, u); >+ g_free(s); >+ } >+ >+ if (ck_session_get_display_device(session, &s, NULL) && s && *s) { >+ extra_env[(*n)++] = g_strdup_printf ("%sDISPLAY_DEVICE=%s", prefix, s); >+ g_free(s); >+ } >+ >+ if (ck_session_get_x11_display_device(session, &s, NULL) && s && *s) { >+ extra_env[(*n)++] = g_strdup_printf ("%sX11_DISPLAY_DEVICE=%s", prefix, s); >+ g_free(s); >+ } >+ >+ if (ck_session_get_x11_display(session, &s, NULL) && s && *s) { >+ extra_env[(*n)++] = g_strdup_printf ("%sX11_DISPLAY=%s", prefix, s); >+ g_free(s); >+ } >+ >+ if (ck_session_get_remote_host_name(session, &s, NULL) && s && *s) { >+ extra_env[(*n)++] = g_strdup_printf ("%sREMOTE_HOST_NAME=%s", prefix, s); >+ g_free(s); >+ } >+ >+ if (ck_session_is_local(session, &b, NULL)) >+ extra_env[(*n)++] = g_strdup_printf ("%sIS_LOCAL=%s", prefix, b ? "true" : "false"); >+} Lots of spacing issues. Missing explicit NULL checks and '\0' checks. >+void >+ck_seat_run_programs (CkSeat *seat, >+ CkSession *old_session, >+ CkSession *new_session, >+ const char *action) >+{ >+ int n; >+ char *extra_env[18]; /* be sure to adjust this as needed */ What are you trying to say with this comment? As needed for what? Maybe reword. >+ n = 0; >+ >+ extra_env[n++] = g_strdup_printf ("CK_SEAT_ID=%s", seat->priv->id); >+ >+ env_add_session_info(old_session, "CK_SEAT_OLD_SESSION_", extra_env, &n); >+ env_add_session_info(new_session, "CK_SEAT_SESSION_", extra_env, &n); spacing. If old_session or new_session are NULL (which IIRC is valid) then we are just skipping adding any information about them to the environment? Might be better to explicitly indicate they were not set? >+ extra_env[n++] = NULL; >+ >+ g_assert(n <= G_N_ELEMENTS(extra_env)); spacing. >+ ck_run_programs (SYSCONFDIR "/ConsoleKit/run-seat.d", action, extra_env); >+ ck_run_programs (PREFIX "/lib/ConsoleKit/run-seat.d", action, extra_env); >+ >+ for (n = 0; extra_env[n]; n++) >+ g_free (extra_env[n]); >+} braces. explicit null check. >+ >+static void > dump_seat_session_iter (char *id, > CkSession *session, > GString *str) >diff --git a/src/ck-seat.h b/src/ck-seat.h >index dbeda1b..9306d39 100644 >--- a/src/ck-seat.h >+++ b/src/ck-seat.h >@@ -91,6 +91,11 @@ CkSeat * ck_seat_new_with_devices (const char *sid, > CkSeatKind kind, > GPtrArray *devices); > >+void ck_seat_run_programs (CkSeat *seat, >+ CkSession *old_session, >+ CkSession *new_session, >+ const char *action); >+ > gboolean ck_seat_register (CkSeat *seat); > > void ck_seat_dump (CkSeat *seat,
Created attachment 28527 [details] [review] [PATCH] database: write the console database to disk before signalling via dbus We simply change the order how the signal handlers for D-Bus and the database dumping are registered. According to the gobject docs it is guaranteed that the signal handlers are run in the same order as they are registered, so this should be safe and have the desired effect. --- src/ck-manager.c | 29 +++++++++++++++++++---------- src/ck-seat.c | 19 ++----------------- src/ck-seat.h | 2 ++ 3 files changed, 23 insertions(+), 27 deletions(-)
Created attachment 28528 [details] [review] [PATCH] get rid of session.d's session_active_changed callout The 'session_active_changed' callout is an invitaton to racy misuses since it splits up the session switches into two events. This patch gets rid of it. At this point there are two known users of the session.d/ callouts: - Suse/Ubuntu ship a script for emulating pam_atconsole style /var/run/console management for supporting D-Bus' at_console feature properly. Both Martin Pitt and Kay Sievers however are happy with having this feature go away. In addition I prepared a patch for D-Bus which makes the need for these scripts go away entirely. That patch is currently awaiting review. - udev's udev-acl tool currently uses this. However this doesn't work correctly anyway since it relies on that the CK database is correctly dumped before the tool is invoked, which CK does not handle properly at this time. In fact fixing this problem is the main reason why I came up with this patch series. Kay is also very interested in seeing the 'session_active_changed' logic go away. A later commit introduces a replacement functionality that is less racy. Please note that this patch does not drop session.d/ in its entirety. Only the 'session_active_changed' type callout is removed. Scripts that only rely on 'session_added'/'session_removed' will continue to work fine, and will continue to be supported. --- NEWS | 8 +++++++- src/ck-session.c | 2 -- 2 files changed, 7 insertions(+), 3 deletions(-)
Created attachment 28529 [details] [review] [PATCH] Add seat.d/ callout directory and guarantee we dump the database before callout invocation This adds a callout directory called seat.d/ that follows the basic session.d/ semantics but works on seats instead of sessions. As replacement for the old session.d/ 'session_active_changed' semantics seat.d/ knows 'seat_active_session_changed'. Which combines the two callouts necessary for session_active_changed into one. This has various advantages: it's not as racy, allows the suppressing of ACL permission changes when switching between sessions of the same user, reduces the amount of disk IO and finally is less ugly. This patch also moves all callout invocations into the CkManager. This has the advantage that we can guarantee to have fully dumped the CK database before the callout. In summary, the session.d/ directory will now get two types of callout invocations: session_added session_removed In contrast, seat.d/ gets three types: seat_added seat_removed seat_active_session_changed The 'seat_active_session_changed' callout type gets two sets of environment variables describing the old resp. the new session that is active. Either set can be left out if no session was active before, or no session will be active after the switch. This is similar to the logic behind D-Bus' NameOwnerChanged. This patch duplicates CkSeat's 'session-removed' signal into 'session-removed-full' (and friends). Reason for that is that the signal forwarded via D-Bus needs the session id as string while the callout code needs the session itself as object. Since at the time of invocation the CkSeat is no longer in the seats hashtable it is hence necessary to pass the object in as argument to the signal handlers, which makes this duplification necessary to not confuse dbus-glib. For a similar reason 'active-session-changed' is duplicated as well. --- src/Makefile.am | 2 + src/ck-manager.c | 56 ++++++++++++++----- src/ck-marshal.list | 1 + src/ck-seat.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++--- src/ck-seat.h | 5 ++ 5 files changed, 192 insertions(+), 25 deletions(-)
All patches updated according to your requests, please merge.
Created attachment 28555 [details] [review] [PATCH] Add seat.d/ callout directory and guarantee we dump the database before callout invocation This adds a callout directory called seat.d/ that follows the basic session.d/ semantics but works on seats instead of sessions. As replacement for the old session.d/ 'session_active_changed' semantics seat.d/ knows 'seat_active_session_changed'. Which combines the two callouts necessary for session_active_changed into one. This has various advantages: it's not as racy, allows the suppressing of ACL permission changes when switching between sessions of the same user, reduces the amount of disk IO and finally is less ugly. This patch also moves all callout invocations into the CkManager. This has the advantage that we can guarantee to have fully dumped the CK database before the callout. In summary, the session.d/ directory will now get two types of callout invocations: session_added session_removed In contrast, seat.d/ gets three types: seat_added seat_removed seat_active_session_changed The 'seat_active_session_changed' callout type gets two sets of environment variables describing the old resp. the new session that is active. Either set can be left out if no session was active before, or no session will be active after the switch. This is similar to the logic behind D-Bus' NameOwnerChanged. This patch duplicates CkSeat's 'session-removed' signal into 'session-removed-full' (and friends). Reason for that is that the signal forwarded via D-Bus needs the session id as string while the callout code needs the session itself as object. Since at the time of invocation the CkSeat is no longer in the seats hashtable it is hence necessary to pass the object in as argument to the signal handlers, which makes this duplification necessary to not confuse dbus-glib. For a similar reason 'active-session-changed' is duplicated as well. --- src/Makefile.am | 2 + src/ck-manager.c | 56 +++++++++++++----- src/ck-marshal.list | 1 + src/ck-seat.c | 161 +++++++++++++++++++++++++++++++++++++++++++++++--- src/ck-seat.h | 5 ++ 5 files changed, 200 insertions(+), 25 deletions(-)
Comment on attachment 28529 [details] [review] [PATCH] Add seat.d/ callout directory and guarantee we dump the database before callout invocation Attached a patch replacing this one wth minimal comment fixes now.
Patches pushed. Thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.