Bug 60859 - dbus-daemon: optionally capture all packets in pcap format
Summary: dbus-daemon: optionally capture all packets in pcap format
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-, only a prototype
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-14 19:22 UTC by Simon McVittie
Modified: 2018-10-12 21:15 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments
_dbus_spawn_async_with_babysitter: correct documentation (950 bytes, patch)
2013-02-14 19:23 UTC, Simon McVittie
Details | Splinter Review
:n (3.83 KB, text/plain)
2013-02-14 19:24 UTC, Simon McVittie
Details
dbus-spawn: draw a diagram (2.18 KB, patch)
2013-02-14 19:24 UTC, Simon McVittie
Details | Splinter Review
dbus-spawn: correct a comment that falsely claimed thread-safety (1.18 KB, patch)
2013-02-14 19:25 UTC, Simon McVittie
Details | Splinter Review
transaction_free: factor out (1.75 KB, patch)
2013-02-14 19:25 UTC, Simon McVittie
Details | Splinter Review
Add support for piping the message stream (in pcap format) to a command (23.35 KB, patch)
2013-02-14 19:26 UTC, Simon McVittie
Details | Splinter Review
Produce fake NameOwnerChanged signals that encode the pid of each peer (3.29 KB, patch)
2013-02-14 19:26 UTC, Simon McVittie
Details | Splinter Review
[PATCH] Configure summary: display message capture status (983 bytes, patch)
2013-11-01 08:10 UTC, Chengwei Yang
Details | Splinter Review
[PATCH 1/2] Use SIGHUP without check in UNIX environment (1.06 KB, patch)
2013-11-01 08:37 UTC, Chengwei Yang
Details | Splinter Review
[PATCH 2/2] capture.c: ignore SIGPIPE if message capture enabled (1.12 KB, patch)
2013-11-01 08:37 UTC, Chengwei Yang
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2013-02-14 19:22:32 UTC
When profiling a boot or login process, it's useful to capture the entire message stream. Connecting dbus-monitor or bustle-pcap after the dbus-daemon starts is not enough: this would race with any other services that are connecting to the socket at the same time, whose messages might be dispatched before the dbus-monitor or bustle-pcap is ready to eavesdrop on them.

One possible solution is for development builds of dbus-daemon to support a way to write out the message stream. I think this should be in pcap format, because that's what Bustle already uses, it would be trivial to adapt dbus-monitor to read pcap format, and it supports packet truncation if we ever need it.

I know Colin ideally wants to have a "management socket" which speaks the full D-Bus protocol and can eavesdrop, receive control messages and other nice things, but that's pretty complicated. This implementation is much simpler: an optional <pcap_recipient/> in the configuration file contains a shell command, usually of the form "cat > $LOCATION". It receives pcap on its stdin, and... er... that's it.
Comment 1 Simon McVittie 2013-02-14 19:23:52 UTC
Created attachment 74834 [details] [review]
_dbus_spawn_async_with_babysitter: correct documentation

env is used as you'd expect now.

---

I was originally going to use dbus-spawn for the subprocess, but life's too short - I'm just using popen() now. Still, I did make some documentation improvements in dbus-spawn while I was in the vicinity.
Comment 2 Simon McVittie 2013-02-14 19:24:05 UTC
Created attachment 74835 [details]
:n
Comment 3 Simon McVittie 2013-02-14 19:24:44 UTC
Created attachment 74836 [details] [review]
dbus-spawn: draw a diagram

There are enough pipes, fds and processes here that it's important to
keep track of them.
Comment 4 Simon McVittie 2013-02-14 19:25:18 UTC
Created attachment 74837 [details] [review]
dbus-spawn: correct a comment that falsely claimed  thread-safety
Comment 5 Simon McVittie 2013-02-14 19:25:37 UTC
Created attachment 74838 [details] [review]
transaction_free: factor out
Comment 6 Simon McVittie 2013-02-14 19:26:05 UTC
Created attachment 74839 [details] [review]
Add support for piping the message stream (in pcap  format) to a command

