New SE-DBUS patch
Havoc Pennington
hp at redhat.com
Fri Jun 25 09:19:22 PDT 2004
Hi,
Thanks for the patch, and for posting iterations as you go.
Extending Colin's comments a bit, this list looks long but it's mostly
minor stuff.
- the #ifdef (and also the SELinux header file inclusion) should
be confined to dbus-sysdeps.c basically, or at most to security.[hc];
basically you want to have your SELinux routines exported in a way
that doesn't use the SELinux headers, and if SELinux is either not
present at runtime or not present at compile time these routines
become no-ops (presumably they just always return "action is allowed")
- I wouldn't worry about get_unix_fd() multiple fds until we have a
DBusTransport with multiple, at that time we can add a new
function
- "FIXME Actually check errors like we should" should be fixed
probably
- e->d.policy.security_context = dbus_new0 (DBusString, 1);
is a bit different from how D-BUS is normally done, usually
either DBusString is directly in the struct (rather than DBusString*)
or char* is used (char* is used in DBusPolicy right now)
- as an example of how to contain SELinux stuff to dbus-sysdeps.c,
you have the "Initialize user space AVC" code, that might
go in a routine called "_dbus_init_selinux()" that did nothing
when compiled without SELinux.
- I might call the security.[hc] files selinux.[hc] or something,
since there are a lot of other security-related files in dbus
such as policy.[hc]
- I'd either put all the selinux system API access in security.[hc]
or sysdeps, not in both
- It looks like some functions may be exported from security.[hc]
that are only used in security.[hc]? Ideally the only exports
are the check() routines and an init() routine
- debugging routines such as print_service_sid_table should
be #ifdef DBUS_ENABLE_VERBOSE_MODE
- I'd default enable_selinux to auto rather than no
- the other config.h defines are HAVE_ rather than WITH_
(is WITH_SELINUX significant to the system headers
or something?)
- DBUS_BUS_CFLAGS="$XML_CFLAGS -DWITH_SELINUX"
shouldn't be required if you define WITH_SELINUX in config.h
- you should check for the
selinux lib, look at the AC_CHECK_LIB(expat for an example
- the _dbus_warn() in dbus_connection_get_unix_fd() should
instead be e.g. "_dbus_return_val_if_fail (connection != NULL, FALSE)"
- dbus_bool_t result = FALSE; should be two lines, one declaration
and one assignment
Thanks,
Havoc
More information about the dbus
mailing list