Bug 75863 - W32: dbus daemon outputs stuff in text mode
Summary: W32: dbus daemon outputs stuff in text mode
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-03-07 00:31 UTC by LRN
Modified: 2014-04-30 18:55 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Use binary mode for dbus daemon (1004 bytes, patch)
2014-03-07 00:31 UTC, LRN
Details | Splinter Review
Handle 0x0d0a EOLs in spawn_dbus_daemon() (1.44 KB, patch)
2014-03-08 04:20 UTC, LRN
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description LRN 2014-03-07 00:31:16 UTC
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!
Comment 1 LRN 2014-03-07 00:31:58 UTC
Created attachment 95273 [details] [review]
Use binary mode for dbus daemon
Comment 2 LRN 2014-03-07 00:53:05 UTC
> 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.
Comment 3 Simon McVittie 2014-03-07 12:12:10 UTC
(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.
Comment 4 LRN 2014-03-07 12:15:27 UTC
(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.
Comment 5 LRN 2014-03-08 04:20:29 UTC
Created attachment 95335 [details] [review]
Handle 0x0d0a EOLs in spawn_dbus_daemon()
Comment 6 Simon McVittie 2014-04-30 17:39:49 UTC
Looks good, will apply
Comment 7 Simon McVittie 2014-04-30 18:55:53 UTC
Fixed in git for 1.8.2, 1.9.0


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.