Bug 89165 - dbus-monitor --profile output format issue
Summary: dbus-monitor --profile output format issue
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.8
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-16 08:54 UTC by Ralf Habacker
Modified: 2015-02-17 07:51 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
dbus-monitor.c: convert tabs to spaces (4.86 KB, patch)
2015-02-16 10:59 UTC, Ralf Habacker
Details | Splinter Review
dbus-monitor.c: unify columns format in --profile mode and display column header. (3.40 KB, patch)
2015-02-16 11:03 UTC, Ralf Habacker
Details | Splinter Review
dbus-monitor: convert remaining hard tabs to 8 spaces (5.00 KB, patch)
2015-02-16 11:52 UTC, Simon McVittie
Details | Splinter Review
dbus-monitor.c: unify columns format in --profile mode and display column header. (3.60 KB, patch)
2015-02-16 12:05 UTC, Simon McVittie
Details | Splinter Review
dbus-monitor: clarify column headers (1.01 KB, patch)
2015-02-16 12:06 UTC, Simon McVittie
Details | Splinter Review
dbus-monitor: Combine sec and usec columns into one timestamp column (1.71 KB, patch)
2015-02-16 13:42 UTC, Ralf Habacker
Details | Splinter Review
dbus-monitor: Remove empty column in profile mode (1.21 KB, patch)
2015-02-16 13:43 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2015-02-16 08:54:27 UTC
While tracking bug https://bugs.freedesktop.org/show_bug.cgi?id=88896 it has been recognized that the dbus-monitor --profile do not print out any header, which makes it hard to interpret the displayed columns.
Comment 1 Ralf Habacker 2015-02-16 10:58:51 UTC
A research on the related code gives: 


type     |---- timestamp ---| 
         seconds         usec   serial  path                      interface                 member 
sig     1424074437      962905  2       /org/freedesktop/DBus   org.freedesktop.DBus    NameAcquired


type     |---- timestamp ---| 
         seconds         usec   serial  sender   path                     interface             member 
mc      1424074437      962925  2       :1.8560 /org/freedesktop/DBus   org.freedesktop.DBus    AddMatch



type     |---- timestamp ---|  ¦- serial -| destination
         seconds         usec  sender replay    
mr      1424074441      91430   78816   18470   :1.1174


type     |---- timestamp ---|  ¦- serial -|  destination
         seconds         usec  sender replay    

err     1424074443      720703  64      5       :1.8561


There are three different formats, which makes it hard to define a common column headers.
Comment 2 Ralf Habacker 2015-02-16 10:59:29 UTC
Created attachment 113523 [details] [review]
dbus-monitor.c: convert tabs to spaces
Comment 3 Ralf Habacker 2015-02-16 11:03:02 UTC
Created attachment 113524 [details] [review]
dbus-monitor.c: unify columns format in --profile mode and display column header.

