Bug 48580

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. ***