Bug 16788

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.