W32 testsuite results - should i be worried?

[Simon McVittie]

On 06/03/14 01:33, Yang Chengwei wrote:
> From the test.log, I see the test doesn't skip "test only for
> *nix", so I guess you build your windows binaries with mingw. This
> isn't the supported way, please use CMake.

Building Windows binaries with the mingw-w64 fork of MinGW *is* meant
to work. If it doesn't, please send bug reports and patches to
<https://bugs.freedesktop.org/enter_bug.cgi?product=dbus>.

Building Windows binaries with the original, non-fork MinGW
(mingw.org, mingw.sourceforge.net) is not currently a maintained
configuration. If you want to resurrect and maintain it, suggested
patches are welcome - but I'll probably say "no, try a better
compiler" if those patches look likely to break other configurations
or cause maintenance burden.

We only recently got the automated regression tests working on Windows
at all, and there are probably still issues. Again, bug reports and
patches to Bugzilla, please. If in doubt, develop portability changes
against git master (which will be D-Bus 1.9.0 when released) - any
changes that are too intrusive will not go into the 1.8.x stable
branch, which is primarily there to support use of D-Bus as an OS
service on Linux, and on any other Unix platforms where it already
works well.

Some brief comments on your patches, please send revised patches or
further discussion to Bugzilla:

0001-use-gcc-atomics.mingw.patch:
> -  return InterlockedIncrement (&atomic->value) - 1; +  return
> __sync_add_and_fetch(&atomic->value, 1)-1;

We can't accept this patch (at least, not as-is) because it will break
compilation with CMake + MSVC, which is also a configuration that the
maintainer of D-Bus on Windows supports. Are InterlockedIncrement()
and similar bits of Win32 API not available on your platform?

0002-binary-mode-for-the-deamon.mingw.patch:
> +  _setmode (_fileno (stdin), _O_BINARY);

What binary data does the dbus-daemon emit on stdout/stderr, or accept
from stdin? In other words, why is this needed?

This change would also break non-Windows platforms. If needed, adding
a function like _dbus_set_binary_mode (FILE *) or
_dbus_set_binary_mode (int fd) to dbus-sysdeps-util-{unix,win}.c would
be a better way; it would do nothing on Unix, and use O_BINARY (or
_O_BINARY if that's absolutely necessary) on Windows.

> +#include <fcntl.h>

This would probably break MSVC, which I don't think has that
Unix-specfic header; it needs to be properly conditional.

0004-fix-test.mingw.patch:

This looks like a workaround for something, but I'm not sure what.
Please open a bug describing the issue.

0005-fix-printf-madness.mingw.patch:

This looks like an incomplete fix for
<https://bugs.freedesktop.org/show_bug.cgi?id=35311>, which also has
an incomplete patch that I have reviewed. Please follow up there if
you are interested in fixing this bug. I would much prefer to use the
safer DBusString idiom, like I sketched out there: we have this
string-buffer abstraction for very good reasons, let's use it.

I note in passing that this implementation appears to have one of the
same bugs as the one I reviewed on Bugzilla: if the result of (in
Python-like pseudocode) s + " " + (msg % args) is exactly 1024 bytes
long, then it calls OutputDebugStringA(buf) while buf contains 1024
nonzero bytes and no zero-termination. This sort of thing is exactly
why we avoid direct use of the (v)snprintf() family in
security-sensitive code :-)

    S