Note: The header for the 'reply serial' value printed for method return and error has been named 'ref serial' because in fact it references the serial of the related method call (which is also wrong titled in --monitor mode)
Comment 4 Simon McVittie 2015-02-16 11:51:59 UTC
(In reply to Ralf Habacker from comment #2)
> dbus-monitor.c: convert tabs to spaces

As much as I'd like to say "yes, fix our whitespace", you seem to have converted hard tabs to 4 spaces, which leads to some bizarre indentation, like the "then" clause of an "if" being less indented than the "if" itself.
Comment 5 Simon McVittie 2015-02-16 11:52:24 UTC
Created attachment 113525 [details] [review]
dbus-monitor: convert remaining hard tabs to 8 spaces
Comment 6 Simon McVittie 2015-02-16 11:57:07 UTC
(In reply to Ralf Habacker from comment #3)
> Note: The header for the 'reply serial' value printed for method return and
> error has been named 'ref serial' because in fact it references the serial
> of the related method call (which is also wrong titled in --monitor mode)

I realise it isn't an ideal name, but "reply_serial" is what this is called internally (e.g. REPLY_SERIAL in the Specification, and dbus_message_get_reply_serial()).

I would suggest either keeping it as-is, or calling it "in reply to" which makes the purpose clear.
Comment 7 Simon McVittie 2015-02-16 12:05:27 UTC
Created attachment 113526 [details] [review]
dbus-monitor.c: unify columns format in --profile mode  and display column header.

From: Ralf Habacker <ralf.habacker@freenet.de>

[rebase onto correctly indented version -smcv]

---

This is just Attachment #113524 [details] adjusted to apply after Attachment #113525 [details].
Comment 8 Simon McVittie 2015-02-16 12:06:59 UTC
Created attachment 113527 [details] [review]
dbus-monitor: clarify column headers

- change "ref serial" to "in_reply_to" (avoiding whitespace for easy
  visual parsing)
- prefix with # to clarify that these are not part of the data
Comment 9 Ralf Habacker 2015-02-16 12:34:15 UTC
Comment on attachment 113526 [details] [review]
dbus-monitor.c: unify columns format in --profile mode  and display column header.

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

::: tools/dbus-monitor.c
@@ +184,5 @@
>      }
>  
> +  if (first)
> +    {
> +      profile_print_headers();

space after function name missing (my fault, I really need to have an astyle or indent setting)
Comment 10 Ralf Habacker 2015-02-16 12:34:58 UTC
Comment on attachment 113525 [details] [review]
dbus-monitor: convert remaining hard tabs to 8 spaces

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

looks good
Comment 11 Ralf Habacker 2015-02-16 12:35:48 UTC
Comment on attachment 113527 [details] [review]
dbus-monitor: clarify column headers

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

looks good
Comment 12 Ralf Habacker 2015-02-16 12:41:14 UTC
Comment on attachment 113526 [details] [review]
dbus-monitor.c: unify columns format in --profile mode  and display column header.

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

::: tools/dbus-monitor.c
@@ +131,5 @@
>  
>  static void
> +profile_print_headers (void)
> +{
> +    printf ("type\tsec\tusec\tserial\tsender\tdestination\tpath\tinterface\tmember\n");

For what is the separation between seconds an usecs good ? To calculate time difference it is always required to combine both columns, so this separation seems to make no sense.
Comment 13 Simon McVittie 2015-02-16 12:50:41 UTC
(In reply to Ralf Habacker from comment #12)
> For what is the separation between seconds an usecs good ?

No idea, I didn't add --profile format. Combining them into "%lu.%06lu", (unsigned long) sec, (unsigned long) usec would be fine IMO.
Comment 14 Simon McVittie 2015-02-16 12:54:32 UTC
(In reply to Ralf Habacker from comment #9)
> space after function name missing (my fault, I really need to have an astyle
> or indent setting)

Fixed that and pushed all three patches for 1.9.12. I'll leave this bug open for now if you're thinking about combining sec and usec.
Comment 15 Ralf Habacker 2015-02-16 13:42:53 UTC
Created attachment 113533 [details] [review]
dbus-monitor: Combine sec and usec columns into one timestamp column
Comment 16 Ralf Habacker 2015-02-16 13:43:21 UTC
Created attachment 113534 [details] [review]
dbus-monitor: Remove empty column in profile mode
Comment 17 Simon McVittie 2015-02-16 13:47:34 UTC
Comment on attachment 113533 [details] [review]
dbus-monitor: Combine sec and usec columns into one timestamp column

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

Looks good, although the header-changing bit should ideally have been part of this commit and not the next one.
Comment 18 Simon McVittie 2015-02-16 13:49:03 UTC
Comment on attachment 113534 [details] [review]
dbus-monitor: Remove empty column in profile mode

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

Sure, ship it
Comment 19 Ralf Habacker 2015-02-16 14:14:09 UTC
Comment on attachment 113533 [details] [review]
dbus-monitor: Combine sec and usec columns into one timestamp column

applied to master with merge of header changing bits
Comment 20 Ralf Habacker 2015-02-16 14:15:05 UTC
Comment on attachment 113534 [details] [review]
dbus-monitor: Remove empty column in profile mode

applied to master with header column remove only
Comment 21 Ralf Habacker 2015-02-17 07:51:11 UTC
For the record: parameter list of error returns does not match --monitor mode


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.