About performance of D-Bus

[Havoc Pennington]

Hi,

Some quick patch comments,

*
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <time.h>

Generally speaking including unix headers directly is not allowed,
unless the feature is unix-specific. There are two things here. First,
we wrap system APIs (similar to vsftpd policy) even when there are no
portability concerns. Second, we do have portability concerns, so we
also abstract system APIs for unix vs. windows. It is OK to do the
abstractions as something very high level, like
"_dbus_add_traffic_file_entry()" rather than open, write, and close
functions, for example.

* if _dbus_string_init() etc. fail, it's necessary to handle them by
"rolling back" the transactional state of the bus daemon; the bus
daemon is robust against memory allocation failure.

* _dbus_string_append_printf() return value should be checked

* +void dbus_profiling_record_message(const char *domain, DBusMessage *message)
Needs more newlines (after void and between the two args). Same for
all other functions.

* +  if (_dbus_getenv("DBUS_TRAFFIC_PROFILING") == NULL)
  +    return;
Suggest putting this in the config file instead if the feature were
ever intended for use on a "live" bus, since it is impractical to
restart the system bus, and annoying to restart the session bus.

* +          fd = open(_dbus_strdup(_dbus_getenv("DBUS_TRAFFIC_PROFILING_LOG")),
Doesn't this leak the duplicated string? Also _dbus_strdup could
return NULL, as could the _dbus_getenv

* +#include "dbus-message.h"
Should be <dbus/dbus-message.h>

* +_message_to_str(DBusString *str, const char *domain, DBusMessage *message)
Should not underscore-prefix static functions. Should spell out
"string" in the function name.

* +#include "config.h"
<config.h>

* +#define dbus_profiling_record_message(d, m) do { } while(0);
That should go in the header I think, and also not end in ";" (the
point of the while(0) is to allow the macro caller to put a semicolon
themselves)

* I'm not sure this should be a configure option; that would mean
(presumably) that to use the feature you have to replace the system
copy of dbus, which is pretty annoying. I think it's fine to just have
a runtime boolean to turn it on and off, as long as there's no
overhead beyond the boolean check when it's off.

* To make it a runtime option, you could have a new config file
element like <trafficlog>filename</trafficlog> or something like that

Hopefully those comments get you started, I have not really thought
about the big picture here. I think this simple log might be most
useful for debugging (seeing what messages go through) as opposed to
profiling (does this log help with timing anything?)

I wonder if the "debugging" log should be somewhat more human-readable
with helpful formatting and maybe show the message args, while the
profiling log should include more info on things like time of each
message and how long round trips last and so forth.

I don't really know though. Maybe other people have thoughts.

Havoc