[rfc] move activation to a helper process

[David Zeuthen]

On Thu, 2006-10-19 at 10:27 -0400, Havoc Pennington wrote:
> Hi,
> 
> This is sort of a half-review, but a few comments from skimming:

Thanks!

>   - Sending all those DBUS_MAXIMUM_NAME_LENGTH blocks that are usually
>     mostly-nul seems a little wasteful, despite premature optimization
>     and all that ...

Well, initially I had a whole bunch of code to cope with variable length
messages but it's much hard to audit than if you assume message size is
the same length. And we want this code to be as simple as possible.

It's especially hard if you want the IPC between helper and bus to not
allocate memory from the heap at all. And we want this property, e.g. no
heap memory allocation to guarantee responses even on OOM.

>   - Why not send the .service file name instead of the bus name to
>     activate, then we don't need the inefficient
>     _scan_service_dir_for_desktop_file

We can't send the full path, obviously, because that would violate the
trust model (helper doesn't trust bus one bit). So we need at least to
parse the config file and look through the directories to find the file.
So all we would be saving would be loading all desktop files when doing
this, not sure it's worth the effort...

>   - the fork-without-exec is inherently somewhat confusing, I think
>     a nice way to make it clearer would be to have a single
>     file where all code runs in the child, exporting one
>     function like bus_activation_helper_start_child(),
>     where this function is called in the child immediately post-fork.
>     but I don't know if this is easily done. I think it'd be
>     nicer to split the BusActivationHelper struct up instead of having
>     the in_helper flag, also.

My latest version has the split into BusActivationHelper and
BusActivationHelperPrivate. I'll move the Private stuff into a separate
file if possible.

>   - breaking up the handle_watch function would improve readability

Yea, I guess, it's a style issue, up to you. The current version is a
bit smaller FWIW. I like to have it in a single function so it's easy to
verify you always get a response even if OOM (new version always
generates a response,).

>   - in handle_bus_pipe the "should never happen" should use _dbus_warn
>     or _dbus_assert_not_reached so we can see it if it does

Will do.

>   - the system headers (especially unix-specific ones) aren't allowed
>     in bus/*.c in general; the current exception is dir-watch-* which
>     is configure-protected. string.h is allowed but almost always
>     DBusString should be used instead (trivial strcmp, strlen is OK,
>     but if you're doing pointer math, DBusString is much safer)
>     The minimum fix is just wrap all the system calls in a trivial way
>     in dbus-sysdeps-unix.[hc], better though is to come up with a
>     higher-level wrapper that could work on windows ... probably on
>     windows it needs to use threads instead of fork ... remember that
>     the sysdeps abstractions don't need to be glib/nspr-worthy, they
>     can be a little ad hoc or special purpose.
>     One thought is to encapsulate the two pipes and make them look
>     like a full duplex pipe (could even use socketpair when available)
>     and allow using some other mechanism on windows; and then also
>     encapsulate threads vs. process. so basically have "run this
>     code async in another thread or process, with a full duplex
>     stream to talk to it" as the abstraction. The stream might
>     have its own read/write methods, since I don't think using sockets
>     on windows will make sense, which means the current _dbus_read_socket
>     etc. won't work.

Ugh, that sounds like real boring work. Let's discuss it after I send
the next version :-)

>   - officially at the moment dbus_uint64_t is "optional" (requires
>     DBUS_HAVE_INT64 checks) but I think GLib and Qt both require 64-bit
>     int now, so making it optional is probably stupid. I was just copying
>     older glib here and I don't think anyone is aware of a
>     currently-interesting platform that lacks int64.

Yea, I see little point in making it optional other than to have more
than one ABI lying around. Can we go ahead and nuke that option?

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

(The latter was very painful when the helper is in a separate thread
because I kept getting messages about I had memory leaks when I didn't
have (was running on a SMP system). It took me 8 hours or so to realize
that dbus_malloc and friends was the culprit. Mea culpa. I guess.)

Anyway, will send latest version soon.

     David