This can be used to record a message stream for Bustle. It's normally
no better than Bustle's own bustle-pcap utility, but it does have the
advantage that it can't miss messages if you need to start capturing
messages as soon as the dbus-daemon starts running.
Comment 7 Simon McVittie 2013-02-14 19:26:52 UTC
Created attachment 74840 [details] [review]
Produce fake NameOwnerChanged signals that encode the  pid of each peer

---

+  /* This is an entertaining hack suggested by the illustrious wjt.
+   * To profile a boot process or login, we want Bustle to show us process
+   * IDs. What it actually shows us are all our bus names. So... pretend
+   * the process had a bus name that contains its process ID. */
Comment 8 Simon McVittie 2013-02-25 21:03:57 UTC
Comment on attachment 74839 [details] [review]
Add support for piping the message stream (in pcap  format) to a command

Review of attachment 74839 [details] [review]:
-----------------------------------------------------------------

::: bus/capture.c
@@ +71,5 @@
> +  if (command == NULL || *command == '\0')
> +    return;
> +
> +  /* For simplicity, we use stdio to perform blocking I/O. */
> +  self->pcap = popen (command, "w");

This should maybe prepend "umask 077; " to the command, since D-Bus traffic should be assumed to be sensitive, and the dbus-daemon cannot be assumed to have a restrictive umask.
Comment 9 Chengwei Yang 2013-10-20 02:15:53 UTC
Comment on attachment 74834 [details] [review]
_dbus_spawn_async_with_babysitter: correct documentation

Review of attachment 74834 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 10 Chengwei Yang 2013-10-20 03:31:58 UTC
Comment on attachment 74836 [details] [review]
dbus-spawn: draw a diagram

Review of attachment 74836 [details] [review]:
-----------------------------------------------------------------

looks good except the ASCII art format.

::: dbus/dbus-spawn.c
@@ +192,5 @@
> + *  ======================|======================|================|
> + *  |          child_err_report_pipe (genuinely a pipe)           |
> + *  |[READ_END]-------------<<------------------------[WRITE_END] |
> + *  |        << on failure: CHILD_EXEC_FAILED, errno              |
> + *  |        << on success: nothing               |               |

I think you were expecting the second '|' be removed.

@@ +193,5 @@
> + *  |          child_err_report_pipe (genuinely a pipe)           |
> + *  |[READ_END]-------------<<------------------------[WRITE_END] |
> + *  |        << on failure: CHILD_EXEC_FAILED, errno              |
> + *  |        << on success: nothing               |               |
> + *  (error_pipe_from_child)|                      | (child_err_report_fd)

Not very well formatted ASCII art.
Comment 11 Chengwei Yang 2013-10-20 03:39:54 UTC
Comment on attachment 74837 [details] [review]
dbus-spawn: correct a comment that falsely claimed  thread-safety

Review of attachment 74837 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 12 Chengwei Yang 2013-10-20 03:47:45 UTC
Comment on attachment 74838 [details] [review]
transaction_free: factor out

Review of attachment 74838 [details] [review]:
-----------------------------------------------------------------

