When we moved to using monotonic time, support for DBUS_COOKIE_SHA1 was broken, in particular interoperability with non-libdbus-1 implementations such as GDBus. The problem is that, if monotonic clocks are available, _dbus_get_current_time() won't return the number of seconds since the Epoch (as required by the D-Bus spec). This bug was originally reported here https://bugzilla.gnome.org/show_bug.cgi?id=673943
Created attachment 59826 [details] [review] Patch This patch fixes the problem for me - I've also double-checked that it works with GDBus, see https://bugzilla.gnome.org/show_bug.cgi?id=673943 for more details. Once a D-Bus release with this bug-fix is available, GDBus will include a test-case for DBUS_COOKIE_SHA1 interop with libdbus-1 (and require this version).
Comment on attachment 59826 [details] [review] Patch Review of attachment 59826 [details] [review]: ----------------------------------------------------------------- We should fix this before 1.6, and possibly also in a 1.4.x release. ::: dbus/dbus-sysdeps-unix.c @@ +2631,5 @@ > + * @param tv_sec return location for number of seconds > + * @param tv_usec return location for number of microseconds (thousandths) > + */ > +void > +_dbus_get_current_time_real (long *tv_sec, I'd prefer _dbus_get_real_time(). Even better would be if you renamed _dbus_get_current_time() to _dbus_get_monotonic_time() to make the difference clearer: this would also reassure me that someone had looked at each caller and confirmed "yes, we do want monotonic time there". ::: dbus/dbus-sysdeps-win.c @@ +1904,5 @@ > +void > +_dbus_get_current_time_real (long *tv_sec, > + long *tv_usec) > +{ > + return _dbus_get_current_time (tv_sec, tv_usec); "return x()", where x returns void, is non-portable. Remove the return keyword. Assuming the Windows code only has wall-clock time and not monotonic time, I'd prefer this to be the other way round: void _dbus_get_monotonic_time (long *tv_sec, long *tv_usec) { /* no implementation yet, fall back to wall-clock time */ _dbus_get_real_time (tv_sec, tv_usec); } void _dbus_get_real_time() { [real implementation here] }
Created attachment 59858 [details] [review] Updated patch Thanks for the review, here's an updated patch.
Comment on attachment 59858 [details] [review] Updated patch Review of attachment 59858 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +2625,5 @@ > } > > /** > + * Get current time, as in gettimeofday(). Never uses the monotonic > + * clock. I'd prefer this one to be called _dbus_get_real_time() (or epoch_time or wallclock_time or something), so we no longer have a _dbus_get_current_time() at all, to avoid the ambiguity of "which one does 'current' mean?". ::: dbus/dbus-sysdeps-win.c @@ +1877,3 @@ > * > * @param tv_sec return location for number of seconds > + * @param tv_usec return location for number of microseconds (thousandths) A microsecond is a millionth. (This is a pre-existing bug, now I look more closely: the Unix version was documented wrong already.)
Created attachment 59863 [details] [review] Patch v3 Thanks again for the review. Updated patch attached.
Comment on attachment 59863 [details] [review] Patch v3 Review of attachment 59863 [details] [review]: ----------------------------------------------------------------- Yes please! Please merge to master, and to dbus-1.4 if you want it there.
Excellent, thanks for the quick review! master (will be in dbus 1.5.13): http://cgit.freedesktop.org/dbus/dbus/commit/?id=8734e4a16ff220a7af0fd718ba50f92d23c496cf dbus-1.4 (will be in dbus 1.4.21): http://cgit.freedesktop.org/dbus/dbus/commit/?h=dbus-1.4&id=870351439769a35344ead435f2490ed933dd8ff4
*** Bug 28564 has been marked as a duplicate of this bug. ***
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.