Created attachment 17786 [details] [review] my patch The attached patches make it possible to build dbus for Windows using the "normal" autofoo/libtool/make mechanism. ("normal" from the perspective of people used to this mechanism in a GTK+ stack context, for instance.) I also "cleaned" up the Windows-specific source files a bit, making their coding style be more in line with the other source fils. As such I didn't touch the way dbus works on Windows, it still uses localhost-bound TCP sockets for instance. I removed the IMHO pointless DBusFile abstraction used in the current dbus-sysdeps-win code, and dropped the dbus-sockets-win.h header. As far as I could see using dbus-monitor, the code after applying this works fine... I couldn't really run "make check", I didn't have time yet to make the test scripts work on Windows. As far as I can see, a dbus daemon compiled with these patches should be compatible with client using the "windbus" code and vice versa. The same name is used for the (local to the session) file mapping ("shared memory segment") where the address of the session bus is stored. However, I wasn't really able to test interoperability as the executables I built from the windbus source repository crashed immediately when starting. There is still much to do for dbus on Windows, here are some points: 1) the current dbus-launch-win.c source is mostly pointless, as it doesn't any command-line arguments like the UNIX dbus-launch.c does. It just starts dbus-daemon with the --session option. It might make more sense to use the dbus-launch.c source file with its option processing on both platforms, and move the platform dependent parts of it to separate unix and win files. One main point of the dbus-launch command is to be able to pass it for instance a --config-file option to make the started dbus-daemon use that config file. dbus-launch-win also doesn't print out any information, which is also a main point of the UNIX one I think? It should at least support the --sh-syntax option, and maybe also a --cmd-syntax to print out the variable settings in cmd.exe syntax? 2) Would it make sense to use a more native and secure IPC mechanism than localhost-bound TCP sockets. Whether dbus will be used for security sensitive use cases on Windows I don't know. Using localhost-bound sockets of course means that there is no risk of somebody with malicious intent contacting a session bus from another machine, but still, Windows machines with multiple simultaneous users do exist (terminal servers), and on such nothing prevents a user from talking to the session bus of another user. (Finding out the session bus address is not hard.) Windows named pipes (not to be confusd with POSIX named pipes) is one possibility. As such that would be kinda a step backward, as named pipes *are* visible from the network even, but on the other hand, when using named pipes it is possible to verify the identity of the client. Or then something completely different... maybe just Windows messages, or some session-local mutex and shared memory stuff? Or just ignore this issue, TCP sockets work fine, and the user-specific secret file means that it isn't so insecure after all, doesn't it? I must confess I am not 100% clear how that stuff works... 3) Make the code work when installed in locations with arbitrary Unicode pathnames. Currently just the "ANSI" Win32 APIs are used. This is not *that* important for most users probably...
I guess best git practice would have quite a few separate commits for this (do you have that locally?) One macro comment, there was a _ton_ of stuff in the mailing list archives about how to do various things, and I don't remember any of it. But it's worth looking through some of it probably. e.g. I know there was discussion of named pipes. If you search for the names of the windbus authors, you should find the right threads. Thanks for working on this patch, some comments - I agree with you that localhost tcp is not great, in general some mechanism that is native/what-you-would-use-from-scratch-on-windows would be ideal imo. I wonder if the makefile conditional should be something like "if ENABLE_SYSTEM_BUS" in several places instead of "if DBUS_UNIX"; maybe we don't want the system bus on e.g. OS X either? In any case it would clarify why those things are conditionalized. - <listen>unix:tmpdir=@DBUS_SESSION_SOCKET_DIR@</listen> + <listen>@DBUS_SESSION_SOCKET@</listen> This variable should be called @DBUS_SESSION_DEFAULT_ADDRESS@ or something (the point is it's a dbus listen address, not a socket) The logging changes in dbus-message.c look good but unrelated to the windows parts of the patch, so maybe should be separate. (hopefully already separate in your local git repo) errno fixes in sysdeps-unix also look good but unrelated. The --publish-address stuff need not be #ifdef'd in main(); just call it unconditionally and have the function do nothing or print an error on unix. I would not relate it at all to --session; the --session command line switch should be equivalent to --config-file=/etc/dbus-1/session.conf with no side effects. Hmm. Isn't --publish-address pretty much the same was what's done by dbus-launch on Linux? Maybe it should be in dbus-launch instead? As you say, dbus-launch would need refactoring so the unix bits are abstracted out. dbus-launch code is kind of a confusing disaster unfortunately. +#ifndef DBUS_WIN extern char **environ; +#endif Is the issue here that windows already declares it? In a perfect world, this would be something like #ifndef HAVE_ENVIRON_DECLARATION but I guess this is OK with a comment added about why it's needed. Overall looks like a nice set of improvements. As it evolves, I would keep in mind that a lot of what dbus does on Unix just won't make sense on Windows; the system bus is one huge example, and a lot of the stuff in the dbus code is only interesting for the system bus, such as policies based on unix users and groups. Also specific auth mechanisms and transports and ways to locate the bus probably won't make sense. In all these cases, it's better to refactor so the stuff isn't available on windows, rather than trying to port or emulate unix things that don't make sense. Things like transports and authentication are already pluggable and configurable, so it's just a matter of adding new windows-specific mechanisms.
> I guess best git practice would have quite a few separate commits > for this (do you have that locally?) I must admit my git skills are almost zero still... I will have to look for some good tutorial instead of just pretending it's like SVN;) Separate commits for unrelated things of course is a good idea, yes. I don't have a local repository even (I think); I just did a git clone and then git pull now and then to update, and git diff HEAD to produce the patch... I did read the mailing list discussions about dbus on Windows. > ENABLE_SYSTEM_BUS" in several places instead of "if DBUS_UNIX"; > maybe we don't want the system bus on e.g. OS X either? Yup, good idea. Will do. > This variable should be called @DBUS_SESSION_DEFAULT_ADDRESS@ or something ' Ah yes. > errno fixes in sysdeps-unix also look good but unrelated. Yes, included that by mistake. (Have a separate bug opened for that.) Will work on it all according to your comments, thanks.
bug #14259 has a patch to use launchd to discover the bus on OS X - this may overlap with efforts to use an alternate bus-discovery mechanism on Windows. Probably worth having a look if only to avoid patch conflicts.
Yeah, we need to break this patch up into consumable parts. Tor, do you think you might be able to create a git branch? I committed a small part first, here's the level of patches that would be nice: commit e2decdf0f1d3ce9c845f98afad9452afb0291ab3 Author: Tor Lillqvist <tml@iki.fi> Date: Thu Sep 18 19:40:50 2008 -0400 [win32] Protect usage of SIGHUP with #ifdef Signed-off-by: Colin Walters <walters@verbum.org> diff --git a/bus/main.c b/bus/main.c index 161de19..51538fe 100644 --- a/bus/main.c +++ b/bus/main.c @@ -44,7 +44,6 @@ static void close_reload_pipe (void); static void signal_handler (int sig) { - DBusString str; switch (sig) { @@ -52,16 +51,20 @@ signal_handler (int sig) case SIGIO: /* explicit fall-through */ #endif /* DBUS_BUS_ENABLE_DNOTIFY_ON_LINUX */ +#ifdef SIGHUP case SIGHUP: - _dbus_string_init_const (&str, "foo"); - if ((reload_pipe[RELOAD_WRITE_END] > 0) && - !_dbus_write_socket (reload_pipe[RELOAD_WRITE_END], &str, 0, 1)) - { - _dbus_warn ("Unable to write to reload pipe.\n"); - close_reload_pipe (); - } + { + DBusString str; + _dbus_string_init_const (&str, "foo"); + if ((reload_pipe[RELOAD_WRITE_END] > 0) && + !_dbus_write_socket (reload_pipe[RELOAD_WRITE_END], &str, 0, 1)) + { + _dbus_warn ("Unable to write to reload pipe.\n"); + close_reload_pipe (); + } + } break; - +#endif case SIGTERM: _dbus_loop_quit (bus_context_get_loop (context)); break; @@ -458,7 +461,9 @@ main (int argc, char **argv) setup_reload_pipe (bus_context_get_loop (context)); +#ifdef SIGHUP _dbus_set_signal_handler (SIGHUP, signal_handler); +#endif _dbus_set_signal_handler (SIGTERM, signal_handler); #ifdef DBUS_BUS_ENABLE_DNOTIFY_ON_LINUX _dbus_set_signal_handler (SIGIO, signal_handler); diff --git a/tools/dbus-launch.c b/tools/dbus-launch.c index 216f743..139d0aa 100644 --- a/tools/dbus-launch.c +++ b/tools/dbus-launch.c @@ -402,7 +402,9 @@ signal_handler (int sig) { switch (sig) { +#ifdef SIGHUP case SIGHUP: +#endif case SIGTERM: got_sighup = TRUE; break;
Created attachment 30984 [details] patchset made by splitting the original patch I really like to see some of the changes in this patch go in. So, split the patch into small parts. None of the patches are reworked based on the suggestions. If Tor is busy, I could try to rework on the patches that I understand. Some notes: - I haven't tested the patches after split. - In the original patch the the minimum required version was bumped to WindowsXP, I instead made it only for MingW. On MSC, built code can run on Windows 2000. This issue is described here: http://lists.zerezo.com/mingw-users/msg04734.html - Rejected Tor's changes to volatile casts in atomic operations as they are outdated. - I tried keep every change in the original patch, however to make my the task of splitting easier, I ignored trivial cleanups like: o Indentation changes o Conversion to C style comments o Removing spaces at the end of the line o Remove documentation comments from dbus-sysdeps-win.c - I first applied windbus patches and made further changes wherever I felt the code is common to windbus changes that are not present here. - I have observed that there are only very minor differences with windbus tree after this patch is applied. So, perhaps it is a good time to declare the trees merged, if there is such a plan. - Patch order may not be the best.
Ugh, if you have worked just on my patch from 2008, I am afraid you have been doing mostly pointless work. There has been more development on the dbus-on-Windows front after that, and the latest is/was in a fork called "dbus4win". There exists also another, older, fork called "windbus". We don't need a third fork... In my opinion, what should be done now is not to work on that patch from 2008 any more, but instead work on getting dbus4win merged into upstream master. (The dbus4win fork includes applicable things from my 2008 patch above, or at least obsoletes it, I would say.) See http://lists.freedesktop.org/archives/dbus/2009-October/011857.html (which didn't get any follow-up, sigh) and also http://lists.freedesktop.org/archives/dbus/2009-October/011862.html (which did get a little discussion going).
I was totally oblivious to the existence of dbus4win. I must have been in a cave for the past many months :) I shall take a look the work. Thank you.
Is this patch after the merge obsolate ? If not it would be nice to have an updated patch based on the dbus master branch
Yeah, sure it is obsolete now. If anything more is needed for trunk, I will open a new bug once I look into dbus again.
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.