[rfc] move activation to a helper process

[David Zeuthen]

Hi,

Attaching the latest version. Now 'make check' works!

 $ diffstat dbus-sysbus-activation-helper-4.patch 
 bus/Makefile.am         |    2 
 bus/activation-helper.c | 1911 ++++++++++++++++++++++++++++++++++++++++++++++++
 bus/activation-helper.h |   67 +
 bus/activation.c        |  336 ++++----
 bus/bus.c               |   93 +-
 bus/bus.h               |   13 
 bus/desktop-file.c      |   13 
 bus/dispatch.c          |   55 -
 bus/main.c              |   10 
 bus/test-main.c         |  154 +--
 bus/test.c              |   93 ++
 bus/test.h              |   20 
 dbus/dbus-memory.c      |   31 
 dbus/dbus-spawn.c       |    2 
 dbus/dbus-threads.c     |    2 
 15 files changed, 2499 insertions(+), 303 deletions(-)

On Thu, 2006-10-19 at 15:30 -0400, Havoc Pennington wrote:
> I think we can save the readdir in addition to the parsing as follows: I 
> bet it's OK to trust the bus to send the right service file as long as 
> we don't trust the bus to send a known service file. IOW I think the bus 
> could send a full path as long as the helper ensures the path is inside 
> one of the directories given in the config file. If there are two 
> .service files that implement the same bus name and an evil bus wants to 
> run the wrong one, I don't think it's a big deal, as long as both 
> .service files are root-owned and in a system directory. (checking the 
> root-owned bit is probably worthwhile ...)

Hmm, ok, so this is actually a new requirement. E.g. for this to work we
will require that service files are owned by root. 

This is fine with me but I can see this breaking some stuff, for example
package management systems where you create a new user for every package
(LFS recommends this or did once at least).

I don't know whether it's sane to check it's a system directory - it's a
pretty loose term anyway, I suspect you mean something in /usr, /etc
or /opt. Not sure we should hard code this into the sources. I can see
this break things like jhbuild and garnome that builds it's own D-Bus
(for better or worse!). So I think just checking whether it's owned by
root is good enough.

If you are fine with this requirement... then I'll just go ahead and
implement it! Shouldn't be hard at all.

> > Btw, I've also got fixes for BusDesktopFile (it doesn't handle OOM
> > correctly) and dbus_malloc + friends (not MT-safe when DBUS_BUILD_TESTS
> > is enabled).
> 
> Sounds good. This fix and also the add-newlines-to-dbus-warn should 
> probably be in 1.0, so maybe you could separate them out?

They should be easy to grab from the patch. When everything is
satisfactory I'll split out this mega-patch into different patches and
make sure the first ones. 

We could also just commit all the code for 1.0, I mean it's a nice
feature and if it doesn't work we should fix it soon :-)

(Btw, if would be nice if we used git for the core, then I'd just commit
this to my repo and you could grab these bits for now.)

> It would be good to avoid locking in malloc when not building tests, if 
> we locked in there it'd probably be a good 10-20% slowdown I would bet 
> based on past profiling where malloc and thread locks both showed up as 
> hot spots.

We don't need to lock when not build tests as we can assume that system
malloc is MT-safe. It's only applicable for DBUS_BUILD_TESTS. Btw, I
didn't bother making the bits for failing mallocs MT-safe, we probably
should, but the right way to do this requires a DBusMutex to lock around
the whole thing. I can do this if you want.

Two other things I had to fix. In dbus/dbus-threads.c

> @@ -350,8 +350,6 @@ init_uninitialized_locks (void)
>  {
>    DBusList *link;
>  
> -  _dbus_assert (thread_init_generation == 0);

I'm pretty sure this is wrong as we call dbus_shutdown() between tests
and docs for this one tells use to use dbus_thread_init() again. Is this
fix right?

The other one is in bus/dispatch.c:

> @@ -2556,12 +2556,13 @@ check_existent_service_no_auto_start (Bu
>              if (message_kind != GOT_ERROR)
>                {
>                  block_connection_until_message_from_bus (context, connection, 
> "error about service exiting");
> -              
> +               
>                  /* and process everything again */
>                  bus_test_run_everything (context);
>                
>                  if (!check_got_error (context, connection,
>                                        DBUS_ERROR_SPAWN_CHILD_EXITED,
> +                                     DBUS_ERROR_NO_MEMORY,
>                                        NULL))

which I also think is the right fix because we should expect OOM
everywhere. Right?

Anyway, looks like we are getting there. Apart from the TODO's
identified above I also need to handle the User= key's presence
depending on whether we are a system bus or not. That shouldn't be hard
though.

     David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbus-sysbus-activation-helper-4.patch
Type: text/x-patch
Size: 105793 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20061019/d920c16e/dbus-sysbus-activation-helper-4-0001.bin