Bug 75863

Created attachment 95273 [details] [review]
Use binary mode for dbus daemon

> 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 bug description (unlike my ML message) does explain that part.

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

I've ifdef'ed on DBUS_WIN32 instead. Would that be sufficient?

> 
>> +#include <fcntl.h>
> 
> This would probably break MSVC, which I don't think has that
> Unix-specfic header; it needs to be properly conditional.

No, MSVC does have <fcntl.h>. O_BINARY and other consts are defined there, which is why i'm including it.

(In reply to comment #0)
> This is ok when working with a console, where user is reading. This is NOT
> ok when the output is fed to spawn_dbus_daemon(), which expects the output
> to use \n EOLs!

I'd prefer to make spawn_dbus_daemon() cope with \r\n text by stripping an optional trailing \r just before the \n, with something like

    if (address->str[address->len - 1] == '\r')
      g_string_truncate (address, address->len - 1);

If the dbus-daemon output really needs to be binary, a better thing to do would be to switch to [_]O_BINARY at the point when we need to output machine-readable information, which appears to be in bus_context_new(), and switch back to text mode afterwards if the pipe isn't closed. However, I don't think the dbus-daemon output really makes sense as binary: the possible machine-readable pieces of output are a process ID in ASCII decimal, and an a D-Bus address in ASCII, both of which I would characterize as primarily text.

The comments below only matter if you go the O_BINARY route, which I would not recommend unless there's a compelling reason to do so.

(In reply to comment #2)
> I've ifdef'ed on DBUS_WIN32 instead. Would that be sufficient?

The macro to use is DBUS_WIN. According to git grep, the only reference to DBUS_WIN32 appears to be in the CMake build system, where defined(DBUS_WIN32) == (defined(DBUS_WIN) && !defined(DBUS_WINCE)). Nothing tests it.

I would prefer to get rid of DBUS_WIN32 altogether, since it's misleading. It really means "non-CE Windows" - 64-bit Windows would also set it.

(In reply to comment #2)
> No, MSVC does have <fcntl.h>. O_BINARY and other consts are defined there,
> which is why i'm including it.

You say O_BINARY here, and the rest of D-Bus uses O_BINARY, but your patch uses _O_BINARY. What's the difference? Which ones are supported under MSVC? Which ones are supported under mingw?

What about _setmode()? Is that supported under MSVC, mingw or both?

FWIW, http://msdn.microsoft.com/en-us/library/tw4k6df8.aspx says _setmode requires io.h and optionally fcntl.h. We try to confine this use of system headers to dbus-{file,sysdeps,sysdeps-util,etc.}-{win,unix}.c, with some appropriate abstraction to minimize platform #ifdefs elsewhere.

The sysdeps-util files are the ones to use for things that only the dbus-daemon or other in-tree tools need - they don't go in the shared library.

(In reply to comment #3)
> (In reply to comment #0)
> > This is ok when working with a console, where user is reading. This is NOT
> > ok when the output is fed to spawn_dbus_daemon(), which expects the output
> > to use \n EOLs!
> 
> I'd prefer to make spawn_dbus_daemon() cope with \r\n text by stripping an
> optional trailing \r just before the \n, with something like
> 
>     if (address->str[address->len - 1] == '\r')
>       g_string_truncate (address, address->len - 1);

Makes sense. I'll try and report back with a new patch.

Created attachment 95335 [details] [review]
Handle 0x0d0a EOLs in spawn_dbus_daemon()

Looks good, will apply

Fixed in git for 1.8.2, 1.9.0