::: bus/connection.c
@@ +2127,5 @@
>  
>  static void
> +transaction_free (BusTransaction *transaction)
> +{
> +  DBusMessage *message;

Unused variable?
Comment 13 Simon McVittie 2013-10-23 16:41:06 UTC
(In reply to comment #10)
> I think you were expecting the second '|' be removed.

The idea was that the text spans across more than one "cell": it's really annotating the arrow, not the column.

> @@ +193,5 @@
> Not very well formatted ASCII art.

Yeah, it's difficult to fit all of that information in any sensible width.

Perhaps this?

 * IPC:
 * 
 *                  child_err_report_pipe
 *          /-----------<---------<--------------\
 *          |                                    ^
 *          v                                    |
 * main process           babysitter          grandchild
 *          ^                 ^
 *          v                 v
 *          \-------<->-------/
 *            babysitter_pipe
 *
 * child_err_report_pipe is genuinely a pipe.
 * The READ_END (also called error_pipe_from_child) is used in the main
 * process. The WRITE_END (also called child_err_report_fd) is used in
 * the grandchild process.
 *
 * On failure, the grandchild process sends CHILD_EXEC_FAILED + errno.
 * On success, the pipe just closes (because it's close-on-exec) without
 * sending any bytes.
 *
 * babysitter_pipe is mis-named: it's really a bidirectional socketpair.
 * The [0] end (also called socket_to_babysitter) is used in the main
 * process, the [1] end (also called parent_pipe) is used in the babysitter.
 *
 * If the fork() labelled B in the diagram above fails, the babysitter sends
 * CHILD_FORK_FAILED + errno.
 * On success, the babysitter sends CHILD_PID + the grandchild's pid.
 * On SIGCHLD, the babysitter sends CHILD_EXITED + the exit status.
 * The main process doesn't explicitly send anything, but when it exits,
 * the babysitter gets POLLHUP or POLLERR.
Comment 14 Simon McVittie 2013-10-23 16:42:59 UTC
(In reply to comment #12)
> Unused variable?

Yes, you're right. It should have been part of the commit after.
Comment 15 Chengwei Yang 2013-10-31 07:08:16 UTC
(In reply to comment #13)
> (In reply to comment #10)
> > I think you were expecting the second '|' be removed.
> 
> The idea was that the text spans across more than one "cell": it's really
> annotating the arrow, not the column.
> 
> > @@ +193,5 @@
> > Not very well formatted ASCII art.
> 
> Yeah, it's difficult to fit all of that information in any sensible width.
> 
> Perhaps this?

Yeah, the below diagram looks much clear.

> 
>  * IPC:
>  * 
>  *                  child_err_report_pipe

Do you think if it's a little better if change it to child error report pipe, since it has different name in main process and babysitter process.

>  *          /-----------<---------<--------------\
>  *          |                                    ^
>  *          v                                    |
>  * main process           babysitter          grandchild
>  *          ^                 ^
>  *          v                 v
>  *          \-------<->-------/
>  *            babysitter_pipe

ditto, is babysitter pipe better?

All the other bits looks good to me.

>  *
>  * child_err_report_pipe is genuinely a pipe.
>  * The READ_END (also called error_pipe_from_child) is used in the main
>  * process. The WRITE_END (also called child_err_report_fd) is used in
>  * the grandchild process.
>  *
>  * On failure, the grandchild process sends CHILD_EXEC_FAILED + errno.
>  * On success, the pipe just closes (because it's close-on-exec) without
>  * sending any bytes.
>  *
>  * babysitter_pipe is mis-named: it's really a bidirectional socketpair.
>  * The [0] end (also called socket_to_babysitter) is used in the main
>  * process, the [1] end (also called parent_pipe) is used in the babysitter.
>  *
>  * If the fork() labelled B in the diagram above fails, the babysitter sends
>  * CHILD_FORK_FAILED + errno.
>  * On success, the babysitter sends CHILD_PID + the grandchild's pid.
>  * On SIGCHLD, the babysitter sends CHILD_EXITED + the exit status.
>  * The main process doesn't explicitly send anything, but when it exits,
>  * the babysitter gets POLLHUP or POLLERR.
Comment 16 Chengwei Yang 2013-10-31 07:09:33 UTC
(In reply to comment #14)
> (In reply to comment #12)
> > Unused variable?
> 
> Yes, you're right. It should have been part of the commit after.

Hmm, yes, fixed in the after commit, not a regression in the whole patchset.
Comment 17 Chengwei Yang 2013-10-31 08:12:48 UTC
Comment on attachment 74839 [details] [review]
Add support for piping the message stream (in pcap  format) to a command

Review of attachment 74839 [details] [review]:
-----------------------------------------------------------------

In general, this looks good to me. Except I'm wondering if we have to handle SIGPIPE in bus daemon if the capture pipe broken in any case. An extrem case is the disk full. :-), I think the capture pipe error isn't a critical to bus daemon so we should not crash on its broken.
Comment 18 Chengwei Yang 2013-10-31 08:32:44 UTC
Comment on attachment 74839 [details] [review]
Add support for piping the message stream (in pcap  format) to a command

Review of attachment 74839 [details] [review]:
-----------------------------------------------------------------

In general, this looks good to me. Except I'm wondering if we have to handle SIGPIPE in bus daemon if the capture pipe broken in any case. An extrem case is the disk full. :-), I think the capture pipe error isn't a critical to bus daemon so we should not crash on its broken.

::: bus/capture.c
@@ +132,5 @@
> +
> +      packet_header[0] = tv_sec;
> +      packet_header[1] = tv_usec;
> +      packet_header[2] = len;
> +      packet_header[3] = len; /* would be < len if truncated */

Hmm? is that correct?. According to http://wiki.wireshark.org/Development/LibpcapFileFormat

incl_len: the number of bytes of packet data actually captured and saved in the file. This value should never become larger than orig_len or the snaplen value of the global header.

orig_len: the length of the packet as it appeared on the network when it was captured. If incl_len and orig_len differ, the actually saved packet size was limited by snaplen.
Comment 19 Chengwei Yang 2013-11-01 08:06:34 UTC
Comment on attachment 74840 [details] [review]
Produce fake NameOwnerChanged signals that encode the  pid of each peer

Review of attachment 74840 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 20 Chengwei Yang 2013-11-01 08:10:58 UTC
Created attachment 88459 [details] [review]
[PATCH] Configure summary: display message capture status

How about display this new option configure status in configure summary output?
Comment 21 Chengwei Yang 2013-11-01 08:37:30 UTC
Created attachment 88460 [details] [review]
[PATCH 1/2] Use SIGHUP without check in UNIX environment
Comment 22 Chengwei Yang 2013-11-01 08:37:59 UTC
Created attachment 88461 [details] [review]
[PATCH 2/2] capture.c: ignore SIGPIPE if message capture enabled
Comment 23 Simon McVittie 2013-11-01 11:33:48 UTC
(In reply to comment #15)
> >  *                  child_err_report_pipe
> 
> Do you think if it's a little better if change it to child error report
> pipe, since it has different name in main process and babysitter process.

I used the name of the variable that's used when it's allocated. I'm only trying to document existing practice here, not change the naming (that'd be pointless code churn, unless someone works out a perfect name and changes all the uses to be consistent).

> >  *            babysitter_pipe
> 
> ditto, is babysitter pipe better?

The same.
Comment 24 Simon McVittie 2013-11-01 11:45:39 UTC
(In reply to comment #18)
> In general, this looks good to me. Except I'm wondering if we have to handle
> SIGPIPE in bus daemon if the capture pipe broken in any case.

Hmm, good point. We do have _dbus_disable_sigpipe() for this. It seems bad for a debugging feature to alter global state, though... and we can't rely on MSG_NOSIGNAL, because this isn't sendmsg().

I think the answer is probably "this is just a prototype, and isn't production-ready; don't merge", unfortunately. I think a production-quality version would have to use MSG_NOSIGNAL to send the pcap packets through a socketpair(), and would also have to use non-blocking I/O. That's more code than I want to write right now (and to be honest, more C code than I want to write without the benefit of something like GLib).
Comment 25 Simon McVittie 2013-11-01 11:46:10 UTC
Comment on attachment 74834 [details] [review]
_dbus_spawn_async_with_babysitter: correct documentation

merged for 1.7.8, thanks
Comment 26 Simon McVittie 2013-11-01 11:46:53 UTC
Comment on attachment 74836 [details] [review]
dbus-spawn: draw a diagram

I merged my diagram from Comment #13 - I think it's better than no diagram.
Comment 27 Simon McVittie 2013-11-01 11:47:35 UTC
Comment on attachment 74837 [details] [review]
dbus-spawn: correct a comment that falsely claimed  thread-safety

Applied for 1.7.8
Comment 28 Simon McVittie 2013-11-01 11:47:56 UTC
Comment on attachment 88460 [details] [review]
[PATCH 1/2] Use SIGHUP without check in UNIX environment

Applied for 1.7.8, thanks
Comment 29 Simon McVittie 2013-11-01 11:49:29 UTC
Comment on attachment 74838 [details] [review]
transaction_free: factor out

Applied for 1.7.8, without the unused variable.
Comment 30 GitLab Migration User 2018-10-12 21:15:42 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/79.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.