[rfc] move activation to a helper process
hp at redhat.com
Thu Oct 19 07:27:07 PDT 2006
This is sort of a half-review, but a few comments from skimming:
- Sending all those DBUS_MAXIMUM_NAME_LENGTH blocks that are usually
mostly-nul seems a little wasteful, despite premature optimization
and all that ...
- Why not send the .service file name instead of the bus name to
activate, then we don't need the inefficient
- 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.
- breaking up the handle_watch function would improve readability
- 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
- 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.
- 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.
More information about the dbus