Bug 23252

Created attachment 28506 [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 28507 [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 28508 [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 28509 [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 28510 [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 28511 [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 28512 [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 28513 [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 28514 [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 28515 [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(-)

And sorry again for the unrelated patches.

A git repo with the full series of patches is also accessible here:

http://git.0pointer.de/?p=dbus.git

git://git.0pointer.de/dbus.git

Comment on attachment 28506 [details] [review]
[PATCH] dbus-monitor: get rid of SIGINT madness

This was committed as 43b1f91865bcaa3929c90

Comment on attachment 28507 [details] [review]
[PATCH] move desktop file parser from bus/ to dbus/

I don't like all of the whitespace changes; I'm fine with cleaning things up incrementally though.

Can you attach a patch without the whitespace changes?

Comment on attachment 28508 [details] [review]
[PATCH] Enable automake 1.11 silent mode by default

I don't know if Thiago has an opinion on this; I don't care one way or the other about silent rules.

However, again on this patch, please attach one without all of the spurious whitespace changes.

Comment on attachment 28509 [details] [review]
[PATCH] desktop-file: relax validity checks a bit

Commit message should specify *what* characters we're making valid here.  
This isn't your fault but the "valid" array could really use some comment delimiters.

Otherwise looks good.

Comment on attachment 28510 [details] [review]
[PATCH] desktop-file: implement _dbus_desktop_file_get_section()

Perhaps call it "section_title"?  

But either is fine by me.

Comment on attachment 28511 [details] [review]
[PATCH] check ConsoleKit database for detecting if user is on console


>+dnl console auth dir
>+if test x$enable_console_auth_dir = xno ; then
>+    have_console_auth_dir=no;
>+else
>+    case $host_os in
>+    linux*)
>+        have_console_auth_dir=yes;

This seems kind of weird; there are embedded/mobile Linux that (unfortunately) won't have ConsoleKit.  You could check for the existence of ck-connector.pc maybe?  Though hmm...ck-connector.pc requires dbus.pc, and that would be a circular dep.

Maybe we need to consider sucking ConsoleKit into dbus itself.  We've discussed having the system bus manage session busses, so having ConsoleKit in dbus would be a natural evolution of that.


>@@ -1,11 +1,11 @@
> /* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */
> /* dbus-sysdeps-util-unix.c Would be in dbus-sysdeps-unix.c, but not used in libdbus
>- * 
>+ *

No spurious whitespace please.

> #include <string.h>
>+#include <errno.h>
>+#include <stdlib.h>

We've regressed on this a bit, but the idea behind dbus-sysdeps.h was to encapsulate most of libc, for both portability and ease of auditing.  In this case looks like you pulled these in for "strtol"?  Should use "_dbus_string_parse_uint" instead.


>+  _dbus_string_init_const(&fn, DBUS_CONSOLEKIT_DATABASE);

DBus coding style is a space between call and paren (your patches fairly consistently do the opposite, so please check them all).


And yeah...we're going to need some caching here.  Can we safely inotify on the file?  Does consolekit use an atomic write-.tmp+rename?

Comment on attachment 28512 [details] [review]
[PATCH] desktop-file: implement _dbus_desktop_file_changed()

Could use a documentation comment (mention that you're going to stat the file).  Otherwise looks fine.

Comment on attachment 28513 [details] [review]
[PATCH] consolekit: cache ckit database file


>+  /* Cache for later. We don't care if that worked. If it didn't the
>+     only price we pay is that we have to recheck next time */
>+  _dbus_hash_table_insert_ulong(ckit_cache, (unsigned long) uid, _DBUS_INT_TO_POINTER(found ? 1 : -1));

Hmm...putting negative numbers into a void * seemed suspicious to me, offhand I didn't know what it would do.  The C FAQ says this is implementation-defined:

http://c-faq.com/ptrs/int2ptr.html

(In reply to comment #17)
> This seems kind of weird; there are embedded/mobile Linux that (unfortunately)
> won't have ConsoleKit.  You could check for the existence of ck-connector.pc
> maybe?  Though hmm...ck-connector.pc requires dbus.pc, and that would be a
> circular dep.

Have the default be "standalone" and require --with-consolekit-auth-dir=/var/whatever, perhaps? Distributions that care about people at consoles will get it right in the packaging.

In embedded/mobile environments at_console will never match, but presumably they can patch system.d policy files to allow their single hard-coded user, or whatever.

This isn't exactly a circular dependency, as long as we deal gracefully with /var/whatever not existing.

I think it's time to close this bug, as CK is on its way out. Theres's no point in adding special support for this now that it is practically dead.