| Summary: | dbus-daemon: optionally capture all packets in pcap format | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED MOVED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | enhancement | ||
| Priority: | low | CC: | bugzilla, chengwei.yang.cn, hp, msniko14, walters, will |
| Version: | 1.5 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | review-, only a prototype | ||
| i915 platform: | i915 features: | ||
| Attachments: |
_dbus_spawn_async_with_babysitter: correct documentation
:n dbus-spawn: draw a diagram dbus-spawn: correct a comment that falsely claimed thread-safety transaction_free: factor out Add support for piping the message stream (in pcap format) to a command Produce fake NameOwnerChanged signals that encode the pid of each peer [PATCH] Configure summary: display message capture status [PATCH 1/2] Use SIGHUP without check in UNIX environment [PATCH 2/2] capture.c: ignore SIGPIPE if message capture enabled |
||
|
Description
Simon McVittie
2013-02-14 19:22:32 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. Created attachment 74835 [details]
:n
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. Created attachment 74837 [details] [review] dbus-spawn: correct a comment that falsely claimed thread-safety Created attachment 74838 [details] [review] transaction_free: factor out 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. 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 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 on attachment 74834 [details] [review] _dbus_spawn_async_with_babysitter: correct documentation Review of attachment 74834 [details] [review]: ----------------------------------------------------------------- looks good 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 on attachment 74837 [details] [review] dbus-spawn: correct a comment that falsely claimed thread-safety Review of attachment 74837 [details] [review]: ----------------------------------------------------------------- looks good 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? (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. (In reply to comment #12) > Unused variable? Yes, you're right. It should have been part of the commit after. (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. (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 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 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 on attachment 74840 [details] [review] Produce fake NameOwnerChanged signals that encode the pid of each peer Review of attachment 74840 [details] [review]: ----------------------------------------------------------------- Looks good. Created attachment 88459 [details] [review] [PATCH] Configure summary: display message capture status How about display this new option configure status in configure summary output? Created attachment 88460 [details] [review] [PATCH 1/2] Use SIGHUP without check in UNIX environment Created attachment 88461 [details] [review] [PATCH 2/2] capture.c: ignore SIGPIPE if message capture enabled (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. (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 on attachment 74834 [details] [review] _dbus_spawn_async_with_babysitter: correct documentation merged for 1.7.8, thanks 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 on attachment 74837 [details] [review] dbus-spawn: correct a comment that falsely claimed thread-safety Applied for 1.7.8 Comment on attachment 88460 [details] [review] [PATCH 1/2] Use SIGHUP without check in UNIX environment Applied for 1.7.8, thanks Comment on attachment 74838 [details] [review] transaction_free: factor out Applied for 1.7.8, without the unused variable. -- 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.